Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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.
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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 jc...@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
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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 jc...@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
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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 jc...@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(0x, 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);
+
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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 jc...@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
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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 jc...@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(0x, 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.
+
+
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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 jc...@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
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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 jc...@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
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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.
Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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.
[Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability.
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 jc...@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));
+
+memset(buf + crc_offset, 0, sizeof(crc));
+crc = crc32c(0x, 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
