[Qemu-devel] [PATCH 0/2] Small improvements to VMDK4 file handling

[Qemu-devel] [PATCH 0/2] Small improvements to VMDK4 file handling

[Richard W.M. Jones]

We're tracing a bug in "qemu-img convert" where it will silently fail
to convert certain types of VMDK file.  The VMDK files produced by
VMWare vSphere / ESX 4.0, which have the VMDK subformat
"streamOptimized" are one notable case:

  https://bugzilla.redhat.com/show_bug.cgi?id=548723

These two patches certainly don't fix this, but they do at least
prevent "qemu-img convert" from silently failing.

Unfortunately to do this I had to look much more deeply into the VMDK
format than the current VMDK block driver does.  We have to parse the
VMDK4 plain text descriptor which is embedded in these binary files.

So the first patch adds the parsing and has the benefit of improving
how the CID and parentCID fields are read and updated.

The second patch parses the createType field from the descriptor, and
refuses to continue if it's not on a short whitelist of subformats
which we are likely to be able to parse.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

[Qemu-devel] [PATCH 1/2] VMDK4: Parse the VMDK descriptor explicitly, improve handling of CIDs

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 260 bytes --]


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

[-- Attachment #2: 0001-VMDK4-Parse-the-VMDK-descriptor-explicitly-improve-h.patch --]
[-- Type: text/plain, Size: 8240 bytes --]

>From 69701355561c328fac5e7986d6122c8411137b7e Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones@redhat.com>
Date: Fri, 18 Dec 2009 15:30:03 +0000
Subject: [PATCH 1/2] VMDK4: Parse the VMDK descriptor explicitly, improve handling of CIDs

The VMDK monolithic file format[1] contains an embedded text
descriptor.  This is at a variable location in the file, pointed
to by fields in the header.  An example of this embedded descriptor
might be:

  # Disk DescriptorFile
  version=1
  CID=12345678
  parentCID=87654321
  createType="monolithicSparse"

  # Extent description
  RW 16777216 SPARSE "generated-stream.vmdk"

The current code assumes the descriptor is at offset 0x200 with a
fixed size, and doesn't really parse the descriptor properly eg
when reading and writing the CID and parentCID fields.

This patch contains code to properly locate and parse the embedded
descriptor in monolithic files (qemu doesn't support "split" VMDK
files at all).

Also this modifies the code which updates the CID fields so it
doesn't rewrite the whole of the descriptor.

[1] The VMDK 1.1 (ie. "VMDK4") format is partially described here:
http://www.vmware.com/app/vmdk/?src=vmdk [PDF link]
---
 block/vmdk.c |  173 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 142 insertions(+), 31 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 4e48622..71680f0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -77,6 +77,10 @@ typedef struct BDRVVmdkState {
     unsigned int cluster_sectors;
     uint32_t parent_cid;
     int is_parent;
+
+    /* VMDK4 fields */
+    int64_t cid_offset;
+    int64_t parent_cid_offset;
 } BDRVVmdkState;
 
 typedef struct VmdkMetaData {
@@ -112,57 +116,159 @@ static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
 #define CHECK_CID 1
 
 #define SECTOR_SIZE 512
-#define DESC_SIZE 20*SECTOR_SIZE	// 20 sectors of 512 bytes each
 #define HEADER_SIZE 512   			// first sector of 512 bytes
 
+/* Read and parse the VMDK4 descriptor.
+ *
+ * So-called "monolithic" VMDK4 files (which are the only ones we can
+ * currently read) have a text descriptor section embedded within the
+ * file at an offset described in the header.
+ *
+ * The other format for VMDK4 files ("split") has a toplevel .vmdk
+ * file which is the plain text descriptor, and that points to other
+ * files that contain the disk data.  We can't read this sort of file
+ * yet.
+ */
+#define MAX_DESC_SIZE         100*512
+
+#define VMDK4_ACCESS_RW       1
+#define VMDK4_ACCESS_RDONLY   2
+#define VMDK4_ACCESS_NOACCESS 3
+
+static int vmdk4_parse_desc_extent(BlockDriverState *bs, VMDK4Header *header,
+                                   int access_type, char *p)
+{
+    /* We should probably check that the monolithic file
+     * has only one extent.
+     */
+    return 0;
+}
+
+static int vmdk4_parse_desc_assignment(BlockDriverState *bs,
+                                       VMDK4Header *header, char *p, size_t len,
+                                       int64_t file_offset)
+{
+    BDRVVmdkState *s = bs->opaque;
+
+    if (strncmp(p, "version=", 8) == 0 && strcmp(p, "version=1") != 0)
+        fprintf (stderr,
+                 "warning: vmdk4_parse_desc: descriptor version != 1 (%s)\n",
+                 p);
+
+    if (!strncmp(p, "CID=", 4))
+        s->cid_offset = file_offset + 4;
+
+    if (!strncmp(p, "parentCID=", 10))
+        s->parent_cid_offset = file_offset + 10;
+
+    return 0;
+}
+
+static int vmdk4_parse_desc(BlockDriverState *bs, VMDK4Header *header)
+{
+    BDRVVmdkState *s = bs->opaque;
+    int64_t desc_offset, desc_size;
+    char desc[MAX_DESC_SIZE];
+    char *p, *pnext;
+    size_t len;
+
+    s->cid_offset = 0;
+    s->parent_cid_offset = 0;
+
+    /* Stored in the header are the offset/size in sectors. */
+    desc_size = le64_to_cpu(header->desc_size) * SECTOR_SIZE;
+    desc_offset = le64_to_cpu(header->desc_offset) * SECTOR_SIZE;
+
+    if (desc_size > MAX_DESC_SIZE) {
+        fprintf (stderr,
+                 "vmdk4_parse_desc: descriptor size > maximum size "
+                 "(%" PRIi64 " > %d)\n", desc_size, MAX_DESC_SIZE);
+        return -1;
+    }
+
+    if (bdrv_pread(s->hd, desc_offset, (uint8_t *)desc, desc_size) != desc_size) {
+        fprintf (stderr,
+                 "vmdk4_parse_desc: could not read descriptor from "
+                 "%" PRIi64 " size %" PRIi64 "\n",
+                 desc_offset, desc_size);
+        return -1;
+    }
+
+    if (!strncmp(desc, "# Disk DescriptorFile", 22)) {
+        fprintf (stderr, "vmdk4_parse_desc: unrecognized descriptor");
+        return -1;
+    }
+
+    /* Parse the descriptor line by line. */
+    for (p = desc; p && *p; p = pnext) {
+        pnext = strchr (p, '\n');
+        if (pnext != NULL) {
+            *pnext = '\0';
+            pnext++;
+        }
+
+        while (*p && isspace (*p))
+            p++;
+        while ((len = strlen (p)) > 0 && isspace (p[len-1]))
+            p[len-1] = '\0';
+
+        if (!*p)
+            continue;
+        if (p[0] == '#')
+            continue;
+
+        /* Ignore any lines that we can't parse. */
+        if (!strncmp(p, "RW ", 3))
+            vmdk4_parse_desc_extent (bs, header, VMDK4_ACCESS_RW, p + 3);
+        else if (!strncmp(p, "RDONLY ", 7))
+            vmdk4_parse_desc_extent (bs, header, VMDK4_ACCESS_RDONLY, p + 7);
+        else if (!strncmp(p, "NOACCESS ", 9))
+            vmdk4_parse_desc_extent (bs, header, VMDK4_ACCESS_NOACCESS, p + 9);
+        else                    /* else expecting name = value */
+            vmdk4_parse_desc_assignment (bs, header, p, len,
+                                         p-desc+desc_offset);
+    }
+
+    return 0;
+}
+
 static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
 {
     BDRVVmdkState *s = bs->opaque;
-    char desc[DESC_SIZE];
+    int64_t offset;
+    char str[9];
     uint32_t cid;
-    const char *p_name, *cid_str;
-    size_t cid_str_size;
 
-    /* the descriptor offset = 0x200 */
-    if (bdrv_pread(s->hd, 0x200, desc, DESC_SIZE) != DESC_SIZE)
-        return 0;
+    if (parent)
+        offset = s->parent_cid_offset;
+    else
+        offset = s->cid_offset;
 
-    if (parent) {
-        cid_str = "parentCID";
-        cid_str_size = sizeof("parentCID");
-    } else {
-        cid_str = "CID";
-        cid_str_size = sizeof("CID");
-    }
+    if (offset == 0)
+        return 0;
 
-    if ((p_name = strstr(desc,cid_str)) != NULL) {
-        p_name += cid_str_size;
-        sscanf(p_name,"%x",&cid);
-    }
+    if (bdrv_pread(s->hd, offset, str, 8) != 8)
+        return 0;
 
+    sscanf(str, "%x", &cid);
     return cid;
 }
 
 static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
 {
     BDRVVmdkState *s = bs->opaque;
-    char desc[DESC_SIZE], tmp_desc[DESC_SIZE];
-    char *p_name, *tmp_str;
+    int64_t offset;
+    char str[9];
 
-    /* the descriptor offset = 0x200 */
-    if (bdrv_pread(s->hd, 0x200, desc, DESC_SIZE) != DESC_SIZE)
-        return -1;
+    offset = s->cid_offset;
+    if (offset == 0)
+        return 0;
 
-    tmp_str = strstr(desc,"parentCID");
-    pstrcpy(tmp_desc, sizeof(tmp_desc), tmp_str);
-    if ((p_name = strstr(desc,"CID")) != NULL) {
-        p_name += sizeof("CID");
-        snprintf(p_name, sizeof(desc) - (p_name - desc), "%x\n", cid);
-        pstrcat(desc, sizeof(desc), tmp_desc);
-    }
+    snprintf(str, sizeof str, "%x\n", cid);
 
-    if (bdrv_pwrite(s->hd, 0x200, desc, DESC_SIZE) != DESC_SIZE)
+    if (bdrv_pwrite(s->hd, offset, str, 8) != 8)
         return -1;
+
     return 0;
 }
 
@@ -184,6 +290,8 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
     return 1;
 }
 
+#define DESC_SIZE 20*SECTOR_SIZE
+
 static int vmdk_snapshot_create(const char *filename, const char *backing_file)
 {
     int snp_fd, p_fd;
@@ -410,6 +518,9 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
         s->l1_table_offset = le64_to_cpu(header.rgd_offset) << 9;
         s->l1_backup_table_offset = le64_to_cpu(header.gd_offset) << 9;
 
+        if (vmdk4_parse_desc(bs, &header) == -1)
+            goto fail;
+
         if (parent_open)
             s->is_parent = 1;
         else
-- 
1.6.5.2

[Qemu-devel] [PATCH 2/2] VMDK4: Parse and check the createType (VMDK file type) field.

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 260 bytes --]


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

[-- Attachment #2: 0002-VMDK4-Parse-and-check-the-createType-VMDK-file-type-.patch --]
[-- Type: text/plain, Size: 2864 bytes --]

>From bc09a7473b0382fbac7c3702b668acc1b04f1e9c Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones@redhat.com>
Date: Fri, 18 Dec 2009 15:36:17 +0000
Subject: [PATCH 2/2] VMDK4: Parse and check the createType (VMDK file type) field.

Currently qemu fails to read VMDK subformats such as streamOptimized.
It silently corrupts these files when it reads them.

>From the embedded descriptor, parse the createType field and check it
against a short list of subformats which are likely to work.
---
 block/vmdk.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 71680f0..c0b3160 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -81,6 +81,7 @@ typedef struct BDRVVmdkState {
     /* VMDK4 fields */
     int64_t cid_offset;
     int64_t parent_cid_offset;
+    char *create_type;
 } BDRVVmdkState;
 
 typedef struct VmdkMetaData {
@@ -161,6 +162,13 @@ static int vmdk4_parse_desc_assignment(BlockDriverState *bs,
     if (!strncmp(p, "parentCID=", 10))
         s->parent_cid_offset = file_offset + 10;
 
+    if (strncmp(p, "createType=\"", 12) == 0) {
+        p[len-1] = '\0';        /* Remove trailing double quote. */
+        if (s->create_type)
+            qemu_free(s->create_type);
+        s->create_type = qemu_strdup(p+12);
+    }
+
     return 0;
 }
 
@@ -174,6 +182,7 @@ static int vmdk4_parse_desc(BlockDriverState *bs, VMDK4Header *header)
 
     s->cid_offset = 0;
     s->parent_cid_offset = 0;
+    s->create_type = NULL;
 
     /* Stored in the header are the offset/size in sectors. */
     desc_size = le64_to_cpu(header->desc_size) * SECTOR_SIZE;
@@ -521,6 +530,16 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
         if (vmdk4_parse_desc(bs, &header) == -1)
             goto fail;
 
+        if (s->create_type &&
+            !(!strcmp(s->create_type, "monolithicSparse") ||
+              !strcmp(s->create_type, "monolithicFlat") ||
+              !strcmp(s->create_type, "twoGbMaxExtentSparse") ||
+              !strcmp(s->create_type, "twoGbMaxExtentFlat"))) {
+            fprintf (stderr, "vmdk: disk type '%s' is not supported by qemu\n",
+                     s->create_type);
+            goto fail;
+        }
+
         if (parent_open)
             s->is_parent = 1;
         else
@@ -559,6 +578,7 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
     qemu_free(s->l1_backup_table);
     qemu_free(s->l1_table);
     qemu_free(s->l2_cache);
+    qemu_free(s->create_type);
     bdrv_delete(s->hd);
     return -1;
 }
@@ -927,6 +947,7 @@ static void vmdk_close(BlockDriverState *bs)
 
     qemu_free(s->l1_table);
     qemu_free(s->l2_cache);
+    qemu_free(s->create_type);
     // try to close parent image, if exist
     vmdk_parent_close(s->hd);
     bdrv_delete(s->hd);
-- 
1.6.5.2

[Qemu-devel] [PATCH 1/2] VMDK4: Parse the VMDK descriptor explicitly, improve handling of CIDs

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 324 bytes --]


Let's fix a buffer overflow, and add the Signed-off-by line ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

[-- Attachment #2: 0001-VMDK4-Parse-the-VMDK-descriptor-explicitly-improve-h.patch --]
[-- Type: text/plain, Size: 8310 bytes --]

>From b22afc3464ec124d131ab21fc4270ef575c7b47d Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones@redhat.com>
Date: Fri, 18 Dec 2009 15:30:03 +0000
Subject: [PATCH 1/2] VMDK4: Parse the VMDK descriptor explicitly, improve handling of CIDs

The VMDK monolithic file format[1] contains an embedded text
descriptor.  This is at a variable location in the file, pointed
to by fields in the header.  An example of this embedded descriptor
might be:

  # Disk DescriptorFile
  version=1
  CID=12345678
  parentCID=87654321
  createType="monolithicSparse"

  # Extent description
  RW 16777216 SPARSE "generated-stream.vmdk"

The current code assumes the descriptor is at offset 0x200 with a
fixed size, and doesn't really parse the descriptor properly eg
when reading and writing the CID and parentCID fields.

This patch contains code to properly locate and parse the embedded
descriptor in monolithic files (qemu doesn't support "split" VMDK
files at all).

Also this modifies the code which updates the CID fields so it
doesn't rewrite the whole of the descriptor.

[1] The VMDK 1.1 (ie. "VMDK4") format is partially described here:
http://www.vmware.com/app/vmdk/?src=vmdk [PDF link]

Signed-off-by: Richard Jones <rjones@redhat.com>
---
 block/vmdk.c |  174 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 143 insertions(+), 31 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 4e48622..0f59ea5 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -77,6 +77,10 @@ typedef struct BDRVVmdkState {
     unsigned int cluster_sectors;
     uint32_t parent_cid;
     int is_parent;
+
+    /* VMDK4 fields */
+    int64_t cid_offset;
+    int64_t parent_cid_offset;
 } BDRVVmdkState;
 
 typedef struct VmdkMetaData {
@@ -112,57 +116,160 @@ static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
 #define CHECK_CID 1
 
 #define SECTOR_SIZE 512
-#define DESC_SIZE 20*SECTOR_SIZE	// 20 sectors of 512 bytes each
 #define HEADER_SIZE 512   			// first sector of 512 bytes
 
+/* Read and parse the VMDK4 descriptor.
+ *
+ * So-called "monolithic" VMDK4 files (which are the only ones we can
+ * currently read) have a text descriptor section embedded within the
+ * file at an offset described in the header.
+ *
+ * The other format for VMDK4 files ("split") has a toplevel .vmdk
+ * file which is the plain text descriptor, and that points to other
+ * files that contain the disk data.  We can't read this sort of file
+ * yet.
+ */
+#define MAX_DESC_SIZE         100*512
+
+#define VMDK4_ACCESS_RW       1
+#define VMDK4_ACCESS_RDONLY   2
+#define VMDK4_ACCESS_NOACCESS 3
+
+static int vmdk4_parse_desc_extent(BlockDriverState *bs, VMDK4Header *header,
+                                   int access_type, char *p)
+{
+    /* We should probably check that the monolithic file
+     * has only one extent.
+     */
+    return 0;
+}
+
+static int vmdk4_parse_desc_assignment(BlockDriverState *bs,
+                                       VMDK4Header *header, char *p, size_t len,
+                                       int64_t file_offset)
+{
+    BDRVVmdkState *s = bs->opaque;
+
+    if (strncmp(p, "version=", 8) == 0 && strcmp(p, "version=1") != 0)
+        fprintf (stderr,
+                 "warning: vmdk4_parse_desc: descriptor version != 1 (%s)\n",
+                 p);
+
+    if (!strncmp(p, "CID=", 4))
+        s->cid_offset = file_offset + 4;
+
+    if (!strncmp(p, "parentCID=", 10))
+        s->parent_cid_offset = file_offset + 10;
+
+    return 0;
+}
+
+static int vmdk4_parse_desc(BlockDriverState *bs, VMDK4Header *header)
+{
+    BDRVVmdkState *s = bs->opaque;
+    int64_t desc_offset, desc_size;
+    char desc[MAX_DESC_SIZE];
+    char *p, *pnext;
+    size_t len;
+
+    s->cid_offset = 0;
+    s->parent_cid_offset = 0;
+
+    /* Stored in the header are the offset/size in sectors. */
+    desc_size = le64_to_cpu(header->desc_size) * SECTOR_SIZE;
+    desc_offset = le64_to_cpu(header->desc_offset) * SECTOR_SIZE;
+
+    if (desc_size > MAX_DESC_SIZE) {
+        fprintf (stderr,
+                 "vmdk4_parse_desc: descriptor size > maximum size "
+                 "(%" PRIi64 " > %d)\n", desc_size, MAX_DESC_SIZE);
+        return -1;
+    }
+
+    if (bdrv_pread(s->hd, desc_offset, (uint8_t *)desc, desc_size) != desc_size) {
+        fprintf (stderr,
+                 "vmdk4_parse_desc: could not read descriptor from "
+                 "%" PRIi64 " size %" PRIi64 "\n",
+                 desc_offset, desc_size);
+        return -1;
+    }
+
+    if (!strncmp(desc, "# Disk DescriptorFile", 22)) {
+        fprintf (stderr, "vmdk4_parse_desc: unrecognized descriptor");
+        return -1;
+    }
+
+    /* Parse the descriptor line by line. */
+    for (p = desc; p && *p; p = pnext) {
+        pnext = strchr (p, '\n');
+        if (pnext != NULL) {
+            *pnext = '\0';
+            pnext++;
+        }
+
+        while (*p && isspace (*p))
+            p++;
+        while ((len = strlen (p)) > 0 && isspace (p[len-1]))
+            p[len-1] = '\0';
+
+        if (!*p)
+            continue;
+        if (p[0] == '#')
+            continue;
+
+        /* Ignore any lines that we can't parse. */
+        if (!strncmp(p, "RW ", 3))
+            vmdk4_parse_desc_extent (bs, header, VMDK4_ACCESS_RW, p + 3);
+        else if (!strncmp(p, "RDONLY ", 7))
+            vmdk4_parse_desc_extent (bs, header, VMDK4_ACCESS_RDONLY, p + 7);
+        else if (!strncmp(p, "NOACCESS ", 9))
+            vmdk4_parse_desc_extent (bs, header, VMDK4_ACCESS_NOACCESS, p + 9);
+        else                    /* else expecting name = value */
+            vmdk4_parse_desc_assignment (bs, header, p, len,
+                                         p-desc+desc_offset);
+    }
+
+    return 0;
+}
+
 static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
 {
     BDRVVmdkState *s = bs->opaque;
-    char desc[DESC_SIZE];
+    int64_t offset;
+    char str[9];
     uint32_t cid;
-    const char *p_name, *cid_str;
-    size_t cid_str_size;
 
-    /* the descriptor offset = 0x200 */
-    if (bdrv_pread(s->hd, 0x200, desc, DESC_SIZE) != DESC_SIZE)
-        return 0;
+    if (parent)
+        offset = s->parent_cid_offset;
+    else
+        offset = s->cid_offset;
 
-    if (parent) {
-        cid_str = "parentCID";
-        cid_str_size = sizeof("parentCID");
-    } else {
-        cid_str = "CID";
-        cid_str_size = sizeof("CID");
-    }
+    if (offset == 0)
+        return 0;
 
-    if ((p_name = strstr(desc,cid_str)) != NULL) {
-        p_name += cid_str_size;
-        sscanf(p_name,"%x",&cid);
-    }
+    if (bdrv_pread(s->hd, offset, str, 8) != 8)
+        return 0;
+    str[8] = '\0';
 
+    sscanf(str, "%x", &cid);
     return cid;
 }
 
 static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
 {
     BDRVVmdkState *s = bs->opaque;
-    char desc[DESC_SIZE], tmp_desc[DESC_SIZE];
-    char *p_name, *tmp_str;
+    int64_t offset;
+    char str[9];
 
-    /* the descriptor offset = 0x200 */
-    if (bdrv_pread(s->hd, 0x200, desc, DESC_SIZE) != DESC_SIZE)
-        return -1;
+    offset = s->cid_offset;
+    if (offset == 0)
+        return 0;
 
-    tmp_str = strstr(desc,"parentCID");
-    pstrcpy(tmp_desc, sizeof(tmp_desc), tmp_str);
-    if ((p_name = strstr(desc,"CID")) != NULL) {
-        p_name += sizeof("CID");
-        snprintf(p_name, sizeof(desc) - (p_name - desc), "%x\n", cid);
-        pstrcat(desc, sizeof(desc), tmp_desc);
-    }
+    snprintf(str, sizeof str, "%08x", cid);
 
-    if (bdrv_pwrite(s->hd, 0x200, desc, DESC_SIZE) != DESC_SIZE)
+    if (bdrv_pwrite(s->hd, offset, str, 8) != 8)
         return -1;
+
     return 0;
 }
 
@@ -184,6 +291,8 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
     return 1;
 }
 
+#define DESC_SIZE 20*SECTOR_SIZE
+
 static int vmdk_snapshot_create(const char *filename, const char *backing_file)
 {
     int snp_fd, p_fd;
@@ -410,6 +519,9 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
         s->l1_table_offset = le64_to_cpu(header.rgd_offset) << 9;
         s->l1_backup_table_offset = le64_to_cpu(header.gd_offset) << 9;
 
+        if (vmdk4_parse_desc(bs, &header) == -1)
+            goto fail;
+
         if (parent_open)
             s->is_parent = 1;
         else
-- 
1.6.5.2

[Qemu-devel] [PATCH 2/2] VMDK4: Parse and check the createType (VMDK file type) field.

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]


Add Signed-off-by line.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html

[-- Attachment #2: 0002-VMDK4-Parse-and-check-the-createType-VMDK-file-type-.patch --]
[-- Type: text/plain, Size: 2914 bytes --]

>From 1d2352173501bef4f95d4a3c17566934eb5abbe4 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones@redhat.com>
Date: Fri, 18 Dec 2009 15:36:17 +0000
Subject: [PATCH 2/2] VMDK4: Parse and check the createType (VMDK file type) field.

Currently qemu fails to read VMDK subformats such as streamOptimized.
It silently corrupts these files when it reads them.

>From the embedded descriptor, parse the createType field and check it
against a short list of subformats which are likely to work.

Signed-off-by: Richard Jones <rjones@redhat.com>
---
 block/vmdk.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 0f59ea5..4cb126b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -81,6 +81,7 @@ typedef struct BDRVVmdkState {
     /* VMDK4 fields */
     int64_t cid_offset;
     int64_t parent_cid_offset;
+    char *create_type;
 } BDRVVmdkState;
 
 typedef struct VmdkMetaData {
@@ -161,6 +162,13 @@ static int vmdk4_parse_desc_assignment(BlockDriverState *bs,
     if (!strncmp(p, "parentCID=", 10))
         s->parent_cid_offset = file_offset + 10;
 
+    if (strncmp(p, "createType=\"", 12) == 0) {
+        p[len-1] = '\0';        /* Remove trailing double quote. */
+        if (s->create_type)
+            qemu_free(s->create_type);
+        s->create_type = qemu_strdup(p+12);
+    }
+
     return 0;
 }
 
@@ -174,6 +182,7 @@ static int vmdk4_parse_desc(BlockDriverState *bs, VMDK4Header *header)
 
     s->cid_offset = 0;
     s->parent_cid_offset = 0;
+    s->create_type = NULL;
 
     /* Stored in the header are the offset/size in sectors. */
     desc_size = le64_to_cpu(header->desc_size) * SECTOR_SIZE;
@@ -522,6 +531,16 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
         if (vmdk4_parse_desc(bs, &header) == -1)
             goto fail;
 
+        if (s->create_type &&
+            !(!strcmp(s->create_type, "monolithicSparse") ||
+              !strcmp(s->create_type, "monolithicFlat") ||
+              !strcmp(s->create_type, "twoGbMaxExtentSparse") ||
+              !strcmp(s->create_type, "twoGbMaxExtentFlat"))) {
+            fprintf (stderr, "vmdk: disk type '%s' is not supported by qemu\n",
+                     s->create_type);
+            goto fail;
+        }
+
         if (parent_open)
             s->is_parent = 1;
         else
@@ -560,6 +579,7 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
     qemu_free(s->l1_backup_table);
     qemu_free(s->l1_table);
     qemu_free(s->l2_cache);
+    qemu_free(s->create_type);
     bdrv_delete(s->hd);
     return -1;
 }
@@ -928,6 +948,7 @@ static void vmdk_close(BlockDriverState *bs)
 
     qemu_free(s->l1_table);
     qemu_free(s->l2_cache);
+    qemu_free(s->create_type);
     // try to close parent image, if exist
     vmdk_parent_close(s->hd);
     bdrv_delete(s->hd);
-- 
1.6.5.2