[Qemu-devel] [PATCH v4 00/10] vmdk: Input validation fixes

[Qemu-devel] [PATCH v4 00/10] vmdk: Input validation fixes

[Fam Zheng]

Fixes for VMDK sparse file opening and other bugs. Header fields are checked
before used for memory allocation.

v4:
    01: Added, fix variable scope bug of ret code.
    02: Squashed two QEMU_PACKED patches.
    03: Make BDRVVmdkState.desc_offset uint64_t as well.
    10: Comment num_gtes_per_gte.
v3:
    00: Rebase to master.
    04: Drop unused line in test script.
    05: change vmdk_add_extent signature (uint64_t cluster_sectors).
    06: Fix num_gtes_per_gte.
    08: Use qemu_blockalign for whole_grain.
        Don't allocate memory for !bs->backing_hd case.


Fam Zheng (9):
  vmdk: remove ret from inner scope.
  vmdk: Make VMDK3Header and VmdkGrainMarker QEMU_PACKED
  vmdk: use unsigned values for on disk header fields
  qemu-iotests: add empty test case for vmdk
  vmdk: check granularity field in opening
  vmdk: check l2 table size when opening
  vmdk: check l1 size before opening image
  vmdk: use heap allocation for whole_grain
  vmdk: rename num_gtes_per_gte to num_gtes_per_gt

Stefan Hajnoczi (1):
  qemu-iotests: add poke_file utility function

 block/vmdk.c                 | 115 +++++++++++++++++++++++++++++--------------
 tests/qemu-iotests/059       |  72 +++++++++++++++++++++++++++
 tests/qemu-iotests/059.out   |  20 ++++++++
 tests/qemu-iotests/common.rc |   6 +++
 tests/qemu-iotests/group     |   1 +
 5 files changed, 177 insertions(+), 37 deletions(-)
 create mode 100755 tests/qemu-iotests/059
 create mode 100644 tests/qemu-iotests/059.out

-- 
1.8.3.4

[Qemu-devel] [PATCH v4 01/10] vmdk: remove ret from inner scope.

[Fam Zheng]

A inner scope "ret" variable hides the real return code, it will always
return VMDK_OK for bs->backing_hd. Fix this by removing the declaration.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 3756333..f42657b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -813,8 +813,8 @@ static int get_whole_cluster(BlockDriverState *bs,
     /* we will be here if it's first write on non-exist grain(cluster).
      * try to read from parent image, if exist */
     if (bs->backing_hd) {
-        int ret;
-
+        whole_grain =
+            qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
         if (!vmdk_is_cid_valid(bs)) {
             return VMDK_ERROR;
         }
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 02/10] vmdk: Make VMDK3Header and VmdkGrainMarker QEMU_PACKED

[Fam Zheng]

It's best to make it consistent that all on disk structures are
QEMU_PACKED.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index f42657b..8b86fe9 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -62,7 +62,7 @@ typedef struct {
     uint32_t cylinders;
     uint32_t heads;
     uint32_t sectors_per_track;
-} VMDK3Header;
+} QEMU_PACKED VMDK3Header;
 
 typedef struct {
     uint32_t version;
@@ -131,7 +131,7 @@ typedef struct VmdkGrainMarker {
     uint64_t lba;
     uint32_t size;
     uint8_t  data[0];
-} VmdkGrainMarker;
+} QEMU_PACKED VmdkGrainMarker;
 
 enum {
     MARKER_END_OF_STREAM    = 0,
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 03/10] vmdk: use unsigned values for on disk header fields

[Fam Zheng]

The size and offset fields are all non-negative values, use uint64_t for
them to avoid getting negative in memory value by int overflow.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 8b86fe9..5d11a43 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -67,14 +67,14 @@ typedef struct {
 typedef struct {
     uint32_t version;
     uint32_t flags;
-    int64_t capacity;
-    int64_t granularity;
-    int64_t desc_offset;
-    int64_t desc_size;
-    int32_t num_gtes_per_gte;
-    int64_t rgd_offset;
-    int64_t gd_offset;
-    int64_t grain_offset;
+    uint64_t capacity;
+    uint64_t granularity;
+    uint64_t desc_offset;
+    uint64_t desc_size;
+    uint32_t num_gtes_per_gte;
+    uint64_t rgd_offset;
+    uint64_t gd_offset;
+    uint64_t grain_offset;
     char filler[1];
     char check_bytes[4];
     uint16_t compressAlgorithm;
@@ -109,7 +109,7 @@ typedef struct VmdkExtent {
 
 typedef struct BDRVVmdkState {
     CoMutex lock;
-    int desc_offset;
+    uint64_t desc_offset;
     bool cid_updated;
     uint32_t parent_cid;
     int num_extents;
@@ -490,7 +490,7 @@ static int vmdk_open_vmdk3(BlockDriverState *bs,
 }
 
 static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
-                               int64_t desc_offset);
+                               uint64_t desc_offset);
 
 static int vmdk_open_vmdk4(BlockDriverState *bs,
                            BlockDriverState *file,
@@ -508,7 +508,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         return ret;
     }
     if (header.capacity == 0) {
-        int64_t desc_offset = le64_to_cpu(header.desc_offset);
+        uint64_t desc_offset = le64_to_cpu(header.desc_offset);
         if (desc_offset) {
             return vmdk_open_desc_file(bs, flags, desc_offset << 9);
         }
@@ -728,7 +728,7 @@ next_line:
 }
 
 static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
-                               int64_t desc_offset)
+                               uint64_t desc_offset)
 {
     int ret;
     char *buf = NULL;
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 04/10] qemu-iotests: add poke_file utility function

[Fam Zheng]

From: Stefan Hajnoczi <stefanha@redhat.com>

The new poke_file function sets bytes at an offset in a file given a
printf-style format string.  It can be used to corrupt an image file for
test coverage of error paths.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/common.rc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index e9ba358..5e077c3 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -34,6 +34,12 @@ dd()
    fi
 }
 
+# poke_file 'test.img' 512 '\xff\xfe'
+poke_file()
+{
+    printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
+}
+
 # we need common.config
 if [ "$iam" != "check" ]
 then
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 05/10] qemu-iotests: add empty test case for vmdk

[Fam Zheng]

Will add vmdk specific tests later here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/059     | 51 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/059.out |  2 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 54 insertions(+)
 create mode 100755 tests/qemu-iotests/059
 create mode 100644 tests/qemu-iotests/059.out

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
new file mode 100755
index 0000000..9dc7f64
--- /dev/null
+++ b/tests/qemu-iotests/059
@@ -0,0 +1,51 @@
+#!/bin/bash
+#
+# Test case for vmdk
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=famz@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests vmdk-specific low-level functionality
+_supported_fmt vmdk
+_supported_proto generic
+_supported_os Linux
+
+granularity_offset=16
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
new file mode 100644
index 0000000..4ca7f29
--- /dev/null
+++ b/tests/qemu-iotests/059.out
@@ -0,0 +1,2 @@
+QA output created by 059
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b1d03c7..93ace2e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -63,3 +63,4 @@
 054 rw auto
 055 rw auto
 056 rw auto backing
+059 rw auto
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 06/10] vmdk: check granularity field in opening

[Fam Zheng]

Granularity is used to calculate the cluster size and allocate r/w
buffer. Check the value from image before using it, so we don't abort()
for unbounded memory allocation.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c               | 40 +++++++++++++++++++++++++++++++---------
 tests/qemu-iotests/059     |  8 +++++++-
 tests/qemu-iotests/059.out |  6 ++++++
 3 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 5d11a43..1479288 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -385,15 +385,22 @@ static int vmdk_parent_open(BlockDriverState *bs)
 
 /* Create and append extent to the extent array. Return the added VmdkExtent
  * address. return NULL if allocation failed. */
-static VmdkExtent *vmdk_add_extent(BlockDriverState *bs,
+static int vmdk_add_extent(BlockDriverState *bs,
                            BlockDriverState *file, bool flat, int64_t sectors,
                            int64_t l1_offset, int64_t l1_backup_offset,
                            uint32_t l1_size,
-                           int l2_size, unsigned int cluster_sectors)
+                           int l2_size, uint64_t cluster_sectors,
+                           VmdkExtent **new_extent)
 {
     VmdkExtent *extent;
     BDRVVmdkState *s = bs->opaque;
 
+    if (cluster_sectors > 0x200000) {
+        /* 0x200000 * 512Bytes = 1GB for one cluster is unrealistic */
+        error_report("invalid granularity, image may be corrupt");
+        return -EINVAL;
+    }
+
     s->extents = g_realloc(s->extents,
                               (s->num_extents + 1) * sizeof(VmdkExtent));
     extent = &s->extents[s->num_extents];
@@ -416,7 +423,10 @@ static VmdkExtent *vmdk_add_extent(BlockDriverState *bs,
         extent->end_sector = extent->sectors;
     }
     bs->total_sectors = extent->end_sector;
-    return extent;
+    if (new_extent) {
+        *new_extent = extent;
+    }
+    return 0;
 }
 
 static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
@@ -475,12 +485,17 @@ static int vmdk_open_vmdk3(BlockDriverState *bs,
     if (ret < 0) {
         return ret;
     }
-    extent = vmdk_add_extent(bs,
+
+    ret = vmdk_add_extent(bs,
                              bs->file, false,
                              le32_to_cpu(header.disk_sectors),
                              le32_to_cpu(header.l1dir_offset) << 9,
                              0, 1 << 6, 1 << 9,
-                             le32_to_cpu(header.granularity));
+                             le32_to_cpu(header.granularity),
+                             &extent);
+    if (ret < 0) {
+        return ret;
+    }
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
         /* free extent allocated by vmdk_add_extent */
@@ -580,13 +595,17 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     if (le32_to_cpu(header.flags) & VMDK4_FLAG_RGD) {
         l1_backup_offset = le64_to_cpu(header.rgd_offset) << 9;
     }
-    extent = vmdk_add_extent(bs, file, false,
+    ret = vmdk_add_extent(bs, file, false,
                           le64_to_cpu(header.capacity),
                           le64_to_cpu(header.gd_offset) << 9,
                           l1_backup_offset,
                           l1_size,
                           le32_to_cpu(header.num_gtes_per_gte),
-                          le64_to_cpu(header.granularity));
+                          le64_to_cpu(header.granularity),
+                          &extent);
+    if (ret < 0) {
+        return ret;
+    }
     extent->compressed =
         le16_to_cpu(header.compressAlgorithm) == VMDK4_COMPRESSION_DEFLATE;
     extent->has_marker = le32_to_cpu(header.flags) & VMDK4_FLAG_MARKER;
@@ -702,8 +721,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             /* FLAT extent */
             VmdkExtent *extent;
 
-            extent = vmdk_add_extent(bs, extent_file, true, sectors,
-                            0, 0, 0, 0, sectors);
+            ret = vmdk_add_extent(bs, extent_file, true, sectors,
+                            0, 0, 0, 0, sectors, &extent);
+            if (ret < 0) {
+                return ret;
+            }
             extent->flat_start_offset = flat_offset << 9;
         } else if (!strcmp(type, "SPARSE")) {
             /* SPARSE extent */
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 9dc7f64..9545e82 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -43,7 +43,13 @@ _supported_fmt vmdk
 _supported_proto generic
 _supported_os Linux
 
-granularity_offset=16
+granularity_offset=20
+
+echo "=== Testing invalid granularity ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$granularity_offset" "\xff\xff\xff\xff\xff\xff\xff\xff"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 4ca7f29..380ca3d 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -1,2 +1,8 @@
 QA output created by 059
+=== Testing invalid granularity ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+invalid granularity, image may be corrupt
+qemu-io: can't open device TEST_DIR/t.vmdk
+no file open, try 'help open'
 *** done
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 07/10] vmdk: check l2 table size when opening

[Fam Zheng]

header.num_gtes_per_gte determines size for L2 table. Check for too big
value before using it. Limit to 512M entries (2GB per one L2 table).

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c               | 5 +++++
 tests/qemu-iotests/059     | 7 +++++++
 tests/qemu-iotests/059.out | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index 1479288..53f473b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -585,6 +585,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         return -ENOTSUP;
     }
 
+    if (le32_to_cpu(header.num_gtes_per_gte) > 512) {
+        error_report("L2 table size too big");
+        return -EINVAL;
+    }
+
     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
                         * le64_to_cpu(header.granularity);
     if (l1_entry_sectors == 0) {
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 9545e82..301eaca 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -44,6 +44,7 @@ _supported_proto generic
 _supported_os Linux
 
 granularity_offset=20
+grain_table_size_offset=44
 
 echo "=== Testing invalid granularity ==="
 echo
@@ -51,6 +52,12 @@ _make_test_img 64M
 poke_file "$TEST_IMG" "$granularity_offset" "\xff\xff\xff\xff\xff\xff\xff\xff"
 { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo "=== Testing too big L2 table size ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$grain_table_size_offset" "\xff\xff\xff\xff"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 380ca3d..583955f 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -5,4 +5,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 invalid granularity, image may be corrupt
 qemu-io: can't open device TEST_DIR/t.vmdk
 no file open, try 'help open'
+=== Testing too big L2 table size ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+L2 table size too big
+qemu-io: can't open device TEST_DIR/t.vmdk
+no file open, try 'help open'
 *** done
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 08/10] vmdk: check l1 size before opening image

[Fam Zheng]

L1 table size is calculated from capacity, granularity and l2 table
size. If capacity is too big or later two are too small, the L1 table
will be too big to allocate in memory. Limit it to a reasonable range.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c               | 8 ++++++++
 tests/qemu-iotests/059     | 8 ++++++++
 tests/qemu-iotests/059.out | 6 ++++++
 3 files changed, 22 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index 53f473b..53e1f91 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -597,6 +597,14 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     }
     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
                 / l1_entry_sectors;
+    if (l1_size > 512 * 1024 * 1024) {
+        /* although with big capacity and small l1_entry_sectors, we can get a
+         * big l1_size, we don't want unbounded value to allocate the table.
+         * Limit it to 512M, which is 16PB for default cluster and L2 table
+         * size */
+        error_report("L1 size too big");
+        return -EFBIG;
+    }
     if (le32_to_cpu(header.flags) & VMDK4_FLAG_RGD) {
         l1_backup_offset = le64_to_cpu(header.rgd_offset) << 9;
     }
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 301eaca..b03429d 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -43,6 +43,7 @@ _supported_fmt vmdk
 _supported_proto generic
 _supported_os Linux
 
+capacity_offset=16
 granularity_offset=20
 grain_table_size_offset=44
 
@@ -58,6 +59,13 @@ _make_test_img 64M
 poke_file "$TEST_IMG" "$grain_table_size_offset" "\xff\xff\xff\xff"
 { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo "=== Testing too big L1 table size ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$capacity_offset" "\xff\xff\xff\xff"
+poke_file "$TEST_IMG" "$grain_table_size_offset" "\x01\x00\x00\x00"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 583955f..9e715e5 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -11,4 +11,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 L2 table size too big
 qemu-io: can't open device TEST_DIR/t.vmdk
 no file open, try 'help open'
+=== Testing too big L1 table size ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+L1 size too big
+qemu-io: can't open device TEST_DIR/t.vmdk
+no file open, try 'help open'
 *** done
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 09/10] vmdk: use heap allocation for whole_grain

[Fam Zheng]

We should never grow the stack beyond 1 MB, otherwise we'll fall off the
end.  Thread stacks and coroutine stacks (1 MB) do not grow.
get_cluster_offset() allocates a big stack offset, it will fail for big
cluster images, change to heap allocated buffer.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 53e1f91..39f5a8b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -842,8 +842,8 @@ static int get_whole_cluster(BlockDriverState *bs,
                 uint64_t offset,
                 bool allocate)
 {
-    /* 128 sectors * 512 bytes each = grain size 64KB */
-    uint8_t  whole_grain[extent->cluster_sectors * 512];
+    int ret = VMDK_OK;
+    uint8_t *whole_grain = NULL;
 
     /* we will be here if it's first write on non-exist grain(cluster).
      * try to read from parent image, if exist */
@@ -851,7 +851,8 @@ static int get_whole_cluster(BlockDriverState *bs,
         whole_grain =
             qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
         if (!vmdk_is_cid_valid(bs)) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
 
         /* floor offset to cluster */
@@ -859,17 +860,21 @@ static int get_whole_cluster(BlockDriverState *bs,
         ret = bdrv_read(bs->backing_hd, offset >> 9, whole_grain,
                 extent->cluster_sectors);
         if (ret < 0) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
 
         /* Write grain only into the active image */
         ret = bdrv_write(extent->file, cluster_offset, whole_grain,
                 extent->cluster_sectors);
         if (ret < 0) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
     }
-    return VMDK_OK;
+exit:
+    qemu_vfree(whole_grain);
+    return ret;
 }
 
 static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
-- 
1.8.3.4

[Qemu-devel] [PATCH v4 10/10] vmdk: rename num_gtes_per_gte to num_gtes_per_gt

[Fam Zheng]

num_gtes_per_gte is a historical typo, rename it to a more sensible
name. It means "number of GrainTableEntries per GrainTable".

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 39f5a8b..d05ff17 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -71,7 +71,8 @@ typedef struct {
     uint64_t granularity;
     uint64_t desc_offset;
     uint64_t desc_size;
-    uint32_t num_gtes_per_gte;
+    /* Number of GrainTableEntries per GrainTable */
+    uint32_t num_gtes_per_gt;
     uint64_t rgd_offset;
     uint64_t gd_offset;
     uint64_t grain_offset;
@@ -585,12 +586,12 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         return -ENOTSUP;
     }
 
-    if (le32_to_cpu(header.num_gtes_per_gte) > 512) {
+    if (le32_to_cpu(header.num_gtes_per_gt) > 512) {
         error_report("L2 table size too big");
         return -EINVAL;
     }
 
-    l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
+    l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gt)
                         * le64_to_cpu(header.granularity);
     if (l1_entry_sectors == 0) {
         return -EINVAL;
@@ -613,7 +614,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
                           le64_to_cpu(header.gd_offset) << 9,
                           l1_backup_offset,
                           l1_size,
-                          le32_to_cpu(header.num_gtes_per_gte),
+                          le32_to_cpu(header.num_gtes_per_gt),
                           le64_to_cpu(header.granularity),
                           &extent);
     if (ret < 0) {
@@ -1407,12 +1408,12 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
     header.compressAlgorithm = compress ? VMDK4_COMPRESSION_DEFLATE : 0;
     header.capacity = filesize / 512;
     header.granularity = 128;
-    header.num_gtes_per_gte = 512;
+    header.num_gtes_per_gt = 512;
 
     grains = (filesize / 512 + header.granularity - 1) / header.granularity;
-    gt_size = ((header.num_gtes_per_gte * sizeof(uint32_t)) + 511) >> 9;
+    gt_size = ((header.num_gtes_per_gt * sizeof(uint32_t)) + 511) >> 9;
     gt_count =
-        (grains + header.num_gtes_per_gte - 1) / header.num_gtes_per_gte;
+        (grains + header.num_gtes_per_gt - 1) / header.num_gtes_per_gt;
     gd_size = (gt_count * sizeof(uint32_t) + 511) >> 9;
 
     header.desc_offset = 1;
@@ -1428,7 +1429,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
     header.flags = cpu_to_le32(header.flags);
     header.capacity = cpu_to_le64(header.capacity);
     header.granularity = cpu_to_le64(header.granularity);
-    header.num_gtes_per_gte = cpu_to_le32(header.num_gtes_per_gte);
+    header.num_gtes_per_gt = cpu_to_le32(header.num_gtes_per_gt);
     header.desc_offset = cpu_to_le64(header.desc_offset);
     header.desc_size = cpu_to_le64(header.desc_size);
     header.rgd_offset = cpu_to_le64(header.rgd_offset);
-- 
1.8.3.4

Re: [Qemu-devel] [PATCH v4 01/10] vmdk: remove ret from inner scope.

[Kevin Wolf]

Am 06.08.2013 um 09:44 hat Fam Zheng geschrieben:
> A inner scope "ret" variable hides the real return code, it will always
> return VMDK_OK for bs->backing_hd. Fix this by removing the declaration.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

In the original code there is no problem.

> ---
>  block/vmdk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 3756333..f42657b 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -813,8 +813,8 @@ static int get_whole_cluster(BlockDriverState *bs,
>      /* we will be here if it's first write on non-exist grain(cluster).
>       * try to read from parent image, if exist */
>      if (bs->backing_hd) {
> -        int ret;
> -
> +        whole_grain =
> +            qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
>          if (!vmdk_is_cid_valid(bs)) {
>              return VMDK_ERROR;
>          }

After this patch, there are two:

block/vmdk.c: In function 'get_whole_cluster':
block/vmdk.c:816:21: error: incompatible types when assigning to type
'uint8_t[(sizetype)(extent->cluster_sectors * 512u)]' from type 'void *'
block/vmdk.c:824:9: error: 'ret' undeclared (first use in this function)

Kevin

Re: [Qemu-devel] [PATCH v4 01/10] vmdk: remove ret from inner scope.

[Jeff Cody]

On Tue, Aug 06, 2013 at 02:15:13PM +0200, Kevin Wolf wrote:
> Am 06.08.2013 um 09:44 hat Fam Zheng geschrieben:
> > A inner scope "ret" variable hides the real return code, it will always
> > return VMDK_OK for bs->backing_hd. Fix this by removing the declaration.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> In the original code there is no problem.
>

Right, this whole patch should be squashed into patch 9.  Looks like a
bad rebase of the patches.

> > ---
> >  block/vmdk.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 3756333..f42657b 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -813,8 +813,8 @@ static int get_whole_cluster(BlockDriverState *bs,
> >      /* we will be here if it's first write on non-exist grain(cluster).
> >       * try to read from parent image, if exist */
> >      if (bs->backing_hd) {
> > -        int ret;
> > -


> > +        whole_grain =
> > +            qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
             ^^^^^^^^^^^^^^^^^^
This hunk is from patch 9.

> >          if (!vmdk_is_cid_valid(bs)) {
> >              return VMDK_ERROR;
> >          }
> 
> After this patch, there are two:
> 
> block/vmdk.c: In function 'get_whole_cluster':
> block/vmdk.c:816:21: error: incompatible types when assigning to type
> 'uint8_t[(sizetype)(extent->cluster_sectors * 512u)]' from type 'void *'
> block/vmdk.c:824:9: error: 'ret' undeclared (first use in this function)
> 
> Kevin