[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/7] block: add header update capability for VHD
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 6/7] block: add header update capability for VHDX images. |
Date: |
Thu, 7 Mar 2013 17:15:31 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- Re: [Qemu-devel] [PATCH 3/7] block: vhdx header for the QEMU support of VHDX images, (continued)
[Qemu-devel] [PATCH 5/7] block: add read-only support to VHDX image format., Jeff Cody, 2013/03/06
[Qemu-devel] [PATCH 6/7] block: add header update capability for VHDX images., Jeff Cody, 2013/03/06
[Qemu-devel] [PATCH 7/7] block: add write support for VHDX images, Jeff Cody, 2013/03/06