Re: [Qemu-devel] [PATCH v2 3/5] block: initial VHDX driver support framework - supports open and probe

[Fam Zheng]

On Tue, 04/23 10:24, Jeff Cody wrote:
> +/* opens the specified header block from the VHDX file header section */
> +static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
> +{
> +    int ret = 0;
> +    vhdx_header *header1;
> +    vhdx_header *header2;
> +    uint64_t h1_seq = 0;
> +    uint64_t h2_seq = 0;
> +    uint8_t *buffer;
> +
> +    header1 = qemu_blockalign(bs, sizeof(vhdx_header));
> +    header2 = qemu_blockalign(bs, sizeof(vhdx_header));
> +
> +    buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE);
> +
> +    s->headers[0] = header1;
> +    s->headers[1] = header2;
> +
Logic for header1 and header2 are completely the same, IMO it might be
better to avoid code with a loop.
> +    /* We have to read the whole VHDX_HEADER_SIZE instead of
> +     * sizeof(vhdx_header), because the checksum is over the whole
> +     * region */
> +    ret = bdrv_pread(bs->file, VHDX_HEADER1_OFFSET, buffer, 
> VHDX_HEADER_SIZE);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    /* copy over just the relevant portion that we need */
> +    memcpy(header1, buffer, sizeof(vhdx_header));
> +    vhdx_header_le_import(header1);
> +
> +    if (vhdx_checksum_is_valid(buffer, VHDX_HEADER_SIZE, 4) &&
> +        header1->signature == VHDX_HDR_MAGIC) {
> +        h1_seq = header1->sequence_number;
> +    }
Do we need to check the version here? As the specification page 15 says:

    The Version field specifies the version of the VHDX format used
    within the VHDX file. This field must be set to 1. If it is not, a
    parser must not attempt to parse the file using the details from
    this format specification.

> +
> +    ret = bdrv_pread(bs->file, VHDX_HEADER2_OFFSET, buffer, 
> VHDX_HEADER_SIZE);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    /* copy over just the relevant portion that we need */
> +    memcpy(header2, buffer, sizeof(vhdx_header));
> +    vhdx_header_le_import(header2);
> +
> +    if (vhdx_checksum_is_valid(buffer, VHDX_HEADER_SIZE, 4) &&
> +        header2->signature == VHDX_HDR_MAGIC) {
> +        h2_seq = header2->sequence_number;
> +    }
Same as above.
> +
> +    if (h1_seq > h2_seq) {
> +        s->curr_header = 0;
> +    } else if (h2_seq > h1_seq) {
> +        s->curr_header = 1;
> +    } else {
> +        printf("NO VALID HEADER\n");
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    ret = 0;
> +
> +    goto exit;
> +
> +fail:
> +    qemu_vfree(header1);
> +    qemu_vfree(header2);
> +    s->headers[0] = NULL;
> +    s->headers[1] = NULL;
> +exit:
> +    qemu_vfree(buffer);
> +    return ret;
> +}

-- 
Fam