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

[Jeff Cody <jcody@redhat.com>]

On Thu, Mar 07, 2013 at 03:50:56PM +0100, Stefan Hajnoczi wrote:
> 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 <jcody@redhat.com>
> > ---
> >  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.
> 

OK.

> > +/* 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.
>

OK.

> > +
> > +    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?
>

The former, I am assuming by the configure check for uuid and
CONFIG_UUID.

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

There are two other users of uuid.h - block/vpc.c and block/vdi.c.
Both uses are wrapped in an #ifdef for CONFIG_UUID, and if
uuid_generate is not present they just leave the uuid field with
zeroes.

At least for the VHDX spec, there are several places where a GUID /
UUID of zero has a special meaning.  So if CONFIG_UUID is not present,
something needs to be generated - and generating a valid GUID that is
compatible with the spec is pretty trivial, which is why I just went
ahead and did that.

I also thought about adding this into util/ or the like, so that if
uuid.h is not available, vdi and vpc could use it.  However, I wasn't
certain of the requirements for vdi and vpc itself, so I didn't do
that yet.


Jeff