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