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