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

[Stefan Hajnoczi]

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(&current, 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.