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

[Stefan Hajnoczi]

On Thu, Mar 07, 2013 at 10:33:49AM -0500, Jeff Cody wrote:
> 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 <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.
> > 
> 
> 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.

No, the configure check just lets you build without libuuid, if you
don't want to install the headers :).  In block/vpc.c it's an optional
dependency.

> > 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.

Exactly, in the general case the problem becomes harder and then you're
really reimplementing libuuid.

I suggest using libuuid and building in block/vhdx.o only when it is
available.

Stefa