Patchwork [v2,2/9] block: vhdx - add header update capability.

login
register
mail settings
Submitter Jeff Cody
Date Aug. 1, 2013, 3:23 a.m.
Message ID <595d71cf1cd25ac9deff521bceb82b8c3da888d0.1375325971.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/263894/
State New
Headers show

Comments

Jeff Cody - Aug. 1, 2013, 3:23 a.m.
This adds the ability to update the headers in a VHDX image, including
generating a new MS-compatible GUID.

As VHDX depends on uuid.h, VHDX is now a configurable build option.  If
VHDX support is enabled, that will also enable uuid as well.  The
default is to have VHDX enabled.

To enable/disable VHDX:  --enable-vhdx, --disable-vhdx

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/Makefile.objs |   2 +-
 block/vhdx.c        | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/vhdx.h        |  12 +++-
 configure           |  13 +++++
 4 files changed, 180 insertions(+), 4 deletions(-)
Stefan Hajnoczi - Aug. 1, 2013, 1:44 p.m.
On Wed, Jul 31, 2013 at 11:23:47PM -0400, Jeff Cody wrote:
> @@ -212,6 +242,24 @@ 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
> + * pretty straightforward for the DCE + random usage case

The talk of uuid.h not being available confuses me.  The code has no
#ifdef and we do not define uuid_generate() ourselves.

Is this an outdated comment?

> +/* Update the VHDX headers
> + *
> + * This follows the VHDX spec procedures for header updates.
> + *
> + *  - non-current header is updated with largest sequence number
> + */
> +static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s, bool rw)

"rw" should be named "generate_file_write_guid".  If you call
vhdx_update_header() again later you do not need to update FileWriteGuid
again according to the spec.  There's probably no harm in doing so but
the spec explicitly says "If this is the first header update within the
session, use a new value for the FileWriteGuid".

This means that future calls to vhdx_update_header() in the same session
should set "generate_file_write_guid" to false.  Therefore "rw" is not
the right name.

> +{
> +    int ret = 0;
> +    int hdr_idx = 0;
> +    uint64_t header_offset = VHDX_HEADER1_OFFSET;
> +
> +    VHDXHeader *active_header;
> +    VHDXHeader *inactive_header;
> +    VHDXHeader header_le;
> +    uint8_t *buffer;
> +
> +    /* operate on the non-current header */
> +    if (s->curr_header == 0) {
> +        hdr_idx = 1;
> +        header_offset = VHDX_HEADER2_OFFSET;
> +    }
> +
> +    active_header   = s->headers[s->curr_header];
> +    inactive_header = s->headers[hdr_idx];
> +
> +    inactive_header->sequence_number = active_header->sequence_number + 1;
> +
> +    /* a new file guid must be generate before any file write, including

s/generate/generated/

> +     * headers */
> +    memcpy(&inactive_header->file_write_guid, &s->session_guid,
> +           sizeof(MSGUID));
> +
> +    /* a new data guid only needs to be generate before any guest-visible
> +     * writes, so update it if the image is opened r/w. */
> +    if (rw) {
> +        vhdx_guid_generate(&inactive_header->data_write_guid);
> +    }
> +
> +    /* the header checksum is not over just the packed size of VHDXHeader,
> +     * but rather over the entire 'reserved' range for the header, which is
> +     * 4KB (VHDX_HEADER_SIZE). */
> +
> +    buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE);
> +    /* we can't assume the extra reserved bytes are 0 */
> +    ret = bdrv_pread(bs->file, header_offset, buffer, VHDX_HEADER_SIZE);
> +    if (ret < 0) {
> +        goto exit;
> +    }
> +    /* overwrite the actual VHDXHeader portion */
> +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> +    inactive_header->checksum = vhdx_update_checksum(buffer,
> +                                                     VHDX_HEADER_SIZE, 4);
> +    vhdx_header_le_export(inactive_header, &header_le);
> +    bdrv_pwrite_sync(bs->file, header_offset, &header_le, sizeof(VHDXHeader));

This function can fail and it's a good indicator that future I/Os will
also be in trouble.  Please propagate the error return.

> @@ -801,8 +955,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
>      }
>  
>      if (flags & BDRV_O_RDWR) {
> -        ret = -ENOTSUP;
> -        goto fail;
> +        vhdx_update_headers(bs, s, false);

Error handling is missing.  At this point it looks like we cannot write
to the file.  Propagating the error seems reasonable.
Jeff Cody - Aug. 1, 2013, 2:09 p.m.
On Thu, Aug 01, 2013 at 03:44:41PM +0200, Stefan Hajnoczi wrote:
> On Wed, Jul 31, 2013 at 11:23:47PM -0400, Jeff Cody wrote:
> > @@ -212,6 +242,24 @@ 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
> > + * pretty straightforward for the DCE + random usage case
> 
> The talk of uuid.h not being available confuses me.  The code has no
> #ifdef and we do not define uuid_generate() ourselves.
> 
> Is this an outdated comment?

Yes, the comment is outdated from when I had my own UUID generation in
the patch - I missed removing the comment.
> 
> > +/* Update the VHDX headers
> > + *
> > + * This follows the VHDX spec procedures for header updates.
> > + *
> > + *  - non-current header is updated with largest sequence number
> > + */
> > +static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s, bool rw)
> 
> "rw" should be named "generate_file_write_guid".  If you call
> vhdx_update_header() again later you do not need to update FileWriteGuid
> again according to the spec.  There's probably no harm in doing so but
> the spec explicitly says "If this is the first header update within the
> session, use a new value for the FileWriteGuid".
> 
> This means that future calls to vhdx_update_header() in the same session
> should set "generate_file_write_guid" to false.  Therefore "rw" is not
> the right name.
> 

Yes, it is a bit misnamed.  I'll change the name. However, redundant
generation is protected against with via vhdx_user_visible_write(),
introduced in patch 6, which is the only placed that calls it with rw
= true (soon to be generate_file_write_guid = true).

> > +{
> > +    int ret = 0;
> > +    int hdr_idx = 0;
> > +    uint64_t header_offset = VHDX_HEADER1_OFFSET;
> > +
> > +    VHDXHeader *active_header;
> > +    VHDXHeader *inactive_header;
> > +    VHDXHeader header_le;
> > +    uint8_t *buffer;
> > +
> > +    /* operate on the non-current header */
> > +    if (s->curr_header == 0) {
> > +        hdr_idx = 1;
> > +        header_offset = VHDX_HEADER2_OFFSET;
> > +    }
> > +
> > +    active_header   = s->headers[s->curr_header];
> > +    inactive_header = s->headers[hdr_idx];
> > +
> > +    inactive_header->sequence_number = active_header->sequence_number + 1;
> > +
> > +    /* a new file guid must be generate before any file write, including
> 
> s/generate/generated/
> 

Thanks

> > +     * headers */
> > +    memcpy(&inactive_header->file_write_guid, &s->session_guid,
> > +           sizeof(MSGUID));
> > +
> > +    /* a new data guid only needs to be generate before any guest-visible
> > +     * writes, so update it if the image is opened r/w. */
> > +    if (rw) {
> > +        vhdx_guid_generate(&inactive_header->data_write_guid);
> > +    }
> > +
> > +    /* the header checksum is not over just the packed size of VHDXHeader,
> > +     * but rather over the entire 'reserved' range for the header, which is
> > +     * 4KB (VHDX_HEADER_SIZE). */
> > +
> > +    buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE);
> > +    /* we can't assume the extra reserved bytes are 0 */
> > +    ret = bdrv_pread(bs->file, header_offset, buffer, VHDX_HEADER_SIZE);
> > +    if (ret < 0) {
> > +        goto exit;
> > +    }
> > +    /* overwrite the actual VHDXHeader portion */
> > +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> > +    inactive_header->checksum = vhdx_update_checksum(buffer,
> > +                                                     VHDX_HEADER_SIZE, 4);
> > +    vhdx_header_le_export(inactive_header, &header_le);
> > +    bdrv_pwrite_sync(bs->file, header_offset, &header_le, sizeof(VHDXHeader));
> 
> This function can fail and it's a good indicator that future I/Os will
> also be in trouble.  Please propagate the error return.
> 

You are right, that was unintentional.

> > @@ -801,8 +955,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
> >      }
> >  
> >      if (flags & BDRV_O_RDWR) {
> > -        ret = -ENOTSUP;
> > -        goto fail;
> > +        vhdx_update_headers(bs, s, false);
> 
> Error handling is missing.  At this point it looks like we cannot write
> to the file.  Propagating the error seems reasonable.

OK, I move the above change into when the write support is actually in
place.
Kevin Wolf - Aug. 5, 2013, 11:42 a.m.
Am 01.08.2013 um 05:23 hat Jeff Cody geschrieben:
> This adds the ability to update the headers in a VHDX image, including
> generating a new MS-compatible GUID.
> 
> As VHDX depends on uuid.h, VHDX is now a configurable build option.  If
> VHDX support is enabled, that will also enable uuid as well.  The
> default is to have VHDX enabled.
> 
> To enable/disable VHDX:  --enable-vhdx, --disable-vhdx
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>

> diff --git a/configure b/configure
> index 877a821..821b790 100755
> --- a/configure
> +++ b/configure
> @@ -244,6 +244,7 @@ gtk=""
>  gtkabi="2.0"
>  tpm="no"
>  libssh2=""
> +vhdx="yes"
>  
>  # parse CC options first
>  for opt do
> @@ -950,6 +951,11 @@ for opt do
>    ;;
>    --enable-libssh2) libssh2="yes"
>    ;;
> +  --enable-vhdx) vhdx="yes" ;
> +                 uuid="yes"
> +  ;;

What's this? Shouldn't auto-detecting uuid and checking later whether
it's available do the right thing?

Test cases:

1. configure --disable-uuid --enable-vhdx
   This should result in an error because the options contradict each
   other. With this patch, the disable flag is overridden.

2. No libuuid headers available; ./configure
   Should build without VHDX. With this patch, uuid is left out, vhdx is
   configured anyway, the build fails.

Kevin
Jeff Cody - Aug. 5, 2013, 2:43 p.m.
On Mon, Aug 05, 2013 at 01:42:32PM +0200, Kevin Wolf wrote:
> Am 01.08.2013 um 05:23 hat Jeff Cody geschrieben:
> > This adds the ability to update the headers in a VHDX image, including
> > generating a new MS-compatible GUID.
> > 
> > As VHDX depends on uuid.h, VHDX is now a configurable build option.  If
> > VHDX support is enabled, that will also enable uuid as well.  The
> > default is to have VHDX enabled.
> > 
> > To enable/disable VHDX:  --enable-vhdx, --disable-vhdx
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> 
> > diff --git a/configure b/configure
> > index 877a821..821b790 100755
> > --- a/configure
> > +++ b/configure
> > @@ -244,6 +244,7 @@ gtk=""
> >  gtkabi="2.0"
> >  tpm="no"
> >  libssh2=""
> > +vhdx="yes"
> >  
> >  # parse CC options first
> >  for opt do
> > @@ -950,6 +951,11 @@ for opt do
> >    ;;
> >    --enable-libssh2) libssh2="yes"
> >    ;;
> > +  --enable-vhdx) vhdx="yes" ;
> > +                 uuid="yes"
> > +  ;;
> 
> What's this? Shouldn't auto-detecting uuid and checking later whether
> it's available do the right thing?
>

You are right, thanks.  Will fix for v3.

> Test cases:
> 
> 1. configure --disable-uuid --enable-vhdx
>    This should result in an error because the options contradict each
>    other. With this patch, the disable flag is overridden.
> 
> 2. No libuuid headers available; ./configure
>    Should build without VHDX. With this patch, uuid is left out, vhdx is
>    configured anyway, the build fails.
> 
> Kevin
Kevin Wolf - Aug. 5, 2013, 2:45 p.m.
Am 01.08.2013 um 05:23 hat Jeff Cody geschrieben:
> This adds the ability to update the headers in a VHDX image, including
> generating a new MS-compatible GUID.
> 
> As VHDX depends on uuid.h, VHDX is now a configurable build option.  If
> VHDX support is enabled, that will also enable uuid as well.  The
> default is to have VHDX enabled.
> 
> To enable/disable VHDX:  --enable-vhdx, --disable-vhdx
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/Makefile.objs |   2 +-
>  block/vhdx.c        | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  block/vhdx.h        |  12 +++-
>  configure           |  13 +++++
>  4 files changed, 180 insertions(+), 4 deletions(-)
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 4cf9aa4..e5e54e6 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -2,7 +2,7 @@ block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat
>  block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
> -block-obj-y += vhdx.o
> +block-obj-$(CONFIG_VHDX) += vhdx.o
>  block-obj-y += parallels.o blkdebug.o blkverify.o
>  block-obj-y += snapshot.o qapi.o
>  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 56bc88e..9b50442 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -21,6 +21,7 @@
>  #include "qemu/crc32c.h"
>  #include "block/vhdx.h"
>  
> +#include <uuid/uuid.h>
>  
>  /* Several metadata and region table data entries are identified by
>   * guids in  a MS-specific GUID format. */
> @@ -156,11 +157,40 @@ typedef struct BDRVVHDXState {
>      VHDXBatEntry *bat;
>      uint64_t bat_offset;
>  
> +    MSGUID session_guid;
> +
> +
>      VHDXParentLocatorHeader parent_header;
>      VHDXParentLocatorEntry *parent_entries;
>  
>  } BDRVVHDXState;
>  
> +/* 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)
> + *
> + * Note: The resulting checksum is in the CPU endianness, not necessarily
> + *       in the file format endianness (LE).  Any header export to disk should
> + *       make sure that vhdx_header_le_export() is used to convert to the
> + *       correct endianness
> + */
> +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));

sizeof(crc) for consistency.

> +
> +    memset(buf + crc_offset, 0, sizeof(crc));
> +    crc =  crc32c(0xffffffff, buf, size);
> +    memcpy(buf + crc_offset, &crc, sizeof(crc));
> +
> +    return crc;
> +}
> +
>  uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
>                              int crc_offset)
>  {
> @@ -212,6 +242,24 @@ 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
> + * pretty straightforward for the DCE + random usage case
> + *
> + */
> +void vhdx_guid_generate(MSGUID *guid)
> +{
> +    uuid_t uuid;
> +    assert(guid != NULL);
> +
> +    uuid_generate(uuid);
> +    memcpy(guid, uuid, 16);
> +}

Should MSGUID be QEMU_PACKED to make sure that this works? Also
sizeof(guid) instead of 16?

> +
> +/*
>   * Per the MS VHDX Specification, for every VHDX file:
>   *      - The header section is fixed size - 1 MB
>   *      - The header section is always the first "object"
> @@ -249,6 +297,107 @@ static void vhdx_header_le_import(VHDXHeader *h)
>      le64_to_cpus(&h->log_offset);
>  }
>  
> +/* All VHDX structures on disk are little endian */
> +static void vhdx_header_le_export(VHDXHeader *orig_h, VHDXHeader *new_h)
> +{
> +    assert(orig_h != NULL);
> +    assert(new_h != NULL);
> +
> +    new_h->signature       = cpu_to_le32(orig_h->signature);
> +    new_h->checksum        = cpu_to_le32(orig_h->checksum);
> +    new_h->sequence_number = cpu_to_le64(orig_h->sequence_number);
> +
> +    memcpy(&new_h->file_write_guid, &orig_h->file_write_guid, sizeof(MSGUID));
> +    memcpy(&new_h->data_write_guid, &orig_h->data_write_guid, sizeof(MSGUID));
> +    memcpy(&new_h->log_guid,        &orig_h->log_guid,        sizeof(MSGUID));

Can we avoid memcpy? I think this should work:

new_h->file_write_guid = orig_h->file_write_guid;
new_h->data_write_guid = orig_h->data_write_guid;
new_h->log_guid        = orig_h->log_guid;

> +
> +    cpu_to_leguids(&new_h->file_write_guid);
> +    cpu_to_leguids(&new_h->data_write_guid);
> +    cpu_to_leguids(&new_h->log_guid);
> +
> +    new_h->log_version     = cpu_to_le16(orig_h->log_version);
> +    new_h->version         = cpu_to_le16(orig_h->version);
> +    new_h->log_length      = cpu_to_le32(orig_h->log_length);
> +    new_h->log_offset      = cpu_to_le64(orig_h->log_offset);
> +}
> +
> +/* Update the VHDX headers
> + *
> + * This follows the VHDX spec procedures for header updates.
> + *
> + *  - non-current header is updated with largest sequence number
> + */
> +static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s, bool rw)
> +{
> +    int ret = 0;
> +    int hdr_idx = 0;
> +    uint64_t header_offset = VHDX_HEADER1_OFFSET;
> +
> +    VHDXHeader *active_header;
> +    VHDXHeader *inactive_header;
> +    VHDXHeader header_le;
> +    uint8_t *buffer;
> +
> +    /* operate on the non-current header */
> +    if (s->curr_header == 0) {
> +        hdr_idx = 1;
> +        header_offset = VHDX_HEADER2_OFFSET;
> +    }
> +
> +    active_header   = s->headers[s->curr_header];
> +    inactive_header = s->headers[hdr_idx];
> +
> +    inactive_header->sequence_number = active_header->sequence_number + 1;
> +
> +    /* a new file guid must be generate before any file write, including
> +     * headers */
> +    memcpy(&inactive_header->file_write_guid, &s->session_guid,
> +           sizeof(MSGUID));

Here a simple assignment should do as well.

> +
> +    /* a new data guid only needs to be generate before any guest-visible
> +     * writes, so update it if the image is opened r/w. */
> +    if (rw) {
> +        vhdx_guid_generate(&inactive_header->data_write_guid);
> +    }
> +
> +    /* the header checksum is not over just the packed size of VHDXHeader,
> +     * but rather over the entire 'reserved' range for the header, which is
> +     * 4KB (VHDX_HEADER_SIZE). */
> +
> +    buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE);
> +    /* we can't assume the extra reserved bytes are 0 */
> +    ret = bdrv_pread(bs->file, header_offset, buffer, VHDX_HEADER_SIZE);
> +    if (ret < 0) {
> +        goto exit;
> +    }
> +    /* overwrite the actual VHDXHeader portion */
> +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> +    inactive_header->checksum = vhdx_update_checksum(buffer,
> +                                                     VHDX_HEADER_SIZE, 4);
> +    vhdx_header_le_export(inactive_header, &header_le);
> +    bdrv_pwrite_sync(bs->file, header_offset, &header_le, sizeof(VHDXHeader));
> +    s->curr_header = hdr_idx;
> +
> +exit:
> +    qemu_vfree(buffer);
> +    return ret;
> +}

Kevin
Jeff Cody - Aug. 5, 2013, 2:57 p.m.
On Mon, Aug 05, 2013 at 04:45:02PM +0200, Kevin Wolf wrote:
> Am 01.08.2013 um 05:23 hat Jeff Cody geschrieben:
> > This adds the ability to update the headers in a VHDX image, including
> > generating a new MS-compatible GUID.
> > 
> > As VHDX depends on uuid.h, VHDX is now a configurable build option.  If
> > VHDX support is enabled, that will also enable uuid as well.  The
> > default is to have VHDX enabled.
> > 
> > To enable/disable VHDX:  --enable-vhdx, --disable-vhdx
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/Makefile.objs |   2 +-
> >  block/vhdx.c        | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  block/vhdx.h        |  12 +++-
> >  configure           |  13 +++++
> >  4 files changed, 180 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 4cf9aa4..e5e54e6 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -2,7 +2,7 @@ block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat
> >  block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
> >  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> >  block-obj-y += qed-check.o
> > -block-obj-y += vhdx.o
> > +block-obj-$(CONFIG_VHDX) += vhdx.o
> >  block-obj-y += parallels.o blkdebug.o blkverify.o
> >  block-obj-y += snapshot.o qapi.o
> >  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> > diff --git a/block/vhdx.c b/block/vhdx.c
> > index 56bc88e..9b50442 100644
> > --- a/block/vhdx.c
> > +++ b/block/vhdx.c
> > @@ -21,6 +21,7 @@
> >  #include "qemu/crc32c.h"
> >  #include "block/vhdx.h"
> >  
> > +#include <uuid/uuid.h>
> >  
> >  /* Several metadata and region table data entries are identified by
> >   * guids in  a MS-specific GUID format. */
> > @@ -156,11 +157,40 @@ typedef struct BDRVVHDXState {
> >      VHDXBatEntry *bat;
> >      uint64_t bat_offset;
> >  
> > +    MSGUID session_guid;
> > +
> > +
> >      VHDXParentLocatorHeader parent_header;
> >      VHDXParentLocatorEntry *parent_entries;
> >  
> >  } BDRVVHDXState;
> >  
> > +/* 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)
> > + *
> > + * Note: The resulting checksum is in the CPU endianness, not necessarily
> > + *       in the file format endianness (LE).  Any header export to disk should
> > + *       make sure that vhdx_header_le_export() is used to convert to the
> > + *       correct endianness
> > + */
> > +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));
> 
> sizeof(crc) for consistency.
>

OK

> > +
> > +    memset(buf + crc_offset, 0, sizeof(crc));
> > +    crc =  crc32c(0xffffffff, buf, size);
> > +    memcpy(buf + crc_offset, &crc, sizeof(crc));
> > +
> > +    return crc;
> > +}
> > +
> >  uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
> >                              int crc_offset)
> >  {
> > @@ -212,6 +242,24 @@ 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
> > + * pretty straightforward for the DCE + random usage case
> > + *
> > + */
> > +void vhdx_guid_generate(MSGUID *guid)
> > +{
> > +    uuid_t uuid;
> > +    assert(guid != NULL);
> > +
> > +    uuid_generate(uuid);
> > +    memcpy(guid, uuid, 16);
> > +}
> 
> Should MSGUID be QEMU_PACKED to make sure that this works? Also
> sizeof(guid) instead of 16?
>

Yikes, yes it definitely should be packed.

> > +
> > +/*
> >   * Per the MS VHDX Specification, for every VHDX file:
> >   *      - The header section is fixed size - 1 MB
> >   *      - The header section is always the first "object"
> > @@ -249,6 +297,107 @@ static void vhdx_header_le_import(VHDXHeader *h)
> >      le64_to_cpus(&h->log_offset);
> >  }
> >  
> > +/* All VHDX structures on disk are little endian */
> > +static void vhdx_header_le_export(VHDXHeader *orig_h, VHDXHeader *new_h)
> > +{
> > +    assert(orig_h != NULL);
> > +    assert(new_h != NULL);
> > +
> > +    new_h->signature       = cpu_to_le32(orig_h->signature);
> > +    new_h->checksum        = cpu_to_le32(orig_h->checksum);
> > +    new_h->sequence_number = cpu_to_le64(orig_h->sequence_number);
> > +
> > +    memcpy(&new_h->file_write_guid, &orig_h->file_write_guid, sizeof(MSGUID));
> > +    memcpy(&new_h->data_write_guid, &orig_h->data_write_guid, sizeof(MSGUID));
> > +    memcpy(&new_h->log_guid,        &orig_h->log_guid,        sizeof(MSGUID));
> 
> Can we avoid memcpy? I think this should work:
> 
> new_h->file_write_guid = orig_h->file_write_guid;
> new_h->data_write_guid = orig_h->data_write_guid;
> new_h->log_guid        = orig_h->log_guid;
> 

Yes, that should work fine as well -  I'll change it to that.

> > +
> > +    cpu_to_leguids(&new_h->file_write_guid);
> > +    cpu_to_leguids(&new_h->data_write_guid);
> > +    cpu_to_leguids(&new_h->log_guid);
> > +
> > +    new_h->log_version     = cpu_to_le16(orig_h->log_version);
> > +    new_h->version         = cpu_to_le16(orig_h->version);
> > +    new_h->log_length      = cpu_to_le32(orig_h->log_length);
> > +    new_h->log_offset      = cpu_to_le64(orig_h->log_offset);
> > +}
> > +
> > +/* Update the VHDX headers
> > + *
> > + * This follows the VHDX spec procedures for header updates.
> > + *
> > + *  - non-current header is updated with largest sequence number
> > + */
> > +static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s, bool rw)
> > +{
> > +    int ret = 0;
> > +    int hdr_idx = 0;
> > +    uint64_t header_offset = VHDX_HEADER1_OFFSET;
> > +
> > +    VHDXHeader *active_header;
> > +    VHDXHeader *inactive_header;
> > +    VHDXHeader header_le;
> > +    uint8_t *buffer;
> > +
> > +    /* operate on the non-current header */
> > +    if (s->curr_header == 0) {
> > +        hdr_idx = 1;
> > +        header_offset = VHDX_HEADER2_OFFSET;
> > +    }
> > +
> > +    active_header   = s->headers[s->curr_header];
> > +    inactive_header = s->headers[hdr_idx];
> > +
> > +    inactive_header->sequence_number = active_header->sequence_number + 1;
> > +
> > +    /* a new file guid must be generate before any file write, including
> > +     * headers */
> > +    memcpy(&inactive_header->file_write_guid, &s->session_guid,
> > +           sizeof(MSGUID));
> 
> Here a simple assignment should do as well.
> 
> > +
> > +    /* a new data guid only needs to be generate before any guest-visible
> > +     * writes, so update it if the image is opened r/w. */
> > +    if (rw) {
> > +        vhdx_guid_generate(&inactive_header->data_write_guid);
> > +    }
> > +
> > +    /* the header checksum is not over just the packed size of VHDXHeader,
> > +     * but rather over the entire 'reserved' range for the header, which is
> > +     * 4KB (VHDX_HEADER_SIZE). */
> > +
> > +    buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE);
> > +    /* we can't assume the extra reserved bytes are 0 */
> > +    ret = bdrv_pread(bs->file, header_offset, buffer, VHDX_HEADER_SIZE);
> > +    if (ret < 0) {
> > +        goto exit;
> > +    }
> > +    /* overwrite the actual VHDXHeader portion */
> > +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> > +    inactive_header->checksum = vhdx_update_checksum(buffer,
> > +                                                     VHDX_HEADER_SIZE, 4);
> > +    vhdx_header_le_export(inactive_header, &header_le);
> > +    bdrv_pwrite_sync(bs->file, header_offset, &header_le, sizeof(VHDXHeader));
> > +    s->curr_header = hdr_idx;
> > +
> > +exit:
> > +    qemu_vfree(buffer);
> > +    return ret;
> > +}
> 
> Kevin
Kevin Wolf - Aug. 5, 2013, 3:05 p.m.
Am 01.08.2013 um 05:23 hat Jeff Cody geschrieben:
> This adds the ability to update the headers in a VHDX image, including
> generating a new MS-compatible GUID.
> 
> As VHDX depends on uuid.h, VHDX is now a configurable build option.  If
> VHDX support is enabled, that will also enable uuid as well.  The
> default is to have VHDX enabled.
> 
> To enable/disable VHDX:  --enable-vhdx, --disable-vhdx
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>

I knew I'd forget to mention something I had seen...

> +    /* overwrite the actual VHDXHeader portion */
> +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> +    inactive_header->checksum = vhdx_update_checksum(buffer,
> +                                                     VHDX_HEADER_SIZE, 4);

This I would prefer as offsetof(VHDXHeader, checksum) instead of a magic
number 4.

Kevin
Jeff Cody - Aug. 5, 2013, 3:09 p.m.
On Mon, Aug 05, 2013 at 05:05:36PM +0200, Kevin Wolf wrote:
> Am 01.08.2013 um 05:23 hat Jeff Cody geschrieben:
> > This adds the ability to update the headers in a VHDX image, including
> > generating a new MS-compatible GUID.
> > 
> > As VHDX depends on uuid.h, VHDX is now a configurable build option.  If
> > VHDX support is enabled, that will also enable uuid as well.  The
> > default is to have VHDX enabled.
> > 
> > To enable/disable VHDX:  --enable-vhdx, --disable-vhdx
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> 
> I knew I'd forget to mention something I had seen...
> 
> > +    /* overwrite the actual VHDXHeader portion */
> > +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> > +    inactive_header->checksum = vhdx_update_checksum(buffer,
> > +                                                     VHDX_HEADER_SIZE, 4);
> 
> This I would prefer as offsetof(VHDXHeader, checksum) instead of a magic
> number 4.

Who doesn't like a little bit of magic? :)  OK, I'll use offsetof()
(I'll also look through the rest of my patches for similar occurrences
as well).

Jeff
Kevin Wolf - Aug. 6, 2013, 8:34 a.m.
Am 05.08.2013 um 17:09 hat Jeff Cody geschrieben:
> On Mon, Aug 05, 2013 at 05:05:36PM +0200, Kevin Wolf wrote:
> > Am 01.08.2013 um 05:23 hat Jeff Cody geschrieben:
> > > This adds the ability to update the headers in a VHDX image, including
> > > generating a new MS-compatible GUID.
> > > 
> > > As VHDX depends on uuid.h, VHDX is now a configurable build option.  If
> > > VHDX support is enabled, that will also enable uuid as well.  The
> > > default is to have VHDX enabled.
> > > 
> > > To enable/disable VHDX:  --enable-vhdx, --disable-vhdx
> > > 
> > > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > 
> > I knew I'd forget to mention something I had seen...
> > 
> > > +    /* overwrite the actual VHDXHeader portion */
> > > +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> > > +    inactive_header->checksum = vhdx_update_checksum(buffer,
> > > +                                                     VHDX_HEADER_SIZE, 4);
> > 
> > This I would prefer as offsetof(VHDXHeader, checksum) instead of a magic
> > number 4.
> 
> Who doesn't like a little bit of magic? :)

I think I would have accepted a properly implemented qemu_rand(). :-)

Kevin
Jeff Cody - Aug. 7, 2013, 2:42 p.m.
On Thu, Aug 01, 2013 at 03:44:41PM +0200, Stefan Hajnoczi wrote:
> On Wed, Jul 31, 2013 at 11:23:47PM -0400, Jeff Cody wrote:
> > @@ -212,6 +242,24 @@ 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
> > + * pretty straightforward for the DCE + random usage case
> 
> The talk of uuid.h not being available confuses me.  The code has no
> #ifdef and we do not define uuid_generate() ourselves.
> 
> Is this an outdated comment?
> 
> > +/* Update the VHDX headers
> > + *
> > + * This follows the VHDX spec procedures for header updates.
> > + *
> > + *  - non-current header is updated with largest sequence number
> > + */
> > +static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s, bool rw)
> 
> "rw" should be named "generate_file_write_guid".  If you call
> vhdx_update_header() again later you do not need to update FileWriteGuid
> again according to the spec.  There's probably no harm in doing so but
> the spec explicitly says "If this is the first header update within the
> session, use a new value for the FileWriteGuid".
> 
> This means that future calls to vhdx_update_header() in the same session
> should set "generate_file_write_guid" to false.  Therefore "rw" is not
> the right name.
> 

Reading through your comments again made me realize there is a
misunderstanding (a good sign in itself that 'rw' should be renamed).
The 'rw' is not to generate the FileWriteGuid, but rather the
DataWriteGuid.  FileWriteGuid is copied from our session_guid, so it
is only generated once and used through the entire open session.  The
s->session_guid field is generated upon vhdx_open(), for usage if the
file is opened BDRV_O_RDWR.

The DataWriteGuid is generated before any user-visible writes
(anything observable via a virtual disk read).  And that DataWriteGuid
is what the 'rw' field is used for, and the function
vhdx_user_visible_write() handles calling vhdx_update_headers() with
that field set to true.

I'll rename 'rw' to 'generate_data_write_guid'.


> > +{
> > +    int ret = 0;
> > +    int hdr_idx = 0;
> > +    uint64_t header_offset = VHDX_HEADER1_OFFSET;
> > +
> > +    VHDXHeader *active_header;
> > +    VHDXHeader *inactive_header;
> > +    VHDXHeader header_le;
> > +    uint8_t *buffer;
> > +
> > +    /* operate on the non-current header */
> > +    if (s->curr_header == 0) {
> > +        hdr_idx = 1;
> > +        header_offset = VHDX_HEADER2_OFFSET;
> > +    }
> > +
> > +    active_header   = s->headers[s->curr_header];
> > +    inactive_header = s->headers[hdr_idx];
> > +
> > +    inactive_header->sequence_number = active_header->sequence_number + 1;
> > +
> > +    /* a new file guid must be generate before any file write, including
> 
> s/generate/generated/
> 
> > +     * headers */
> > +    memcpy(&inactive_header->file_write_guid, &s->session_guid,
> > +           sizeof(MSGUID));
> > +
> > +    /* a new data guid only needs to be generate before any guest-visible
> > +     * writes, so update it if the image is opened r/w. */
> > +    if (rw) {
> > +        vhdx_guid_generate(&inactive_header->data_write_guid);
> > +    }
> > +
> > +    /* the header checksum is not over just the packed size of VHDXHeader,
> > +     * but rather over the entire 'reserved' range for the header, which is
> > +     * 4KB (VHDX_HEADER_SIZE). */
> > +
> > +    buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE);
> > +    /* we can't assume the extra reserved bytes are 0 */
> > +    ret = bdrv_pread(bs->file, header_offset, buffer, VHDX_HEADER_SIZE);
> > +    if (ret < 0) {
> > +        goto exit;
> > +    }
> > +    /* overwrite the actual VHDXHeader portion */
> > +    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
> > +    inactive_header->checksum = vhdx_update_checksum(buffer,
> > +                                                     VHDX_HEADER_SIZE, 4);
> > +    vhdx_header_le_export(inactive_header, &header_le);
> > +    bdrv_pwrite_sync(bs->file, header_offset, &header_le, sizeof(VHDXHeader));
> 
> This function can fail and it's a good indicator that future I/Os will
> also be in trouble.  Please propagate the error return.
> 
> > @@ -801,8 +955,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
> >      }
> >  
> >      if (flags & BDRV_O_RDWR) {
> > -        ret = -ENOTSUP;
> > -        goto fail;
> > +        vhdx_update_headers(bs, s, false);
> 
> Error handling is missing.  At this point it looks like we cannot write
> to the file.  Propagating the error seems reasonable.

Patch

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 4cf9aa4..e5e54e6 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -2,7 +2,7 @@  block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat
 block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
-block-obj-y += vhdx.o
+block-obj-$(CONFIG_VHDX) += vhdx.o
 block-obj-y += parallels.o blkdebug.o blkverify.o
 block-obj-y += snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
diff --git a/block/vhdx.c b/block/vhdx.c
index 56bc88e..9b50442 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -21,6 +21,7 @@ 
 #include "qemu/crc32c.h"
 #include "block/vhdx.h"
 
+#include <uuid/uuid.h>
 
 /* Several metadata and region table data entries are identified by
  * guids in  a MS-specific GUID format. */
@@ -156,11 +157,40 @@  typedef struct BDRVVHDXState {
     VHDXBatEntry *bat;
     uint64_t bat_offset;
 
+    MSGUID session_guid;
+
+
     VHDXParentLocatorHeader parent_header;
     VHDXParentLocatorEntry *parent_entries;
 
 } BDRVVHDXState;
 
+/* 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)
+ *
+ * Note: The resulting checksum is in the CPU endianness, not necessarily
+ *       in the file format endianness (LE).  Any header export to disk should
+ *       make sure that vhdx_header_le_export() is used to convert to the
+ *       correct endianness
+ */
+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));
+
+    return crc;
+}
+
 uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
                             int crc_offset)
 {
@@ -212,6 +242,24 @@  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
+ * pretty straightforward for the DCE + random usage case
+ *
+ */
+void vhdx_guid_generate(MSGUID *guid)
+{
+    uuid_t uuid;
+    assert(guid != NULL);
+
+    uuid_generate(uuid);
+    memcpy(guid, uuid, 16);
+}
+
+/*
  * Per the MS VHDX Specification, for every VHDX file:
  *      - The header section is fixed size - 1 MB
  *      - The header section is always the first "object"
@@ -249,6 +297,107 @@  static void vhdx_header_le_import(VHDXHeader *h)
     le64_to_cpus(&h->log_offset);
 }
 
+/* All VHDX structures on disk are little endian */
+static void vhdx_header_le_export(VHDXHeader *orig_h, VHDXHeader *new_h)
+{
+    assert(orig_h != NULL);
+    assert(new_h != NULL);
+
+    new_h->signature       = cpu_to_le32(orig_h->signature);
+    new_h->checksum        = cpu_to_le32(orig_h->checksum);
+    new_h->sequence_number = cpu_to_le64(orig_h->sequence_number);
+
+    memcpy(&new_h->file_write_guid, &orig_h->file_write_guid, sizeof(MSGUID));
+    memcpy(&new_h->data_write_guid, &orig_h->data_write_guid, sizeof(MSGUID));
+    memcpy(&new_h->log_guid,        &orig_h->log_guid,        sizeof(MSGUID));
+
+    cpu_to_leguids(&new_h->file_write_guid);
+    cpu_to_leguids(&new_h->data_write_guid);
+    cpu_to_leguids(&new_h->log_guid);
+
+    new_h->log_version     = cpu_to_le16(orig_h->log_version);
+    new_h->version         = cpu_to_le16(orig_h->version);
+    new_h->log_length      = cpu_to_le32(orig_h->log_length);
+    new_h->log_offset      = cpu_to_le64(orig_h->log_offset);
+}
+
+/* Update the VHDX headers
+ *
+ * This follows the VHDX spec procedures for header updates.
+ *
+ *  - non-current header is updated with largest sequence number
+ */
+static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s, bool rw)
+{
+    int ret = 0;
+    int hdr_idx = 0;
+    uint64_t header_offset = VHDX_HEADER1_OFFSET;
+
+    VHDXHeader *active_header;
+    VHDXHeader *inactive_header;
+    VHDXHeader header_le;
+    uint8_t *buffer;
+
+    /* operate on the non-current header */
+    if (s->curr_header == 0) {
+        hdr_idx = 1;
+        header_offset = VHDX_HEADER2_OFFSET;
+    }
+
+    active_header   = s->headers[s->curr_header];
+    inactive_header = s->headers[hdr_idx];
+
+    inactive_header->sequence_number = active_header->sequence_number + 1;
+
+    /* a new file guid must be generate before any file write, including
+     * headers */
+    memcpy(&inactive_header->file_write_guid, &s->session_guid,
+           sizeof(MSGUID));
+
+    /* a new data guid only needs to be generate before any guest-visible
+     * writes, so update it if the image is opened r/w. */
+    if (rw) {
+        vhdx_guid_generate(&inactive_header->data_write_guid);
+    }
+
+    /* the header checksum is not over just the packed size of VHDXHeader,
+     * but rather over the entire 'reserved' range for the header, which is
+     * 4KB (VHDX_HEADER_SIZE). */
+
+    buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE);
+    /* we can't assume the extra reserved bytes are 0 */
+    ret = bdrv_pread(bs->file, header_offset, buffer, VHDX_HEADER_SIZE);
+    if (ret < 0) {
+        goto exit;
+    }
+    /* overwrite the actual VHDXHeader portion */
+    memcpy(buffer, inactive_header, sizeof(VHDXHeader));
+    inactive_header->checksum = vhdx_update_checksum(buffer,
+                                                     VHDX_HEADER_SIZE, 4);
+    vhdx_header_le_export(inactive_header, &header_le);
+    bdrv_pwrite_sync(bs->file, header_offset, &header_le, sizeof(VHDXHeader));
+    s->curr_header = hdr_idx;
+
+exit:
+    qemu_vfree(buffer);
+    return ret;
+}
+
+/*
+ * The VHDX spec calls for header updates to be performed twice, so that both
+ * the current and non-current header have valid info
+ */
+static int vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s, bool rw)
+{
+    int ret;
+
+    ret = vhdx_update_header(bs, s, rw);
+    if (ret < 0) {
+        return ret;
+    }
+    ret = vhdx_update_header(bs, s, rw);
+    return ret;
+}
 
 /* opens the specified header block from the VHDX file header section */
 static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
@@ -739,6 +888,11 @@  static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
         goto fail;
     }
 
+    /* This is used for any header updates, for the file_write_guid.
+     * The spec dictates that a new value should be used for the first
+     * header update */
+    vhdx_guid_generate(&s->session_guid);
+
     ret = vhdx_parse_header(bs, s);
     if (ret) {
         goto fail;
@@ -801,8 +955,7 @@  static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
     }
 
     if (flags & BDRV_O_RDWR) {
-        ret = -ENOTSUP;
-        goto fail;
+        vhdx_update_headers(bs, s, false);
     }
 
     /* TODO: differencing files, write */
diff --git a/block/vhdx.h b/block/vhdx.h
index 1dbb320..3999cb1 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -309,17 +309,27 @@  typedef struct QEMU_PACKED VHDXParentLocatorEntry {
 /* ----- END VHDX SPECIFICATION STRUCTURES ---- */
 
 
+void vhdx_guid_generate(MSGUID *guid);
+
+uint32_t vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset);
 uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
                             int crc_offset);
 
 bool vhdx_checksum_is_valid(uint8_t *buf, size_t size, int crc_offset);
 
 
-static void leguid_to_cpus(MSGUID *guid)
+static inline void leguid_to_cpus(MSGUID *guid)
 {
     le32_to_cpus(&guid->data1);
     le16_to_cpus(&guid->data2);
     le16_to_cpus(&guid->data3);
 }
 
+static inline void cpu_to_leguids(MSGUID *guid)
+{
+    cpu_to_le32s(&guid->data1);
+    cpu_to_le16s(&guid->data2);
+    cpu_to_le16s(&guid->data3);
+}
+
 #endif
diff --git a/configure b/configure
index 877a821..821b790 100755
--- a/configure
+++ b/configure
@@ -244,6 +244,7 @@  gtk=""
 gtkabi="2.0"
 tpm="no"
 libssh2=""
+vhdx="yes"
 
 # parse CC options first
 for opt do
@@ -950,6 +951,11 @@  for opt do
   ;;
   --enable-libssh2) libssh2="yes"
   ;;
+  --enable-vhdx) vhdx="yes" ;
+                 uuid="yes"
+  ;;
+  --disable-vhdx) vhdx="no"
+  ;;
   *) echo "ERROR: unknown option $opt"; show_help="yes"
   ;;
   esac
@@ -1166,6 +1172,8 @@  echo "  --gcov=GCOV              use specified gcov [$gcov_tool]"
 echo "  --enable-tpm             enable TPM support"
 echo "  --disable-libssh2        disable ssh block device support"
 echo "  --enable-libssh2         enable ssh block device support"
+echo "  --disable-vhdx           disables support for the Microsoft VHDX image format"
+echo "  --enable-vhdx            enable support for the Microsoft VHDX image format"
 echo ""
 echo "NOTE: The object files are built at the place where configure is launched"
 exit 1
@@ -3622,6 +3630,7 @@  echo "TPM support       $tpm"
 echo "libssh2 support   $libssh2"
 echo "TPM passthrough   $tpm_passthrough"
 echo "QOM debugging     $qom_cast_debug"
+echo "vhdx              $vhdx"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -4012,6 +4021,10 @@  if test "$virtio_blk_data_plane" = "yes" ; then
   echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak
 fi
 
+if test "$vhdx" = "yes" ; then
+  echo "CONFIG_VHDX=y" >> $config_host_mak
+fi
+
 # USB host support
 case "$usb" in
 linux)