Re: [Qemu-devel] [PATCH 6/7] block: add header update capability for VHDX images.

[Stefan Hajnoczi]

On Wed, Mar 06, 2013 at 09:48:33AM -0500, Jeff Cody wrote:
> This adds the ability to update the headers in a VHDX image, including
> generating a new MS-compatible GUID, and checksum.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/vhdx.c | 165 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 163 insertions(+), 2 deletions(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 97775d2..13d1e7f 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -27,6 +27,11 @@
>      le16_to_cpus(&(guid)->data2); \
>      le16_to_cpus(&(guid)->data3); } while (0)
>  
> +#define cpu_to_leguids(guid) do { \
> +    cpu_to_le32s(&(guid)->data1); \
> +    cpu_to_le16s(&(guid)->data2); \
> +    cpu_to_le16s(&(guid)->data3); } while (0)
> +

Please use a function instead of a macro.

> +/* Calculates new checksum.
> + *
> + * Zero is substituted during crc calculation for the original crc field
> + * crc_offset: byte offset in buf of the buffer crc
> + * buf: buffer pointer
> + * size: size of buffer (must be > crc_offset+4)
> + */
> +static uint32_t vhdx_update_checksum(uint8_t *buf, size_t size, int 
> crc_offset)
> +{
> +    uint32_t crc;
> +
> +    assert(buf != NULL);
> +    assert(size > (crc_offset+4));
> +
> +    memset(buf+crc_offset, 0, sizeof(crc));
> +    crc =  crc32c(0xffffffff, buf, size);
> +    memcpy(buf+crc_offset, &crc, sizeof(crc));

Worth a comment that the checksum is in CPU endianness.  Must use
vhdx_header_le_export() before writing to file.  That implies the buffer
must also be in CPU endianness otherwise vhdx_header_le_export() will
byteswap unwanted fields.

> +
> +    return crc;
> +}
> +
>  /* Validates the checksum of the buffer, with an in-place CRC.
>   *
>   * Zero is substituted during crc calculation for the original crc field,
> @@ -198,6 +227,33 @@ static 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

Do you mean libuuid is not available on all host platforms supported by
QEMU, or just that you wanted to avoid the dependency?

Since other parts of QEMU already use libuuid I'd rather see it used
than reimplemented in VHDX.