[Qemu-devel] [PATCH 0/6] Add various VMDK subformats support

[Qemu-devel] [PATCH 0/6] Add various VMDK subformats support

[Fam Zheng]

Add subformats support for:
    twoGbMaxExtentFlat
    twoGbMaxExtentSparse
    streamOptimized

Fam Zheng (6):
  VMDK: enable twoGbMaxExtentFlat
  VMDK: add twoGbMaxExtentSparse support
  VMDK: separate vmdk_read_extent/vmdk_write_extent
  VMDK: Opening compressed extent.
  VMDK: read/write compressed extent
  VMDK: creating streamOptimized subformat

 block/vmdk.c |  316 +++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 248 insertions(+), 68 deletions(-)

[Qemu-devel] [PATCH 1/6] VMDK: enable twoGbMaxExtentFlat

[Fam Zheng]

Enable the createType 'twoGbMaxExtentFlat'. The supporting code is
already in.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 37478d2..9e6c67a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -551,7 +551,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
         return -EINVAL;
     }
-    if (strcmp(ct, "monolithicFlat")) {
+    if (strcmp(ct, "monolithicFlat") &&
+        strcmp(ct, "twoGbMaxExtentFlat")) {
         fprintf(stderr,
                 "VMDK: Not supported image type \"%s\""".\n", ct);
         return -ENOTSUP;

[Qemu-devel] [PATCH 2/6] VMDK: add twoGbMaxExtentSparse support

[Fam Zheng]

Add twoGbMaxExtentSparse support. Only opening code is changed.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |  124 ++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 77 insertions(+), 47 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 9e6c67a..9d1ae32 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -357,7 +357,9 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
     return ret;
 }
 
-static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
+static int vmdk_open_vmdk3(BlockDriverState *bs,
+                           BlockDriverState *file,
+                           int flags)
 {
     int ret;
     uint32_t magic;
@@ -365,10 +367,9 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
-    s->desc_offset = 0x200;
-    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
+    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
-        goto fail;
+        return ret;
     }
     extent = vmdk_add_extent(bs,
                              bs->file, false,
@@ -378,17 +379,16 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
                              le32_to_cpu(header.granularity));
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
-        /* vmdk_init_tables cleans up on fail, so only free allocation of
-         * vmdk_add_extent here. */
-        goto fail;
+        /* free extent allocated by vmdk_add_extent */
+        s->num_extents--;
+        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
     }
-    return 0;
- fail:
-    vmdk_free_extents(bs);
     return ret;
 }
 
-static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
+static int vmdk_open_vmdk4(BlockDriverState *bs,
+                           BlockDriverState *file,
+                           int flags)
 {
     int ret;
     uint32_t magic;
@@ -397,39 +397,30 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
-    s->desc_offset = 0x200;
-    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
+    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
-        goto fail;
+        return ret;
     }
     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
                         * le64_to_cpu(header.granularity);
+    if (l1_entry_sectors <= 0) {
+        return -EINVAL;
+    }
     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
                 / l1_entry_sectors;
-    extent = vmdk_add_extent(bs, bs->file, false,
+    extent = vmdk_add_extent(bs, file, false,
                           le64_to_cpu(header.capacity),
                           le64_to_cpu(header.gd_offset) << 9,
                           le64_to_cpu(header.rgd_offset) << 9,
                           l1_size,
                           le32_to_cpu(header.num_gtes_per_gte),
                           le64_to_cpu(header.granularity));
-    if (extent->l1_entry_sectors <= 0) {
-        ret = -EINVAL;
-        goto fail;
-    }
-    /* try to open parent images, if exist */
-    ret = vmdk_parent_open(bs);
-    if (ret) {
-        goto fail;
-    }
-    s->parent_cid = vmdk_read_cid(bs, 1);
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
-        goto fail;
+        /* free extent allocated by vmdk_add_extent */
+        s->num_extents--;
+        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
     }
-    return 0;
- fail:
-    vmdk_free_extents(bs);
     return ret;
 }
 
@@ -460,6 +451,35 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
     return 0;
 }
 
+/* Open an extent file and append to bs array */
+static int vmdk_open_sparse(BlockDriverState *bs,
+                            BlockDriverState *file,
+                            int flags)
+{
+    int ret;
+    uint32_t magic;
+
+    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
+        return -EIO;
+    }
+
+    magic = be32_to_cpu(magic);
+    if (magic == VMDK3_MAGIC) {
+        ret = vmdk_open_vmdk3(bs, file, flags);
+        if (ret) {
+            return ret;
+        }
+    } else if (magic == VMDK4_MAGIC) {
+        ret = vmdk_open_vmdk4(bs, file, flags);
+        if (ret) {
+            return ret;
+        }
+    } else {
+        return -EINVAL;
+    }
+    return 0;
+}
+
 static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         const char *desc_file_path)
 {
@@ -470,6 +490,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     const char *p = desc;
     int64_t sectors = 0;
     int64_t flat_offset;
+    char extent_path[PATH_MAX];
+    BlockDriverState *extent_file;
 
     while (*p) {
         /* parse extent line:
@@ -504,22 +526,28 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             goto next_line;
         }
 
+        path_combine(extent_path, sizeof(extent_path),
+                desc_file_path, fname);
+        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
+        if (ret) {
+            return ret;
+        }
+
         /* save to extents array */
         if (!strcmp(type, "FLAT")) {
             /* FLAT extent */
-            char extent_path[PATH_MAX];
-            BlockDriverState *extent_file;
             VmdkExtent *extent;
 
-            path_combine(extent_path, sizeof(extent_path),
-                    desc_file_path, fname);
-            ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
-            if (ret) {
-                return ret;
-            }
             extent = vmdk_add_extent(bs, extent_file, true, sectors,
                             0, 0, 0, 0, sectors);
             extent->flat_start_offset = flat_offset;
+        } else if (!strcmp(type, "SPARSE")) {
+            /* SPARSE extent */
+            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
+            if (ret) {
+                bdrv_delete(extent_file);
+                return ret;
+            }
         } else {
             /* SPARSE extent, not supported for now */
             fprintf(stderr,
@@ -552,6 +580,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
         return -EINVAL;
     }
     if (strcmp(ct, "monolithicFlat") &&
+        strcmp(ct, "twoGbMaxExtentSparse") &&
         strcmp(ct, "twoGbMaxExtentFlat")) {
         fprintf(stderr,
                 "VMDK: Not supported image type \"%s\""".\n", ct);
@@ -574,17 +603,18 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
 
 static int vmdk_open(BlockDriverState *bs, int flags)
 {
-    uint32_t magic;
-
-    if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
-        return -EIO;
-    }
+    int ret;
+    BDRVVmdkState *s = bs->opaque;
 
-    magic = be32_to_cpu(magic);
-    if (magic == VMDK3_MAGIC) {
-        return vmdk_open_vmdk3(bs, flags);
-    } else if (magic == VMDK4_MAGIC) {
-        return vmdk_open_vmdk4(bs, flags);
+    if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
+        s->desc_offset = 0x200;
+        /* try to open parent images, if exist */
+        ret = vmdk_parent_open(bs);
+        if (ret) {
+            return ret;
+        }
+        s->parent_cid = vmdk_read_cid(bs, 1);
+        return 0;
     } else {
         return vmdk_open_desc_file(bs, flags);
     }

[Qemu-devel] [PATCH 3/6] VMDK: separate vmdk_read_extent/vmdk_write_extent

[Fam Zheng]

Factor out read/write extent code, since there will be more things to
take care of once reading/writing compressed clusters is introduced.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |   54 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 9d1ae32..0d989f6 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -829,6 +829,43 @@ static int vmdk_is_allocated(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
+static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
+                            int64_t offset_in_cluster, const uint8_t *buf,
+                            int nb_sectors, int64_t sector_num)
+{
+    int ret;
+    const uint8_t *write_buf = buf;
+    int write_len = nb_sectors * 512;
+
+    ret = bdrv_pwrite(extent->file,
+                        cluster_offset + offset_in_cluster,
+                        write_buf,
+                        write_len);
+    if (ret != write_len) {
+        ret = ret < 0 ? ret : -EIO;
+        goto out;
+    }
+    ret = 0;
+ out:
+    return ret;
+}
+
+static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
+                            int64_t offset_in_cluster, uint8_t *buf,
+                            int nb_sectors)
+{
+    int ret;
+
+    ret = bdrv_pread(extent->file,
+                      cluster_offset + offset_in_cluster,
+                      buf, nb_sectors * 512);
+    if (ret == nb_sectors * 512) {
+        return 0;
+    } else {
+        return -EIO;
+    }
+}
+
 static int vmdk_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -865,10 +902,10 @@ static int vmdk_read(BlockDriverState *bs, int64_t sector_num,
                 memset(buf, 0, 512 * n);
             }
         } else {
-            ret = bdrv_pread(extent->file,
-                            cluster_offset + index_in_cluster * 512,
-                            buf, n * 512);
-            if (ret < 0) {
+            ret = vmdk_read_extent(extent,
+                            cluster_offset, index_in_cluster * 512,
+                            buf, n);
+            if (ret) {
                 return ret;
             }
         }
@@ -917,11 +954,10 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
             n = nb_sectors;
         }
 
-        ret = bdrv_pwrite(extent->file,
-                        cluster_offset + index_in_cluster * 512,
-                        buf,
-                        n * 512);
-        if (ret < 0) {
+        ret = vmdk_write_extent(extent,
+                        cluster_offset, index_in_cluster * 512,
+                        buf, n, sector_num);
+        if (ret) {
             return ret;
         }
         if (m_data.valid) {

[Qemu-devel] [PATCH 4/6] VMDK: Opening compressed extent.

[Fam Zheng]

Added flags field for compressed/streamOptimized extents, open and save
image configuration.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 0d989f6..5f1638e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -26,9 +26,13 @@
 #include "qemu-common.h"
 #include "block_int.h"
 #include "module.h"
+#include "zlib.h"
 
 #define VMDK3_MAGIC (('C' << 24) | ('O' << 16) | ('W' << 8) | 'D')
 #define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V')
+#define VMDK4_COMPRESSION_DEFLATE 1
+#define VMDK4_FLAG_COMPRESS (1 << 16)
+#define VMDK4_FLAG_MARKER (1 << 17)
 
 typedef struct {
     uint32_t version;
@@ -56,6 +60,7 @@ typedef struct {
     int64_t grain_offset;
     char filler[1];
     char check_bytes[4];
+    uint16_t compressAlgorithm;
 } __attribute__((packed)) VMDK4Header;
 
 #define L2_CACHE_SIZE 16
@@ -63,6 +68,8 @@ typedef struct {
 typedef struct VmdkExtent {
     BlockDriverState *file;
     bool flat;
+    bool compressed;
+    bool has_marker;
     int64_t sectors;
     int64_t end_sector;
     int64_t flat_start_offset;
@@ -98,6 +105,12 @@ typedef struct VmdkMetaData {
     int valid;
 } VmdkMetaData;
 
+typedef struct VmdkGrainMarker {
+    uint64_t lba;
+    uint32_t size;
+    uint8_t  data[0];
+} VmdkGrainMarker;
+
 static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     uint32_t magic;
@@ -415,6 +428,9 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
                           l1_size,
                           le32_to_cpu(header.num_gtes_per_gte),
                           le64_to_cpu(header.granularity));
+    extent->compressed =
+        le16_to_cpu(header.compressAlgorithm) == VMDK4_COMPRESSION_DEFLATE;
+    extent->has_marker = header.flags & VMDK4_FLAG_MARKER;
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
         /* free extent allocated by vmdk_add_extent */

[Qemu-devel] [PATCH 5/6] VMDK: read/write compressed extent

[Fam Zheng]

Add support for reading/writing compressed extent.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |  115 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 103 insertions(+), 12 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 5f1638e..4799aa5 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -771,10 +771,12 @@ static int get_cluster_offset(BlockDriverState *bs,
 
         /* Avoid the L2 tables update for the images that have snapshots. */
         *cluster_offset = bdrv_getlength(extent->file);
-        bdrv_truncate(
-            extent->file,
-            *cluster_offset + (extent->cluster_sectors << 9)
-        );
+        if (!extent->compressed) {
+            bdrv_truncate(
+                extent->file,
+                *cluster_offset + (extent->cluster_sectors << 9)
+            );
+        }
 
         *cluster_offset >>= 9;
         tmp = cpu_to_le32(*cluster_offset);
@@ -850,9 +852,28 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
                             int nb_sectors, int64_t sector_num)
 {
     int ret;
+    VmdkGrainMarker *data = NULL;
+    uLongf buf_len;
     const uint8_t *write_buf = buf;
     int write_len = nb_sectors * 512;
 
+    if (extent->compressed) {
+        if (!extent->has_marker) {
+            ret = -EINVAL;
+            goto out;
+        }
+        buf_len = (extent->cluster_sectors << 9) * 2;
+        data = qemu_mallocz(buf_len + sizeof(VmdkGrainMarker));
+        if (compress(data->data, &buf_len, buf, nb_sectors << 9) != Z_OK ||
+                buf_len == 0) {
+            ret = -EINVAL;
+            goto out;
+        }
+        data->lba = sector_num;
+        data->size = buf_len;
+        write_buf = (uint8_t *)data;
+        write_len = buf_len + sizeof(VmdkGrainMarker);
+    }
     ret = bdrv_pwrite(extent->file,
                         cluster_offset + offset_in_cluster,
                         write_buf,
@@ -863,6 +884,9 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
     }
     ret = 0;
  out:
+    if (data) {
+        qemu_free(data);
+    }
     return ret;
 }
 
@@ -871,15 +895,65 @@ static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
                             int nb_sectors)
 {
     int ret;
-
+    int cluster_bytes, buf_bytes;
+    uint8_t *cluster_buf, *compressed_data;
+    uint8_t *uncomp_buf;
+    uint32_t data_len;
+    VmdkGrainMarker *marker;
+    uLongf buf_len;
+ 
+
+    if (!extent->compressed) {
+        ret = bdrv_pread(extent->file,
+                          cluster_offset + offset_in_cluster,
+                          buf, nb_sectors * 512);
+        if (ret == nb_sectors * 512) {
+            return 0;
+        } else {
+            return -EIO;
+        }
+    }
+    cluster_bytes = extent->cluster_sectors * 512;
+    /* Read two clusters in case GrainMarker + compressed data > one cluster */
+    buf_bytes = cluster_bytes * 2;
+    cluster_buf = qemu_mallocz(buf_bytes);
+    uncomp_buf = qemu_mallocz(cluster_bytes);
     ret = bdrv_pread(extent->file,
-                      cluster_offset + offset_in_cluster,
-                      buf, nb_sectors * 512);
-    if (ret == nb_sectors * 512) {
-        return 0;
-    } else {
-        return -EIO;
+                cluster_offset,
+                cluster_buf, buf_bytes);
+    if (ret < 0) {
+        goto out;
+    }
+    compressed_data = cluster_buf;
+    buf_len = cluster_bytes;
+    data_len = cluster_bytes;
+    if (extent->has_marker) {
+        marker = (VmdkGrainMarker *)cluster_buf;
+        compressed_data = marker->data;
+        data_len = marker->size;
+    }
+    if (!data_len || data_len > buf_bytes) {
+        ret = -EINVAL;
+        goto out;
+    }
+    ret = uncompress(uncomp_buf, &buf_len, compressed_data, data_len);
+    if (ret != Z_OK) {
+        ret = -EINVAL;
+        goto out;
+
+    }
+    if (offset_in_cluster < 0 ||
+            offset_in_cluster + nb_sectors * 512 > buf_len) {
+        ret = -EINVAL;
+        goto out;
     }
+    memcpy(buf, uncomp_buf + offset_in_cluster, nb_sectors * 512);
+    ret = 0;
+
+ out:
+    qemu_free(uncomp_buf);
+    qemu_free(cluster_buf);
+    return ret;
 }
 
 static int vmdk_read(BlockDriverState *bs, int64_t sector_num,
@@ -959,8 +1033,25 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
                                 bs,
                                 extent,
                                 &m_data,
-                                sector_num << 9, 1,
+                                sector_num << 9, !extent->compressed,
                                 &cluster_offset);
+        if (extent->compressed) {
+            if (ret == 0) {
+                /* Refuse write to allocated cluster for streamOptimized */
+                fprintf(stderr,
+                        "VMDK: can't write to allocated cluster"
+                        " for streamOptimized\n");
+                return -EIO;
+            } else {
+                /* allocate */
+                ret = get_cluster_offset(
+                                        bs,
+                                        extent,
+                                        &m_data,
+                                        sector_num << 9, 1,
+                                        &cluster_offset);
+            }
+        }
         if (ret) {
             return -EINVAL;
         }

[Qemu-devel] [PATCH 6/6] VMDK: creating streamOptimized subformat

[Fam Zheng]

Creating streamOptimized subformat. Added subformat option
'streamOptimized', to create a image with compression enabled and each
cluster with a GrainMarker.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 4799aa5..52e518d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1088,7 +1088,8 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
 }
 
 
-static int vmdk_create_extent(const char *filename, int64_t filesize, bool flat)
+static int vmdk_create_extent(const char *filename, int64_t filesize,
+                              bool flat, bool compress)
 {
     int ret, i;
     int fd = 0;
@@ -1112,7 +1113,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, bool flat)
     magic = cpu_to_be32(VMDK4_MAGIC);
     memset(&header, 0, sizeof(header));
     header.version = 1;
-    header.flags = 3; /* ?? */
+    header.flags =
+        3 | (compress ? VMDK4_FLAG_COMPRESS | VMDK4_FLAG_MARKER : 0);
+    header.compressAlgorithm = compress ? VMDK4_COMPRESSION_DEFLATE : 0;
     header.capacity = filesize / 512;
     header.granularity = 128;
     header.num_gtes_per_gte = 512;
@@ -1142,6 +1145,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, bool flat)
     header.rgd_offset = cpu_to_le64(header.rgd_offset);
     header.gd_offset = cpu_to_le64(header.gd_offset);
     header.grain_offset = cpu_to_le64(header.grain_offset);
+    header.compressAlgorithm = cpu_to_le16(header.compressAlgorithm);
 
     header.check_bytes[0] = 0xa;
     header.check_bytes[1] = 0x20;
@@ -1283,7 +1287,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     const char *fmt = NULL;
     int flags = 0;
     int ret = 0;
-    bool flat, split;
+    bool flat, split, compress;
     char ext_desc_lines[BUF_SIZE] = "";
     char path[PATH_MAX], prefix[PATH_MAX], postfix[PATH_MAX];
     const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
@@ -1332,7 +1336,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     } else if (strcmp(fmt, "monolithicFlat") &&
                strcmp(fmt, "monolithicSparse") &&
                strcmp(fmt, "twoGbMaxExtentSparse") &&
-               strcmp(fmt, "twoGbMaxExtentFlat")) {
+               strcmp(fmt, "twoGbMaxExtentFlat") &&
+               strcmp(fmt, "streamOptimized")) {
         fprintf(stderr, "VMDK: Unknown subformat: %s\n", fmt);
         return -EINVAL;
     }
@@ -1340,6 +1345,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
               strcmp(fmt, "twoGbMaxExtentSparse"));
     flat = !(strcmp(fmt, "monolithicFlat") &&
              strcmp(fmt, "twoGbMaxExtentFlat"));
+    compress = !strcmp(fmt, "streamOptimized");
     if (flat) {
         desc_extent_line = "RW %lld FLAT \"%s\" 0\n";
     } else {
@@ -1394,7 +1400,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
         snprintf(ext_filename, sizeof(ext_filename), "%s%s",
                 path, desc_filename);
 
-        if (vmdk_create_extent(ext_filename, size, flat)) {
+        if (vmdk_create_extent(ext_filename, size, flat, compress)) {
             return -EINVAL;
         }
         filesize -= size;
@@ -1508,7 +1514,7 @@ static QEMUOptionParameter vmdk_create_options[] = {
         .type = OPT_STRING,
         .help =
             "VMDK flat extent format, can be one of "
-            "{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse | twoGbMaxExtentFlat} "
+            "{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse | twoGbMaxExtentFlat | streamOptimized} "
     },
     { NULL }
 };

Re: [Qemu-devel] [PATCH 1/6] VMDK: enable twoGbMaxExtentFlat

[Fam Zheng]

Sorry, I missed one line in this patch. Will fix it in V2.

On Wed, Jul 27, 2011 at 5:27 PM, Fam Zheng <famcool@gmail.com> wrote:
> Enable the createType 'twoGbMaxExtentFlat'. The supporting code is
> already in.
>
> Signed-off-by: Fam Zheng <famcool@gmail.com>
> ---
>  block/vmdk.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 37478d2..9e6c67a 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -551,7 +551,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
>     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
>         return -EINVAL;
>     }
> -    if (strcmp(ct, "monolithicFlat")) {
> +    if (strcmp(ct, "monolithicFlat") &&
> +        strcmp(ct, "twoGbMaxExtentFlat")) {
>         fprintf(stderr,
>                 "VMDK: Not supported image type \"%s\""".\n", ct);
>         return -ENOTSUP;
>



-- 
Best regards!
Fam Zheng

Re: [Qemu-devel] [PATCH 1/6] VMDK: enable twoGbMaxExtentFlat

[Fam Zheng]

Enable the createType 'twoGbMaxExtentFlat'. The supporting code is
already in.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 37478d2..e22a893 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -551,7 +551,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
         return -EINVAL;
     }
-    if (strcmp(ct, "monolithicFlat")) {
+    if (strcmp(ct, "monolithicFlat") &&
+        strcmp(ct, "twoGbMaxExtentFlat")) {
         fprintf(stderr,
                 "VMDK: Not supported image type \"%s\""".\n", ct);
         return -ENOTSUP;
@@ -672,6 +673,7 @@ static int get_cluster_offset(BlockDriverState *bs,
         return 0;
     }
 
+    offset -= (extent->end_sector - extent->sectors) * SECTOR_SIZE;
     l1_index = (offset >> 9) / extent->l1_entry_sectors;
     if (l1_index >= extent->l1_size) {
         return -1;

Re: [Qemu-devel] [PATCH 2/6] VMDK: add twoGbMaxExtentSparse support

[Stefan Hajnoczi]

On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famcool@gmail.com> wrote:
> Add twoGbMaxExtentSparse support. Only opening code is changed.
>
> Signed-off-by: Fam Zheng <famcool@gmail.com>
> ---
>  block/vmdk.c |  124 ++++++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 77 insertions(+), 47 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 9e6c67a..9d1ae32 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -357,7 +357,9 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
>     return ret;
>  }
>
> -static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
> +static int vmdk_open_vmdk3(BlockDriverState *bs,
> +                           BlockDriverState *file,
> +                           int flags)
>  {
>     int ret;
>     uint32_t magic;
> @@ -365,10 +367,9 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>     BDRVVmdkState *s = bs->opaque;
>     VmdkExtent *extent;
>
> -    s->desc_offset = 0x200;
> -    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
> +    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>     if (ret < 0) {
> -        goto fail;
> +        return ret;
>     }
>     extent = vmdk_add_extent(bs,
>                              bs->file, false,
> @@ -378,17 +379,16 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>                              le32_to_cpu(header.granularity));
>     ret = vmdk_init_tables(bs, extent);
>     if (ret) {
> -        /* vmdk_init_tables cleans up on fail, so only free allocation of
> -         * vmdk_add_extent here. */
> -        goto fail;
> +        /* free extent allocated by vmdk_add_extent */
> +        s->num_extents--;
> +        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));

The return value of qemu_realloc() is the new address of s->extents[].
 You are ignoring the new address and continue to use the old one.

Please add a vmdk_free_last_extent() function that encapsulates this.
The rest of the vmdk.c code should not manipulate
s->extents[]/s->num_extents.  Use vmdk_add_extent(),
vmdk_free_last_extent(), vmdk_free_extents() instead of spreading the
extent management code in various places.

>     }
> -    return 0;
> - fail:
> -    vmdk_free_extents(bs);
>     return ret;
>  }
>
> -static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
> +static int vmdk_open_vmdk4(BlockDriverState *bs,
> +                           BlockDriverState *file,
> +                           int flags)
>  {
>     int ret;
>     uint32_t magic;
> @@ -397,39 +397,30 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
>     BDRVVmdkState *s = bs->opaque;
>     VmdkExtent *extent;
>
> -    s->desc_offset = 0x200;
> -    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
> +    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>     if (ret < 0) {
> -        goto fail;
> +        return ret;
>     }
>     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
>                         * le64_to_cpu(header.granularity);
> +    if (l1_entry_sectors <= 0) {
> +        return -EINVAL;
> +    }
>     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
>                 / l1_entry_sectors;
> -    extent = vmdk_add_extent(bs, bs->file, false,
> +    extent = vmdk_add_extent(bs, file, false,
>                           le64_to_cpu(header.capacity),
>                           le64_to_cpu(header.gd_offset) << 9,
>                           le64_to_cpu(header.rgd_offset) << 9,
>                           l1_size,
>                           le32_to_cpu(header.num_gtes_per_gte),
>                           le64_to_cpu(header.granularity));
> -    if (extent->l1_entry_sectors <= 0) {
> -        ret = -EINVAL;
> -        goto fail;
> -    }
> -    /* try to open parent images, if exist */
> -    ret = vmdk_parent_open(bs);
> -    if (ret) {
> -        goto fail;
> -    }
> -    s->parent_cid = vmdk_read_cid(bs, 1);
>     ret = vmdk_init_tables(bs, extent);
>     if (ret) {
> -        goto fail;
> +        /* free extent allocated by vmdk_add_extent */
> +        s->num_extents--;
> +        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));

Same bug.

>     }
> -    return 0;
> - fail:
> -    vmdk_free_extents(bs);
>     return ret;
>  }
>
> @@ -460,6 +451,35 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
>     return 0;
>  }
>
> +/* Open an extent file and append to bs array */
> +static int vmdk_open_sparse(BlockDriverState *bs,
> +                            BlockDriverState *file,
> +                            int flags)
> +{
> +    int ret;
> +    uint32_t magic;
> +
> +    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
> +        return -EIO;
> +    }
> +
> +    magic = be32_to_cpu(magic);
> +    if (magic == VMDK3_MAGIC) {
> +        ret = vmdk_open_vmdk3(bs, file, flags);
> +        if (ret) {
> +            return ret;
> +        }
> +    } else if (magic == VMDK4_MAGIC) {
> +        ret = vmdk_open_vmdk4(bs, file, flags);
> +        if (ret) {
> +            return ret;
> +        }
> +    } else {
> +        return -EINVAL;
> +    }
> +    return 0;

Shorter way of writing this (ret is no longer necessary):

switch (magic) {
case VMDK3_MAGIC:
    return vmdk_open_vmdk3(bs, file, flags);
case VMDK4_MAGIC:
    return vmdk_open_vmdk4(bs, file, flags);
default:
    return -EINVAL;
}

> +}
> +
>  static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>         const char *desc_file_path)
>  {
> @@ -470,6 +490,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>     const char *p = desc;
>     int64_t sectors = 0;
>     int64_t flat_offset;
> +    char extent_path[PATH_MAX];
> +    BlockDriverState *extent_file;
>
>     while (*p) {
>         /* parse extent line:
> @@ -504,22 +526,28 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>             goto next_line;
>         }
>
> +        path_combine(extent_path, sizeof(extent_path),
> +                desc_file_path, fname);
> +        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
> +        if (ret) {
> +            return ret;
> +        }
> +
>         /* save to extents array */
>         if (!strcmp(type, "FLAT")) {
>             /* FLAT extent */
> -            char extent_path[PATH_MAX];
> -            BlockDriverState *extent_file;
>             VmdkExtent *extent;
>
> -            path_combine(extent_path, sizeof(extent_path),
> -                    desc_file_path, fname);
> -            ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
> -            if (ret) {
> -                return ret;
> -            }
>             extent = vmdk_add_extent(bs, extent_file, true, sectors,
>                             0, 0, 0, 0, sectors);
>             extent->flat_start_offset = flat_offset;
> +        } else if (!strcmp(type, "SPARSE")) {
> +            /* SPARSE extent */
> +            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
> +            if (ret) {
> +                bdrv_delete(extent_file);
> +                return ret;
> +            }
>         } else {
>             /* SPARSE extent, not supported for now */

This comment is outdated.

>             fprintf(stderr,
> @@ -552,6 +580,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
>         return -EINVAL;
>     }
>     if (strcmp(ct, "monolithicFlat") &&
> +        strcmp(ct, "twoGbMaxExtentSparse") &&
>         strcmp(ct, "twoGbMaxExtentFlat")) {
>         fprintf(stderr,
>                 "VMDK: Not supported image type \"%s\""".\n", ct);
> @@ -574,17 +603,18 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
>
>  static int vmdk_open(BlockDriverState *bs, int flags)
>  {
> -    uint32_t magic;
> -
> -    if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
> -        return -EIO;
> -    }
> +    int ret;
> +    BDRVVmdkState *s = bs->opaque;
>
> -    magic = be32_to_cpu(magic);
> -    if (magic == VMDK3_MAGIC) {
> -        return vmdk_open_vmdk3(bs, flags);
> -    } else if (magic == VMDK4_MAGIC) {
> -        return vmdk_open_vmdk4(bs, flags);
> +    if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
> +        s->desc_offset = 0x200;
> +        /* try to open parent images, if exist */
> +        ret = vmdk_parent_open(bs);
> +        if (ret) {
> +            return ret;

What needs to be freed here?

Stefan

Re: [Qemu-devel] [PATCH 4/6] VMDK: Opening compressed extent.

[Stefan Hajnoczi]

On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famcool@gmail.com> wrote:
> @@ -415,6 +428,9 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>                           l1_size,
>                           le32_to_cpu(header.num_gtes_per_gte),
>                           le64_to_cpu(header.granularity));
> +    extent->compressed =
> +        le16_to_cpu(header.compressAlgorithm) == VMDK4_COMPRESSION_DEFLATE;
> +    extent->has_marker = header.flags & VMDK4_FLAG_MARKER;

le32_to_cpu(header.flags)?

Stefan

Re: [Qemu-devel] [PATCH 5/6] VMDK: read/write compressed extent

[Stefan Hajnoczi]

On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famcool@gmail.com> wrote:
> @@ -863,6 +884,9 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
>     }
>     ret = 0;
>  out:
> +    if (data) {
> +        qemu_free(data);
> +    }

qemu_free(NULL) is a nop, you don't need the if statement.  Just call
qemu_free(data).

>     return ret;
>  }
>
> @@ -871,15 +895,65 @@ static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
>                             int nb_sectors)
>  {
>     int ret;
> -
> +    int cluster_bytes, buf_bytes;
> +    uint8_t *cluster_buf, *compressed_data;
> +    uint8_t *uncomp_buf;
> +    uint32_t data_len;
> +    VmdkGrainMarker *marker;
> +    uLongf buf_len;
> +
> +
> +    if (!extent->compressed) {
> +        ret = bdrv_pread(extent->file,
> +                          cluster_offset + offset_in_cluster,
> +                          buf, nb_sectors * 512);
> +        if (ret == nb_sectors * 512) {
> +            return 0;
> +        } else {
> +            return -EIO;
> +        }
> +    }
> +    cluster_bytes = extent->cluster_sectors * 512;
> +    /* Read two clusters in case GrainMarker + compressed data > one cluster */
> +    buf_bytes = cluster_bytes * 2;
> +    cluster_buf = qemu_mallocz(buf_bytes);
> +    uncomp_buf = qemu_mallocz(cluster_bytes);

Do these really need to be zeroed?

>     ret = bdrv_pread(extent->file,
> -                      cluster_offset + offset_in_cluster,
> -                      buf, nb_sectors * 512);
> -    if (ret == nb_sectors * 512) {
> -        return 0;
> -    } else {
> -        return -EIO;
> +                cluster_offset,
> +                cluster_buf, buf_bytes);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +    compressed_data = cluster_buf;
> +    buf_len = cluster_bytes;
> +    data_len = cluster_bytes;
> +    if (extent->has_marker) {
> +        marker = (VmdkGrainMarker *)cluster_buf;
> +        compressed_data = marker->data;
> +        data_len = marker->size;

Endianness?

Stefan

Re: [Qemu-devel] [PATCH 2/6] VMDK: add twoGbMaxExtentSparse support

[Fam Zheng]

On Tue, Aug 2, 2011 at 6:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famcool@gmail.com> wrote:
>> Add twoGbMaxExtentSparse support. Only opening code is changed.
>>
>> Signed-off-by: Fam Zheng <famcool@gmail.com>
>> ---
>>  block/vmdk.c |  124 ++++++++++++++++++++++++++++++++++++----------------------
>>  1 files changed, 77 insertions(+), 47 deletions(-)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 9e6c67a..9d1ae32 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -357,7 +357,9 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
>>     return ret;
>>  }
>>
>> -static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>> +static int vmdk_open_vmdk3(BlockDriverState *bs,
>> +                           BlockDriverState *file,
>> +                           int flags)
>>  {
>>     int ret;
>>     uint32_t magic;
>> @@ -365,10 +367,9 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>>     BDRVVmdkState *s = bs->opaque;
>>     VmdkExtent *extent;
>>
>> -    s->desc_offset = 0x200;
>> -    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
>> +    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>>     if (ret < 0) {
>> -        goto fail;
>> +        return ret;
>>     }
>>     extent = vmdk_add_extent(bs,
>>                              bs->file, false,
>> @@ -378,17 +379,16 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>>                              le32_to_cpu(header.granularity));
>>     ret = vmdk_init_tables(bs, extent);
>>     if (ret) {
>> -        /* vmdk_init_tables cleans up on fail, so only free allocation of
>> -         * vmdk_add_extent here. */
>> -        goto fail;
>> +        /* free extent allocated by vmdk_add_extent */
>> +        s->num_extents--;
>> +        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
>
> The return value of qemu_realloc() is the new address of s->extents[].
>  You are ignoring the new address and continue to use the old one.
>
> Please add a vmdk_free_last_extent() function that encapsulates this.
> The rest of the vmdk.c code should not manipulate
> s->extents[]/s->num_extents.  Use vmdk_add_extent(),
> vmdk_free_last_extent(), vmdk_free_extents() instead of spreading the
> extent management code in various places.
>
>>     }
>> -    return 0;
>> - fail:
>> -    vmdk_free_extents(bs);
>>     return ret;
>>  }
>>
>> -static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
>> +static int vmdk_open_vmdk4(BlockDriverState *bs,
>> +                           BlockDriverState *file,
>> +                           int flags)
>>  {
>>     int ret;
>>     uint32_t magic;
>> @@ -397,39 +397,30 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
>>     BDRVVmdkState *s = bs->opaque;
>>     VmdkExtent *extent;
>>
>> -    s->desc_offset = 0x200;
>> -    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
>> +    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>>     if (ret < 0) {
>> -        goto fail;
>> +        return ret;
>>     }
>>     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
>>                         * le64_to_cpu(header.granularity);
>> +    if (l1_entry_sectors <= 0) {
>> +        return -EINVAL;
>> +    }
>>     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
>>                 / l1_entry_sectors;
>> -    extent = vmdk_add_extent(bs, bs->file, false,
>> +    extent = vmdk_add_extent(bs, file, false,
>>                           le64_to_cpu(header.capacity),
>>                           le64_to_cpu(header.gd_offset) << 9,
>>                           le64_to_cpu(header.rgd_offset) << 9,
>>                           l1_size,
>>                           le32_to_cpu(header.num_gtes_per_gte),
>>                           le64_to_cpu(header.granularity));
>> -    if (extent->l1_entry_sectors <= 0) {
>> -        ret = -EINVAL;
>> -        goto fail;
>> -    }
>> -    /* try to open parent images, if exist */
>> -    ret = vmdk_parent_open(bs);
>> -    if (ret) {
>> -        goto fail;
>> -    }
>> -    s->parent_cid = vmdk_read_cid(bs, 1);
>>     ret = vmdk_init_tables(bs, extent);
>>     if (ret) {
>> -        goto fail;
>> +        /* free extent allocated by vmdk_add_extent */
>> +        s->num_extents--;
>> +        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
>
> Same bug.
>
>>     }
>> -    return 0;
>> - fail:
>> -    vmdk_free_extents(bs);
>>     return ret;
>>  }
>>
>> @@ -460,6 +451,35 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
>>     return 0;
>>  }
>>
>> +/* Open an extent file and append to bs array */
>> +static int vmdk_open_sparse(BlockDriverState *bs,
>> +                            BlockDriverState *file,
>> +                            int flags)
>> +{
>> +    int ret;
>> +    uint32_t magic;
>> +
>> +    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
>> +        return -EIO;
>> +    }
>> +
>> +    magic = be32_to_cpu(magic);
>> +    if (magic == VMDK3_MAGIC) {
>> +        ret = vmdk_open_vmdk3(bs, file, flags);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +    } else if (magic == VMDK4_MAGIC) {
>> +        ret = vmdk_open_vmdk4(bs, file, flags);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +    } else {
>> +        return -EINVAL;
>> +    }
>> +    return 0;
>
> Shorter way of writing this (ret is no longer necessary):
>
> switch (magic) {
> case VMDK3_MAGIC:
>    return vmdk_open_vmdk3(bs, file, flags);
> case VMDK4_MAGIC:
>    return vmdk_open_vmdk4(bs, file, flags);
> default:
>    return -EINVAL;
> }
>
>> +}
>> +
>>  static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>         const char *desc_file_path)
>>  {
>> @@ -470,6 +490,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>     const char *p = desc;
>>     int64_t sectors = 0;
>>     int64_t flat_offset;
>> +    char extent_path[PATH_MAX];
>> +    BlockDriverState *extent_file;
>>
>>     while (*p) {
>>         /* parse extent line:
>> @@ -504,22 +526,28 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>             goto next_line;
>>         }
>>
>> +        path_combine(extent_path, sizeof(extent_path),
>> +                desc_file_path, fname);
>> +        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +
>>         /* save to extents array */
>>         if (!strcmp(type, "FLAT")) {
>>             /* FLAT extent */
>> -            char extent_path[PATH_MAX];
>> -            BlockDriverState *extent_file;
>>             VmdkExtent *extent;
>>
>> -            path_combine(extent_path, sizeof(extent_path),
>> -                    desc_file_path, fname);
>> -            ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
>> -            if (ret) {
>> -                return ret;
>> -            }
>>             extent = vmdk_add_extent(bs, extent_file, true, sectors,
>>                             0, 0, 0, 0, sectors);
>>             extent->flat_start_offset = flat_offset;
>> +        } else if (!strcmp(type, "SPARSE")) {
>> +            /* SPARSE extent */
>> +            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
>> +            if (ret) {
>> +                bdrv_delete(extent_file);
>> +                return ret;
>> +            }
>>         } else {
>>             /* SPARSE extent, not supported for now */
>
> This comment is outdated.
>
>>             fprintf(stderr,
>> @@ -552,6 +580,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
>>         return -EINVAL;
>>     }
>>     if (strcmp(ct, "monolithicFlat") &&
>> +        strcmp(ct, "twoGbMaxExtentSparse") &&
>>         strcmp(ct, "twoGbMaxExtentFlat")) {
>>         fprintf(stderr,
>>                 "VMDK: Not supported image type \"%s\""".\n", ct);
>> @@ -574,17 +603,18 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
>>
>>  static int vmdk_open(BlockDriverState *bs, int flags)
>>  {
>> -    uint32_t magic;
>> -
>> -    if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
>> -        return -EIO;
>> -    }
>> +    int ret;
>> +    BDRVVmdkState *s = bs->opaque;
>>
>> -    magic = be32_to_cpu(magic);
>> -    if (magic == VMDK3_MAGIC) {
>> -        return vmdk_open_vmdk3(bs, flags);
>> -    } else if (magic == VMDK4_MAGIC) {
>> -        return vmdk_open_vmdk4(bs, flags);
>> +    if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
>> +        s->desc_offset = 0x200;
>> +        /* try to open parent images, if exist */
>> +        ret = vmdk_parent_open(bs);
>> +        if (ret) {
>> +            return ret;
>
> What needs to be freed here?
>

No resource allocation in vmdk_parent_open, so I think none.


-- 
Best regards!
Fam Zheng

[Qemu-devel] [PATCH v2 2/6] VMDK: add twoGbMaxExtentSparse support

[Fam Zheng]

Add twoGbMaxExtentSparse support. Introduce vmdk_free_last_extent.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |  132 ++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 82 insertions(+), 50 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index e22a893..be3b860 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -174,6 +174,17 @@ static void vmdk_free_extents(BlockDriverState *bs)
     qemu_free(s->extents);
 }
 
+static void vmdk_free_last_extent(BlockDriverState *bs)
+{
+    BDRVVmdkState *s = bs->opaque;
+
+    if (s->num_extents == 0) {
+        return;
+    }
+    s->num_extents--;
+    s->extents = qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
+}
+
 static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
 {
     char desc[DESC_SIZE];
@@ -357,18 +368,18 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
     return ret;
 }
 
-static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
+static int vmdk_open_vmdk3(BlockDriverState *bs,
+                           BlockDriverState *file,
+                           int flags)
 {
     int ret;
     uint32_t magic;
     VMDK3Header header;
-    BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
-    s->desc_offset = 0x200;
-    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
+    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
-        goto fail;
+        return ret;
     }
     extent = vmdk_add_extent(bs,
                              bs->file, false,
@@ -378,58 +389,45 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
                              le32_to_cpu(header.granularity));
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
-        /* vmdk_init_tables cleans up on fail, so only free allocation of
-         * vmdk_add_extent here. */
-        goto fail;
+        /* free extent allocated by vmdk_add_extent */
+        vmdk_free_last_extent(bs);
     }
-    return 0;
- fail:
-    vmdk_free_extents(bs);
     return ret;
 }
 
-static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
+static int vmdk_open_vmdk4(BlockDriverState *bs,
+                           BlockDriverState *file,
+                           int flags)
 {
     int ret;
     uint32_t magic;
     uint32_t l1_size, l1_entry_sectors;
     VMDK4Header header;
-    BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
-    s->desc_offset = 0x200;
-    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
+    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
-        goto fail;
+        return ret;
     }
     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
                         * le64_to_cpu(header.granularity);
+    if (l1_entry_sectors <= 0) {
+        return -EINVAL;
+    }
     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
                 / l1_entry_sectors;
-    extent = vmdk_add_extent(bs, bs->file, false,
+    extent = vmdk_add_extent(bs, file, false,
                           le64_to_cpu(header.capacity),
                           le64_to_cpu(header.gd_offset) << 9,
                           le64_to_cpu(header.rgd_offset) << 9,
                           l1_size,
                           le32_to_cpu(header.num_gtes_per_gte),
                           le64_to_cpu(header.granularity));
-    if (extent->l1_entry_sectors <= 0) {
-        ret = -EINVAL;
-        goto fail;
-    }
-    /* try to open parent images, if exist */
-    ret = vmdk_parent_open(bs);
-    if (ret) {
-        goto fail;
-    }
-    s->parent_cid = vmdk_read_cid(bs, 1);
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
-        goto fail;
+        /* free extent allocated by vmdk_add_extent */
+        vmdk_free_last_extent(bs);
     }
-    return 0;
- fail:
-    vmdk_free_extents(bs);
     return ret;
 }
 
@@ -460,6 +458,31 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
     return 0;
 }
 
+/* Open an extent file and append to bs array */
+static int vmdk_open_sparse(BlockDriverState *bs,
+                            BlockDriverState *file,
+                            int flags)
+{
+    uint32_t magic;
+
+    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
+        return -EIO;
+    }
+
+    magic = be32_to_cpu(magic);
+    switch (magic) {
+        case VMDK3_MAGIC:
+            return vmdk_open_vmdk3(bs, file, flags);
+            break;
+        case VMDK4_MAGIC:
+            return vmdk_open_vmdk4(bs, file, flags);
+            break;
+        default:
+            return -EINVAL;
+            break;
+    }
+}
+
 static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         const char *desc_file_path)
 {
@@ -470,6 +493,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     const char *p = desc;
     int64_t sectors = 0;
     int64_t flat_offset;
+    char extent_path[PATH_MAX];
+    BlockDriverState *extent_file;
 
     while (*p) {
         /* parse extent line:
@@ -504,24 +529,29 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             goto next_line;
         }
 
+        path_combine(extent_path, sizeof(extent_path),
+                desc_file_path, fname);
+        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
+        if (ret) {
+            return ret;
+        }
+
         /* save to extents array */
         if (!strcmp(type, "FLAT")) {
             /* FLAT extent */
-            char extent_path[PATH_MAX];
-            BlockDriverState *extent_file;
             VmdkExtent *extent;
 
-            path_combine(extent_path, sizeof(extent_path),
-                    desc_file_path, fname);
-            ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
-            if (ret) {
-                return ret;
-            }
             extent = vmdk_add_extent(bs, extent_file, true, sectors,
                             0, 0, 0, 0, sectors);
             extent->flat_start_offset = flat_offset;
+        } else if (!strcmp(type, "SPARSE")) {
+            /* SPARSE extent */
+            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
+            if (ret) {
+                bdrv_delete(extent_file);
+                return ret;
+            }
         } else {
-            /* SPARSE extent, not supported for now */
             fprintf(stderr,
                 "VMDK: Not supported extent type \"%s\""".\n", type);
             return -ENOTSUP;
@@ -552,6 +582,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
         return -EINVAL;
     }
     if (strcmp(ct, "monolithicFlat") &&
+        strcmp(ct, "twoGbMaxExtentSparse") &&
         strcmp(ct, "twoGbMaxExtentFlat")) {
         fprintf(stderr,
                 "VMDK: Not supported image type \"%s\""".\n", ct);
@@ -574,17 +605,18 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
 
 static int vmdk_open(BlockDriverState *bs, int flags)
 {
-    uint32_t magic;
-
-    if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
-        return -EIO;
-    }
+    int ret;
+    BDRVVmdkState *s = bs->opaque;
 
-    magic = be32_to_cpu(magic);
-    if (magic == VMDK3_MAGIC) {
-        return vmdk_open_vmdk3(bs, flags);
-    } else if (magic == VMDK4_MAGIC) {
-        return vmdk_open_vmdk4(bs, flags);
+    if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
+        s->desc_offset = 0x200;
+        /* try to open parent images, if exist */
+        ret = vmdk_parent_open(bs);
+        if (ret) {
+            return ret;
+        }
+        s->parent_cid = vmdk_read_cid(bs, 1);
+        return 0;
     } else {
         return vmdk_open_desc_file(bs, flags);
     }

Re: [Qemu-devel] [PATCH v2 2/6] VMDK: add twoGbMaxExtentSparse support

[Stefan Hajnoczi]

On Thu, Aug 4, 2011 at 4:09 AM, Fam Zheng <famcool@gmail.com> wrote:
> +static void vmdk_free_last_extent(BlockDriverState *bs)
> +{
> +    BDRVVmdkState *s = bs->opaque;
> +
> +    if (s->num_extents == 0) {
> +        return;
> +    }
> +    s->num_extents--;
> +    s->extents = qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));

vmdk_free_extents() frees extent->l1_table, extent->l2_cache, and
extent->l1_backup_table.  Are they being leaked here?

Stefan

Re: [Qemu-devel] [PATCH 2/6] VMDK: add twoGbMaxExtentSparse support

[Stefan Hajnoczi]

On Thu, Aug 4, 2011 at 3:35 AM, Fam Zheng <famcool@gmail.com> wrote:
> On Tue, Aug 2, 2011 at 6:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <famcool@gmail.com> wrote:
>>> Add twoGbMaxExtentSparse support. Only opening code is changed.
>>>
>>> Signed-off-by: Fam Zheng <famcool@gmail.com>
>>> ---
>>>  block/vmdk.c |  124 ++++++++++++++++++++++++++++++++++++----------------------
>>>  1 files changed, 77 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/block/vmdk.c b/block/vmdk.c
>>> index 9e6c67a..9d1ae32 100644
>>> --- a/block/vmdk.c
>>> +++ b/block/vmdk.c
>>> @@ -357,7 +357,9 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
>>>     return ret;
>>>  }
>>>
>>> -static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>>> +static int vmdk_open_vmdk3(BlockDriverState *bs,
>>> +                           BlockDriverState *file,
>>> +                           int flags)
>>>  {
>>>     int ret;
>>>     uint32_t magic;
>>> @@ -365,10 +367,9 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>>>     BDRVVmdkState *s = bs->opaque;
>>>     VmdkExtent *extent;
>>>
>>> -    s->desc_offset = 0x200;
>>> -    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
>>> +    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>>>     if (ret < 0) {
>>> -        goto fail;
>>> +        return ret;
>>>     }
>>>     extent = vmdk_add_extent(bs,
>>>                              bs->file, false,
>>> @@ -378,17 +379,16 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
>>>                              le32_to_cpu(header.granularity));
>>>     ret = vmdk_init_tables(bs, extent);
>>>     if (ret) {
>>> -        /* vmdk_init_tables cleans up on fail, so only free allocation of
>>> -         * vmdk_add_extent here. */
>>> -        goto fail;
>>> +        /* free extent allocated by vmdk_add_extent */
>>> +        s->num_extents--;
>>> +        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
>>
>> The return value of qemu_realloc() is the new address of s->extents[].
>>  You are ignoring the new address and continue to use the old one.
>>
>> Please add a vmdk_free_last_extent() function that encapsulates this.
>> The rest of the vmdk.c code should not manipulate
>> s->extents[]/s->num_extents.  Use vmdk_add_extent(),
>> vmdk_free_last_extent(), vmdk_free_extents() instead of spreading the
>> extent management code in various places.
>>
>>>     }
>>> -    return 0;
>>> - fail:
>>> -    vmdk_free_extents(bs);
>>>     return ret;
>>>  }
>>>
>>> -static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
>>> +static int vmdk_open_vmdk4(BlockDriverState *bs,
>>> +                           BlockDriverState *file,
>>> +                           int flags)
>>>  {
>>>     int ret;
>>>     uint32_t magic;
>>> @@ -397,39 +397,30 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
>>>     BDRVVmdkState *s = bs->opaque;
>>>     VmdkExtent *extent;
>>>
>>> -    s->desc_offset = 0x200;
>>> -    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
>>> +    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>>>     if (ret < 0) {
>>> -        goto fail;
>>> +        return ret;
>>>     }
>>>     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
>>>                         * le64_to_cpu(header.granularity);
>>> +    if (l1_entry_sectors <= 0) {
>>> +        return -EINVAL;
>>> +    }
>>>     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
>>>                 / l1_entry_sectors;
>>> -    extent = vmdk_add_extent(bs, bs->file, false,
>>> +    extent = vmdk_add_extent(bs, file, false,
>>>                           le64_to_cpu(header.capacity),
>>>                           le64_to_cpu(header.gd_offset) << 9,
>>>                           le64_to_cpu(header.rgd_offset) << 9,
>>>                           l1_size,
>>>                           le32_to_cpu(header.num_gtes_per_gte),
>>>                           le64_to_cpu(header.granularity));
>>> -    if (extent->l1_entry_sectors <= 0) {
>>> -        ret = -EINVAL;
>>> -        goto fail;
>>> -    }
>>> -    /* try to open parent images, if exist */
>>> -    ret = vmdk_parent_open(bs);
>>> -    if (ret) {
>>> -        goto fail;
>>> -    }
>>> -    s->parent_cid = vmdk_read_cid(bs, 1);
>>>     ret = vmdk_init_tables(bs, extent);
>>>     if (ret) {
>>> -        goto fail;
>>> +        /* free extent allocated by vmdk_add_extent */
>>> +        s->num_extents--;
>>> +        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
>>
>> Same bug.
>>
>>>     }
>>> -    return 0;
>>> - fail:
>>> -    vmdk_free_extents(bs);
>>>     return ret;
>>>  }
>>>
>>> @@ -460,6 +451,35 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
>>>     return 0;
>>>  }
>>>
>>> +/* Open an extent file and append to bs array */
>>> +static int vmdk_open_sparse(BlockDriverState *bs,
>>> +                            BlockDriverState *file,
>>> +                            int flags)
>>> +{
>>> +    int ret;
>>> +    uint32_t magic;
>>> +
>>> +    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    magic = be32_to_cpu(magic);
>>> +    if (magic == VMDK3_MAGIC) {
>>> +        ret = vmdk_open_vmdk3(bs, file, flags);
>>> +        if (ret) {
>>> +            return ret;
>>> +        }
>>> +    } else if (magic == VMDK4_MAGIC) {
>>> +        ret = vmdk_open_vmdk4(bs, file, flags);
>>> +        if (ret) {
>>> +            return ret;
>>> +        }
>>> +    } else {
>>> +        return -EINVAL;
>>> +    }
>>> +    return 0;
>>
>> Shorter way of writing this (ret is no longer necessary):
>>
>> switch (magic) {
>> case VMDK3_MAGIC:
>>    return vmdk_open_vmdk3(bs, file, flags);
>> case VMDK4_MAGIC:
>>    return vmdk_open_vmdk4(bs, file, flags);
>> default:
>>    return -EINVAL;
>> }
>>
>>> +}
>>> +
>>>  static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>>         const char *desc_file_path)
>>>  {
>>> @@ -470,6 +490,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>>     const char *p = desc;
>>>     int64_t sectors = 0;
>>>     int64_t flat_offset;
>>> +    char extent_path[PATH_MAX];
>>> +    BlockDriverState *extent_file;
>>>
>>>     while (*p) {
>>>         /* parse extent line:
>>> @@ -504,22 +526,28 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>>             goto next_line;
>>>         }
>>>
>>> +        path_combine(extent_path, sizeof(extent_path),
>>> +                desc_file_path, fname);
>>> +        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
>>> +        if (ret) {
>>> +            return ret;
>>> +        }
>>> +
>>>         /* save to extents array */
>>>         if (!strcmp(type, "FLAT")) {
>>>             /* FLAT extent */
>>> -            char extent_path[PATH_MAX];
>>> -            BlockDriverState *extent_file;
>>>             VmdkExtent *extent;
>>>
>>> -            path_combine(extent_path, sizeof(extent_path),
>>> -                    desc_file_path, fname);
>>> -            ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
>>> -            if (ret) {
>>> -                return ret;
>>> -            }
>>>             extent = vmdk_add_extent(bs, extent_file, true, sectors,
>>>                             0, 0, 0, 0, sectors);
>>>             extent->flat_start_offset = flat_offset;
>>> +        } else if (!strcmp(type, "SPARSE")) {
>>> +            /* SPARSE extent */
>>> +            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
>>> +            if (ret) {
>>> +                bdrv_delete(extent_file);
>>> +                return ret;
>>> +            }
>>>         } else {
>>>             /* SPARSE extent, not supported for now */
>>
>> This comment is outdated.
>>
>>>             fprintf(stderr,
>>> @@ -552,6 +580,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
>>>         return -EINVAL;
>>>     }
>>>     if (strcmp(ct, "monolithicFlat") &&
>>> +        strcmp(ct, "twoGbMaxExtentSparse") &&
>>>         strcmp(ct, "twoGbMaxExtentFlat")) {
>>>         fprintf(stderr,
>>>                 "VMDK: Not supported image type \"%s\""".\n", ct);
>>> @@ -574,17 +603,18 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
>>>
>>>  static int vmdk_open(BlockDriverState *bs, int flags)
>>>  {
>>> -    uint32_t magic;
>>> -
>>> -    if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
>>> -        return -EIO;
>>> -    }
>>> +    int ret;
>>> +    BDRVVmdkState *s = bs->opaque;
>>>
>>> -    magic = be32_to_cpu(magic);
>>> -    if (magic == VMDK3_MAGIC) {
>>> -        return vmdk_open_vmdk3(bs, flags);
>>> -    } else if (magic == VMDK4_MAGIC) {
>>> -        return vmdk_open_vmdk4(bs, flags);
>>> +    if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
>>> +        s->desc_offset = 0x200;
>>> +        /* try to open parent images, if exist */
>>> +        ret = vmdk_parent_open(bs);
>>> +        if (ret) {
>>> +            return ret;
>>
>> What needs to be freed here?
>>
>
> No resource allocation in vmdk_parent_open, so I think none.

But what about vmdk_open_sparse(), which succeeded?  This function is
a .bdrv_open().  If you return an error then bs->file will be deleted
and bs->opaque is freed - but you need to clean up all image
format-specific resources *before* returning error.  bdrv_close() will
not be called if bdrv_open() returns error.

Stefan

Re: [Qemu-devel] [PATCH v2 2/6] VMDK: add twoGbMaxExtentSparse support

[Fam Zheng]

On Thu, Aug 4, 2011 at 6:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Aug 4, 2011 at 4:09 AM, Fam Zheng <famcool@gmail.com> wrote:
>> +static void vmdk_free_last_extent(BlockDriverState *bs)
>> +{
>> +    BDRVVmdkState *s = bs->opaque;
>> +
>> +    if (s->num_extents == 0) {
>> +        return;
>> +    }
>> +    s->num_extents--;
>> +    s->extents = qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
>
> vmdk_free_extents() frees extent->l1_table, extent->l2_cache, and
> extent->l1_backup_table.  Are they being leaked here?

No, it's only called after vmdk_init_tables fails, where no table is
actually allocated to the extent.


-- 
Best regards!
Fam Zheng

Re: [Qemu-devel] [PATCH v2 2/6] VMDK: add twoGbMaxExtentSparse support

[Stefan Hajnoczi]

On Thu, Aug 4, 2011 at 11:38 AM, Fam Zheng <famcool@gmail.com> wrote:
> On Thu, Aug 4, 2011 at 6:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Thu, Aug 4, 2011 at 4:09 AM, Fam Zheng <famcool@gmail.com> wrote:
>>> +static void vmdk_free_last_extent(BlockDriverState *bs)
>>> +{
>>> +    BDRVVmdkState *s = bs->opaque;
>>> +
>>> +    if (s->num_extents == 0) {
>>> +        return;
>>> +    }
>>> +    s->num_extents--;
>>> +    s->extents = qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
>>
>> vmdk_free_extents() frees extent->l1_table, extent->l2_cache, and
>> extent->l1_backup_table.  Are they being leaked here?
>
> No, it's only called after vmdk_init_tables fails, where no table is
> actually allocated to the extent.

Okay, thanks for explaining.

Stefan

[Qemu-devel] [PATCH v2 2/6] VMDK: add twoGbMaxExtentSparse support

[Fam Zheng]

Add twoGbMaxExtentSparse support. Introduce vmdk_free_last_extent.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |  133 ++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 83 insertions(+), 50 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index e22a893..ab15840 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -174,6 +174,17 @@ static void vmdk_free_extents(BlockDriverState *bs)
     qemu_free(s->extents);
 }
 
+static void vmdk_free_last_extent(BlockDriverState *bs)
+{
+    BDRVVmdkState *s = bs->opaque;
+
+    if (s->num_extents == 0) {
+        return;
+    }
+    s->num_extents--;
+    s->extents = qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
+}
+
 static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
 {
     char desc[DESC_SIZE];
@@ -357,18 +368,18 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
     return ret;
 }
 
-static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
+static int vmdk_open_vmdk3(BlockDriverState *bs,
+                           BlockDriverState *file,
+                           int flags)
 {
     int ret;
     uint32_t magic;
     VMDK3Header header;
-    BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
-    s->desc_offset = 0x200;
-    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
+    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
-        goto fail;
+        return ret;
     }
     extent = vmdk_add_extent(bs,
                              bs->file, false,
@@ -378,58 +389,45 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
                              le32_to_cpu(header.granularity));
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
-        /* vmdk_init_tables cleans up on fail, so only free allocation of
-         * vmdk_add_extent here. */
-        goto fail;
+        /* free extent allocated by vmdk_add_extent */
+        vmdk_free_last_extent(bs);
     }
-    return 0;
- fail:
-    vmdk_free_extents(bs);
     return ret;
 }
 
-static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
+static int vmdk_open_vmdk4(BlockDriverState *bs,
+                           BlockDriverState *file,
+                           int flags)
 {
     int ret;
     uint32_t magic;
     uint32_t l1_size, l1_entry_sectors;
     VMDK4Header header;
-    BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
-    s->desc_offset = 0x200;
-    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
+    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
-        goto fail;
+        return ret;
     }
     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
                         * le64_to_cpu(header.granularity);
+    if (l1_entry_sectors <= 0) {
+        return -EINVAL;
+    }
     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
                 / l1_entry_sectors;
-    extent = vmdk_add_extent(bs, bs->file, false,
+    extent = vmdk_add_extent(bs, file, false,
                           le64_to_cpu(header.capacity),
                           le64_to_cpu(header.gd_offset) << 9,
                           le64_to_cpu(header.rgd_offset) << 9,
                           l1_size,
                           le32_to_cpu(header.num_gtes_per_gte),
                           le64_to_cpu(header.granularity));
-    if (extent->l1_entry_sectors <= 0) {
-        ret = -EINVAL;
-        goto fail;
-    }
-    /* try to open parent images, if exist */
-    ret = vmdk_parent_open(bs);
-    if (ret) {
-        goto fail;
-    }
-    s->parent_cid = vmdk_read_cid(bs, 1);
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
-        goto fail;
+        /* free extent allocated by vmdk_add_extent */
+        vmdk_free_last_extent(bs);
     }
-    return 0;
- fail:
-    vmdk_free_extents(bs);
     return ret;
 }
 
@@ -460,6 +458,31 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
     return 0;
 }
 
+/* Open an extent file and append to bs array */
+static int vmdk_open_sparse(BlockDriverState *bs,
+                            BlockDriverState *file,
+                            int flags)
+{
+    uint32_t magic;
+
+    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
+        return -EIO;
+    }
+
+    magic = be32_to_cpu(magic);
+    switch (magic) {
+        case VMDK3_MAGIC:
+            return vmdk_open_vmdk3(bs, file, flags);
+            break;
+        case VMDK4_MAGIC:
+            return vmdk_open_vmdk4(bs, file, flags);
+            break;
+        default:
+            return -EINVAL;
+            break;
+    }
+}
+
 static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         const char *desc_file_path)
 {
@@ -470,6 +493,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     const char *p = desc;
     int64_t sectors = 0;
     int64_t flat_offset;
+    char extent_path[PATH_MAX];
+    BlockDriverState *extent_file;
 
     while (*p) {
         /* parse extent line:
@@ -504,24 +529,29 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             goto next_line;
         }
 
+        path_combine(extent_path, sizeof(extent_path),
+                desc_file_path, fname);
+        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
+        if (ret) {
+            return ret;
+        }
+
         /* save to extents array */
         if (!strcmp(type, "FLAT")) {
             /* FLAT extent */
-            char extent_path[PATH_MAX];
-            BlockDriverState *extent_file;
             VmdkExtent *extent;
 
-            path_combine(extent_path, sizeof(extent_path),
-                    desc_file_path, fname);
-            ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
-            if (ret) {
-                return ret;
-            }
             extent = vmdk_add_extent(bs, extent_file, true, sectors,
                             0, 0, 0, 0, sectors);
             extent->flat_start_offset = flat_offset;
+        } else if (!strcmp(type, "SPARSE")) {
+            /* SPARSE extent */
+            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
+            if (ret) {
+                bdrv_delete(extent_file);
+                return ret;
+            }
         } else {
-            /* SPARSE extent, not supported for now */
             fprintf(stderr,
                 "VMDK: Not supported extent type \"%s\""".\n", type);
             return -ENOTSUP;
@@ -552,6 +582,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
         return -EINVAL;
     }
     if (strcmp(ct, "monolithicFlat") &&
+        strcmp(ct, "twoGbMaxExtentSparse") &&
         strcmp(ct, "twoGbMaxExtentFlat")) {
         fprintf(stderr,
                 "VMDK: Not supported image type \"%s\""".\n", ct);
@@ -574,17 +605,19 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
 
 static int vmdk_open(BlockDriverState *bs, int flags)
 {
-    uint32_t magic;
-
-    if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
-        return -EIO;
-    }
+    int ret;
+    BDRVVmdkState *s = bs->opaque;
 
-    magic = be32_to_cpu(magic);
-    if (magic == VMDK3_MAGIC) {
-        return vmdk_open_vmdk3(bs, flags);
-    } else if (magic == VMDK4_MAGIC) {
-        return vmdk_open_vmdk4(bs, flags);
+    if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
+        s->desc_offset = 0x200;
+        /* try to open parent images, if exist */
+        ret = vmdk_parent_open(bs);
+        if (ret) {
+            vmdk_free_extents(bs);
+            return ret;
+        }
+        s->parent_cid = vmdk_read_cid(bs, 1);
+        return 0;
     } else {
         return vmdk_open_desc_file(bs, flags);
     }