| Message ID | 595d71cf1cd25ac9deff521bceb82b8c3da888d0.1375325971.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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
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
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
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
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
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
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.
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)
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(-)