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:
> +/* 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); > +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) { > +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. > +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. > + 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.