Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.

[Stefan Hajnoczi]

On Wed, Jul 31, 2013 at 11:23:47PM -0400, Jeff Cody wrote:
> @@ -212,6 +242,24 @@ bool vhdx_checksum_is_valid(uint8_t *buf, size_t size, 
> int crc_offset)
>  
>  
>  /*
> + * This generates a UUID that is compliant with the MS GUIDs used
> + * in the VHDX spec (and elsewhere).
> + *
> + * We can do this with uuid_generate if uuid.h is present,
> + * however not all systems have uuid and the generation is
> + * pretty straightforward for the DCE + random usage case

The talk of uuid.h not being available confuses me.  The code has no
#ifdef and we do not define uuid_generate() ourselves.

Is this an outdated comment?

> +/* Update the VHDX headers
> + *
> + * This follows the VHDX spec procedures for header updates.
> + *
> + *  - non-current header is updated with largest sequence number
> + */
> +static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s, bool 
> rw)

"rw" should be named "generate_file_write_guid".  If you call
vhdx_update_header() again later you do not need to update FileWriteGuid
again according to the spec.  There's probably no harm in doing so but
the spec explicitly says "If this is the first header update within the
session, use a new value for the FileWriteGuid".

This means that future calls to vhdx_update_header() in the same session
should set "generate_file_write_guid" to false.  Therefore "rw" is not
the right name.

> +{
> +    int ret = 0;
> +    int hdr_idx = 0;
> +    uint64_t header_offset = VHDX_HEADER1_OFFSET;
> +
> +    VHDXHeader *active_header;
> +    VHDXHeader *inactive_header;
> +    VHDXHeader header_le;
> +    uint8_t *buffer;
> +
> +    /* operate on the non-current header */
> +    if (s->curr_header == 0) {
> +        hdr_idx = 1;
> +        header_offset = VHDX_HEADER2_OFFSET;
> +    }
> +
> +    active_header   = s->headers[s->curr_header];
> +    inactive_header = s->headers[hdr_idx];
> +
> +    inactive_header->sequence_number = active_header->sequence_number + 1;
> +
> +    /* a new file guid must be generate before any file write, including

s/generate/generated/

> +     * headers */
> +    memcpy(&inactive_header->file_write_guid, &s->session_guid,
> +           sizeof(MSGUID));
> +
> +    /* a new data guid only needs to be generate before any guest-visible
> +     * writes, so update it if the image is opened r/w. */
> +    if (rw) {
> +        vhdx_guid_generate(&inactive_header->data_write_guid);
> +    }
> +
> +    /* the header checksum is not over just the packed size of VHDXHeader,
> +     * but rather over the entire 'reserved' range for the header, which is
> +     * 4KB (VHDX_HEADER_SIZE). */
> +
> +    buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE);
> +    /* we can't assume the extra reserved bytes are 0 */
> +    ret = bdrv_pread(bs->file, header_offset, buffer, VHDX_HEADER_SIZE);
> +    if (ret < 0) {
> +        goto exit;
> +    }
> +    /* overwrite the actual VHDXHeader portion */
> +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> +    inactive_header->checksum = vhdx_update_checksum(buffer,
> +                                                     VHDX_HEADER_SIZE, 4);
> +    vhdx_header_le_export(inactive_header, &header_le);
> +    bdrv_pwrite_sync(bs->file, header_offset, &header_le, 
> sizeof(VHDXHeader));

This function can fail and it's a good indicator that future I/Os will
also be in trouble.  Please propagate the error return.

> @@ -801,8 +955,7 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> *options, int flags)
>      }
>  
>      if (flags & BDRV_O_RDWR) {
> -        ret = -ENOTSUP;
> -        goto fail;
> +        vhdx_update_headers(bs, s, false);

Error handling is missing.  At this point it looks like we cannot write
to the file.  Propagating the error seems reasonable.