Re: [Qemu-devel] [PATCH v4 07/13] block: vhdx - log parsing, replay, and flush support

[Jeff Cody]

On Wed, Aug 21, 2013 at 05:09:30PM +0200, Stefan Hajnoczi wrote:
> On Tue, Aug 20, 2013 at 02:01:18AM -0400, Jeff Cody wrote:
> 
> Will require more iterations of review, but here's what I have so far:
> 

Yeah, I assumed it would take a few iterations of review.

> > +/* Returns true if the GUID is zero */
> > +static bool vhdx_log_guid_is_zero(MSGUID *guid)
> > +{
> > +    int i;
> > +    int ret = 0;
> > +
> > +    /* If either the log guid, or log length is zero,
> > +     * then a replay log is not present */
> 
> The comment about log length here is not relevant to this function.
> 
> > +    for (i = 0; i < sizeof(MSGUID); i++) {
> > +        ret |= ((uint8_t *) guid)[i];
> > +    }
> > +
> > +    return ret == 0;
> > +}
> 
> IMO there is no need for this function.  Just declare a const MSGUID
> zero_guid = {0} global and use memcmp():
> 
>   is_zero = guid_eq(guid, zero_guid);
> 

OK, sounds good to me.

> > +static bool vhdx_log_desc_is_valid(VHDXLogDescriptor *desc,
> > +                                   VHDXLogEntryHeader *hdr)
> > +{
> > +    bool ret = false;
> > +
> > +    if (desc->sequence_number != hdr->sequence_number) {
> > +        goto exit;
> > +    }
> > +    if (desc->file_offset % VHDX_LOG_SECTOR_SIZE) {
> > +        goto exit;
> > +    }
> > +
> > +    if (!memcmp(&desc->signature, "zero", 4)) {
> > +        if (!desc->zero_length % VHDX_LOG_SECTOR_SIZE) {
> 
> Precedence looks wrong here, did you mean:
> 
> if (desc->zero_length % VHDX_LOG_SECTOR_SIZE == 0) {
> 

You are correct.  The precedence is wrong - the modulo should have
been encapsulated in (), but the explicit == is better.

This never triggered anything in my testing because the internal code
does not use zero descriptors, and none of the logs I was able to
produce from Hyper-V contained zero descriptors.

> > +static int vhdx_log_search(BlockDriverState *bs, BDRVVHDXState *s,
> > +                           VHDXLogSequence *logs)
> > +{
> > +    int ret = 0;
> > +
> > +    uint64_t curr_seq = 0;
> > +    VHDXLogSequence candidate = { 0 };
> > +    VHDXLogSequence current = { 0 };
> > +
> > +    uint32_t tail;
> > +    bool seq_valid = false;
> > +    VHDXLogEntryHeader hdr = { 0 };
> > +    VHDXLogEntries curr_log;
> > +
> > +    memcpy(&curr_log, &s->log, sizeof(VHDXLogEntries));
> > +    curr_log.write = curr_log.length;   /* assume log is full */
> > +    curr_log.read = 0;
> > +
> > +
> > +    /* now we will go through the whole log sector by sector, until
> > +     * we find a valid, active log sequence, or reach the end of the
> > +     * log buffer */
> > +    for (;;) {
> > +        tail = curr_log.read;
> > +
> > +        curr_seq = 0;
> > +        memset(&current, 0, sizeof(current));
> 
> You could declare curr_seq, current, and friends inside the for loop
> scope to avoid duplicate initializations.
>

OK

> > +int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s)
> > +{
> > +    int ret = 0;
> > +    VHDXHeader *hdr;
> > +    VHDXLogSequence logs = { 0 };
> > +
> > +    hdr = s->headers[s->curr_header];
> > +
> > +    /* s->log.hdr is freed in vhdx_close() */
> 
> vhdx_close() is not called when .bdrv_open() fails so s->log.hdr is
> leaked.
> 

Good point.  I'll look and see to make sure, but I think I can just
have vhdx_open() call vhdx_close() to cleanup on error, assuming
everything is NULL initialized.

> > +    if (s->log.hdr == NULL) {
> > +        s->log.hdr = qemu_blockalign(bs, sizeof(VHDXLogEntryHeader));
> > +    }
> > +
> > +    s->log.offset = hdr->log_offset;
> > +    s->log.length = hdr->log_length;
> > +
> > +    if (s->log.offset < VHDX_LOG_MIN_SIZE ||
> > +        s->log.offset % VHDX_LOG_MIN_SIZE) {
> > +        ret = -EINVAL;
> > +        goto exit;
> > +    }
> 
> To be completely safe we should probably ensure that the log does not
> overlap any other structures, as mentioned in the spec.

I agree.

Thanks for the review,

Jeff