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(¤t, 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