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.
