vmdk: Implement blockdev-create

From: Fam Zheng <[email protected]>

This makes VMDK support blockdev-create. The implementation reuses the image creation code in vmdk_co_create_opts which now acceptes a callback pointer to "retrieve" BlockBackend pointers from the caller. This way we separate the logic between file/extent acquisition and initialization.

The QAPI command parameters are mostly the same as the old create_opts except the dropped legacy @compat6 switch, which is redundant with @hwversion.

Signed-off-by Fam Zheng <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json  |  70 +++++++
 qapi/qapi-schema.json |  16 +-
 block/vmdk.c          | 448 ++++++++++++++++++++++++++++++------------
 3 files changed, 400 insertions(+), 134 deletions(-)

qapi/block-core.json +38 -0

index d4fe710836..0793550cf2 100644
@@ -4021,6 +4021,75 @@
             'size':             'size',
             '*cluster-size' :   'size' } }
 
+##
+# @BlockdevVmdkSubformat:
+#
+# Subformat options for VMDK images
+#
+# @monolithicSparse:     Single file image with sparse cluster allocation
+#
+# @monolithicFlat:       Single flat data image and a descriptor file
+#
+# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent
+#                        files, in addition to a descriptor file
+#
+# @twoGbMaxExtentFlat:   Data is split into 2GB (per virtual LBA) flat extent
+#                        files, in addition to a descriptor file
+#
+# @streamOptimized:      Single file image sparse cluster allocation, optimized
+#                        for streaming over network.
+#
+# Since: 4.0
+##
+{ 'enum': 'BlockdevVmdkSubformat',
+  'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse',
+            'twoGbMaxExtentFlat', 'streamOptimized'] }
+
+##
+# @BlockdevVmdkAdapterType:
+#
+# Adapter type info for VMDK images
+#
+# Since: 4.0
+##
+{ 'enum': 'BlockdevVmdkAdapterType',
+  'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyESX'] }
+
+##
+# @BlockdevCreateOptionsVmdk:
+#
+# Driver specific image creation options for VMDK.

qapi/block-core.json +22 -0

+# @subformat    The subformat of the VMDK image. Default: "monolithicSparse".
+# @backing-file The path of backing file. Default: no backing file is used.
+# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
+# @hwversion    Hardware version. The meaningful options are "4" or "6".
+#               Default: "4".
+# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
+#               Default: false.
+#
+# Since: 4.0
+##
+{ 'struct': 'BlockdevCreateOptionsVmdk',
+  'data': { 'file':             'BlockdevRef',
+            'size':             'size',
+            '*extents':          ['BlockdevRef'],
+            '*subformat':       'BlockdevVmdkSubformat',
+            '*backing-file':    'str',
+            '*adapter-type':    'BlockdevVmdkAdapterType',
+            '*hwversion':       'str',
+            '*zeroed-grain':    'bool' } }
+
+
 ##
 # @SheepdogRedundancyType:
 #

@@ -4215,6 +4284,7 @@
       'ssh':            'BlockdevCreateOptionsSsh',
       'vdi':            'BlockdevCreateOptionsVdi',
       'vhdx':           'BlockdevCreateOptionsVhdx',
+      'vmdk':           'BlockdevCreateOptionsVmdk',
       'vpc':            'BlockdevCreateOptionsVpc'
   } }
 

Quoted 23 lines · Kevin Wolf

From: Fam Zheng <[email protected]>

This makes VMDK support blockdev-create. The implementation reuses the image creation code in vmdk_co_create_opts which now acceptes a callback pointer to "retrieve" BlockBackend pointers from the caller. This way we separate the logic between file/extent acquisition and initialization.

The QAPI command parameters are mostly the same as the old create_opts except the dropped legacy @compat6 switch, which is redundant with @hwversion.

Signed-off-by Fam Zheng <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json  |  70 +++++++
 qapi/qapi-schema.json |  16 +-
 block/vmdk.c          | 448 ++++++++++++++++++++++++++++++------------
 3 files changed, 400 insertions(+), 134 deletions(-)

qapi/block-core.json +0 -0

index d4fe710836..0793550cf2 100644

Quoted 9 lines

qapi/block-core.json +9 -0

+##
+# @BlockdevCreateOptionsVmdk:
+#
+# Driver specific image creation options for VMDK.
+#
+# @file         Where to store the new image file. This refers to the image
+#               file for monolithcSparse and streamOptimized format, or the
+#               descriptor file for other formats.
+# @size         Size of the virtual disk in bytes

qapi/block-core.json line 4071

Quoted 5 lines

qapi/block-core.json +5 -0

+# @extents      Where to store the data extents. Required for monolithcFlat,
+#               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
+#               monolithicFlat, only one entry is required; for
+#               twoGbMaxExtent* formats, the number of entries required is
+#               calculated as extent_number = virtual_size / 2GB.

Doesn't quite spell out that the number of extents has to match exactly. I'm not sure I care.

Signed-off-by Fam Zheng <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 tests/qemu-iotests/237     | 233 +++++++++++++++++++++++++
 tests/qemu-iotests/237.out | 347 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 581 insertions(+)
 create mode 100755 tests/qemu-iotests/237
 create mode 100644 tests/qemu-iotests/237.out

tests/qemu-iotests/237 new file +166 -0

new file mode 100755
index 0000000000..e04a1ac6be
@@ -0,0 +1,233 @@
+#!/usr/bin/env python
+#
+# Test vmdk and file image creation
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# Creator/Owner: Kevin Wolf <[email protected]>
+#
+# 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/>.
+#
+
+import iotests
+from iotests import imgfmt
+
+iotests.verify_image_format(supported_fmts=['vmdk'])
+
+def blockdev_create(vm, options):
+    result = vm.qmp_log('blockdev-create', job_id='job0', options=options)
+
+    if 'return' in result:
+        assert result['return'] == {}
+        vm.run_job('job0')
+    iotests.log("")
+
+with iotests.FilePath('t.vmdk') as disk_path, \
+     iotests.FilePath('t.vmdk.1') as extent1_path, \
+     iotests.FilePath('t.vmdk.2') as extent2_path, \
+     iotests.FilePath('t.vmdk.3') as extent3_path, \
+     iotests.VM() as vm:
+
+    #
+    # Successful image creation (defaults)
+    #
+    iotests.log("=== Successful image creation (defaults) ===")
+    iotests.log("")
+
+    size = 5 * 1024 * 1024 * 1024
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': 'file',
+                          'filename': disk_path,
+                          'size': 0 })
+
+    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
+               node_name='imgfile')
+
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': 'imgfile',
+                          'size': size })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
+
+    #
+    # Successful image creation (inline blockdev-add, explicit defaults)
+    #
+    iotests.log("=== Successful image creation (inline blockdev-add, explicit defaults) ===")
+    iotests.log("")
+
+    # Choose a different size to show that we got a new image
+    size = 64 * 1024 * 1024
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': 'file',
+                          'filename': disk_path,
+                          'size': 0 })
+
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': {
+                              'driver': 'file',
+                              'filename': disk_path,
+                          },
+                          'size': size,
+                          'extents': [],
+                          'subformat': 'monolithicSparse',
+                          'adapter-type': 'ide',
+                          'hwversion': '4',
+                          'zeroed-grain': False })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
+
+    #
+    # Successful image creation (non-default options)
+    #
+    iotests.log("=== Successful image creation (with non-default options) ===")
+    iotests.log("")
+
+    # Choose a different size to show that we got a new image
+    size = 32 * 1024 * 1024
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': 'file',
+                          'filename': disk_path,
+                          'size': 0 })
+
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': {
+                              'driver': 'file',
+                              'filename': disk_path,
+                          },
+                          'size': size,
+                          'extents': [],
+                          'subformat': 'monolithicSparse',
+                          'adapter-type': 'buslogic',
+                          'zeroed-grain': True })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
+
+    #
+    # Invalid BlockdevRef
+    #
+    iotests.log("=== Invalid BlockdevRef ===")
+    iotests.log("")
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': "this doesn't exist",
+                          'size': size })
+    vm.shutdown()
+
+    #
+    # Adapter types
+    #
+
+    iotests.log("=== Adapter types ===")
+    iotests.log("")
+
+    vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path))
+
+    # Valid
+    iotests.log("== Valid adapter types ==")
+    iotests.log("")
+
+    vm.launch()
+    for adapter_type in [ 'ide', 'buslogic', 'lsilogic', 'legacyESX' ]:
+        blockdev_create(vm, { 'driver': imgfmt,
+                              'file': 'node0',
+                              'size': size,
+                              'adapter-type': adapter_type })
+    vm.shutdown()
+
+    # Invalid
+    iotests.log("== Invalid adapter types ==")
+    iotests.log("")
+
+    vm.launch()
+    for adapter_type in [ 'foo', 'IDE', 'legacyesx', 1 ]:
+        blockdev_create(vm, { 'driver': imgfmt,
+                              'file': 'node0',
+                              'size': size,
+                              'adapter-type': adapter_type })
+    vm.shutdown()
+

tests/qemu-iotests/237 new file +3 -0

+    vm.add_blockdev('driver=file,filename=%s,node-name=ext1' % (extent1_path))
+    vm.add_blockdev('driver=file,filename=%s,node-name=ext2' % (extent2_path))
+    vm.add_blockdev('driver=file,filename=%s,node-name=ext3' % (extent3_path))

git-am complains

Applying: iotests: Add VMDK tests for blockdev-create .git/rebase-apply/patch:281: trailing whitespace.
format:
.git/rebase-apply/patch:308: trailing whitespace.
format:
.git/rebase-apply/patch:335: trailing whitespace.
format:
.git/rebase-apply/patch:600: new blank line at EOF. +
warning: 4 lines add whitespace errors.

Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:

Quoted 12 lines

git-am complains

Applying: iotests: Add VMDK tests for blockdev-create .git/rebase-apply/patch:281: trailing whitespace.
format:
.git/rebase-apply/patch:308: trailing whitespace.
format:
.git/rebase-apply/patch:335: trailing whitespace.
format:
.git/rebase-apply/patch:600: new blank line at EOF. +
warning: 4 lines add whitespace errors.

This is in the reference output, so trailing whitespace/blank lines are actually correct.

Quoted 14 lines

Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:

Quoted 12 lines

git-am complains

Applying: iotests: Add VMDK tests for blockdev-create .git/rebase-apply/patch:281: trailing whitespace.
format:
.git/rebase-apply/patch:308: trailing whitespace.
format:
.git/rebase-apply/patch:335: trailing whitespace.
format:
.git/rebase-apply/patch:600: new blank line at EOF. +
warning: 4 lines add whitespace errors.

Quoted 2 lines

This is in the reference output, so trailing whitespace/blank lines are actually correct.

Ah, but doesn't ./check already ignore differences in trailing whitespace present in the actual running that is not present in the *.out files, precisely so we don't have to check in trailing whitespace reference outputs?

Am 07.12.2018 um 16:40 hat Eric Blake geschrieben:

Quoted 15 lines · Kevin Wolf

Quoted 14 lines

Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:

Quoted 12 lines

git-am complains

Applying: iotests: Add VMDK tests for blockdev-create .git/rebase-apply/patch:281: trailing whitespace.
format:
.git/rebase-apply/patch:308: trailing whitespace.
format:
.git/rebase-apply/patch:335: trailing whitespace.
format:
.git/rebase-apply/patch:600: new blank line at EOF. +
warning: 4 lines add whitespace errors.

Quoted 7 lines

Quoted 2 lines

This is in the reference output, so trailing whitespace/blank lines are actually correct.

Ah, but doesn't ./check already ignore differences in trailing whitespace present in the actual running that is not present in the *.out files, precisely so we don't have to check in trailing whitespace reference outputs?

It does ignore whitespace changes, so even if we remove that whitespace, the test won't fail. But I don't think that's a good reason to check in inaccurate reference output.

There are a few test cases that have a reference output like this and it's always annoying: When I later add a new subtest, I add the new test code, review the ./check output and if it looks good, I do something like 'cp 237.out.bad 237.out'. At that point, I'll have to manually revert completely unrelated whitespace changes again.

Some 'git am' warnings feel like the lesser evil to me.

Quoted 28 lines

Am 07.12.2018 um 16:40 hat Eric Blake geschrieben:

Quoted 15 lines · Kevin Wolf

Quoted 14 lines

Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:

Quoted 12 lines

git-am complains

Applying: iotests: Add VMDK tests for blockdev-create .git/rebase-apply/patch:281: trailing whitespace.
format:
.git/rebase-apply/patch:308: trailing whitespace.
format:
.git/rebase-apply/patch:335: trailing whitespace.
format:
.git/rebase-apply/patch:600: new blank line at EOF. +
warning: 4 lines add whitespace errors.

Quoted 7 lines

Quoted 2 lines

This is in the reference output, so trailing whitespace/blank lines are actually correct.

Ah, but doesn't ./check already ignore differences in trailing whitespace present in the actual running that is not present in the *.out files, precisely so we don't have to check in trailing whitespace reference outputs?

It does ignore whitespace changes, so even if we remove that whitespace, the test won't fail. But I don't think that's a good reason to check in inaccurate reference output.

Quoted 5 lines

There are a few test cases that have a reference output like this and it's always annoying: When I later add a new subtest, I add the new test code, review the ./check output and if it looks good, I do something like 'cp 237.out.bad 237.out'. At that point, I'll have to manually revert completely unrelated whitespace changes again.

Is it worth teaching iotests.img_info_log() to strip trailing whitespace? Or even to teach qemu-img itself to quit generating trailing whitespace?

Quoted 1 line

Some 'git am' warnings feel like the lesser evil to me.

True, and a comment in the commit message about intentionally triggering a known checkpatch flag goes a long ways to document why you want the trailing whitespace (assuming we don't instead decide to tackle a root cause of having the whitespace in the first place).

Clarify that the number of extents provided in BlockdevCreateOptionsVmdk must match the number of extents that will actually be used. Providing more extents will result in an error now.

This requires adapting the test case to provide the right number of extents.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json       |  3 ++-
 block/vmdk.c               | 29 ++++++++++++++++++++++++-----
 tests/qemu-iotests/237     |  6 +++++-
 tests/qemu-iotests/237.out | 13 +++++++------
 4 files changed, 38 insertions(+), 13 deletions(-)

qapi/block-core.json +2 -1

index 0793550cf2..afdd2f89a2 100644
@@ -4068,7 +4068,8 @@
 #               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
 #               monolithicFlat, only one entry is required; for
 #               twoGbMaxExtent* formats, the number of entries required is
-#               calculated as extent_number = virtual_size / 2GB.
+#               calculated as extent_number = virtual_size / 2GB. Providing
+#               more extents than will be used is an error.
 # @subformat    The subformat of the VMDK image. Default: "monolithicSparse".
 # @backing-file The path of backing file. Default: no backing file is used.
 # @adapter-type The adapter type used to fill in the descriptor. Default: ide.

block/vmdk.c +12 -1

index 5a162ee85c..682ad93aa1 100644
@@ -1970,6 +1970,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
 {
     int extent_idx;
     BlockBackend *blk = NULL;
+    BlockBackend *extent_blk;
     Error *local_err = NULL;
     char *desc = NULL;
     int ret = 0;

@@ -2107,7 +2108,6 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
     }
     extent_idx = 1;
     while (created_size < size) {
-        BlockBackend *extent_blk;
         int64_t cur_size = MIN(size - created_size, extent_size);
         extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress,
                                zeroed_grain, opaque, errp);

@@ -2121,6 +2121,17 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
         extent_idx++;
         blk_unref(extent_blk);
     }
+
+    /* Check whether we got excess extents */
+    extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
+                           opaque, NULL);
+    if (extent_blk) {
+        blk_unref(extent_blk);
+        error_setg(errp, "List of extents contains unused extents");
+        ret = -EINVAL;
+        goto exit;
+    }
+
     /* generate descriptor file */
     desc = g_strdup_printf(desc_template,
                            g_random_int(),

block/vmdk.c +6 -4

     if (idx == 0) {
         rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix);
     } else if (split) {

@@ -2342,10 +2359,12 @@ static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
     blk_set_allow_write_beyond_eof(blk, true);
     bdrv_unref(bs);
 
-    ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
-    if (ret) {
-        blk_unref(blk);
-        blk = NULL;
+    if (size != -1) {
+        ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
+        if (ret) {
+            blk_unref(blk);
+            blk = NULL;
+        }
     }
     return blk;
 }

tests/qemu-iotests/237 +5 -1

index e04a1ac6be..251771d7fb 100755
@@ -20,6 +20,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+import math
 import iotests
 from iotests import imgfmt
 

@@ -222,12 +223,15 @@ with iotests.FilePath('t.vmdk') as disk_path, \
             iotests.log("= %s %d =" % (subfmt, size))
             iotests.log("")
 
+            num_extents = math.ceil(size / 2.0**31)
+            extents = [ "ext%d" % (i) for i in range(1, num_extents + 1) ]
+
             vm.launch()
             blockdev_create(vm, { 'driver': imgfmt,
                                   'file': 'node0',
                                   'size': size,
                                   'subformat': subfmt,
-                                  'extents': ['ext1', 'ext2', 'ext3'] })
+                                  'extents': extents })
             vm.shutdown()
 
             iotests.img_info_log(disk_path)

tests/qemu-iotests/237.out +6 -5

index 1aa9aad349..241c864369 100644
@@ -154,6 +154,7 @@ Job failed: Extent [0] not specified
 
 {"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "monolithicFlat"}}}
 {"return": {}}
+Job failed: List of extents contains unused extents
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
 

@@ -161,7 +162,7 @@ Job failed: Extent [0] not specified
 
 = twoGbMaxExtentFlat 512 =
 
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentFlat"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentFlat"}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}

@@ -181,7 +182,7 @@ Format specific information:
 
 = twoGbMaxExtentSparse 512 =
 
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentSparse"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentSparse"}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}

@@ -203,7 +204,7 @@ Format specific information:
 
 = twoGbMaxExtentFlat 1073741824 =
 
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentFlat"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentFlat"}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}

@@ -223,7 +224,7 @@ Format specific information:
 
 = twoGbMaxExtentSparse 1073741824 =
 
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentSparse"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentSparse"}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}

@@ -245,7 +246,7 @@ Format specific information:
 
 = twoGbMaxExtentFlat 2147483648 =
 
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentFlat"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentFlat"}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}

Quoted 71 lines · Kevin Wolf

Clarify that the number of extents provided in BlockdevCreateOptionsVmdk must match the number of extents that will actually be used. Providing more extents will result in an error now.

This requires adapting the test case to provide the right number of extents.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json       |  3 ++-
 block/vmdk.c               | 29 ++++++++++++++++++++++++-----
 tests/qemu-iotests/237     |  6 +++++-
 tests/qemu-iotests/237.out | 13 +++++++------
 4 files changed, 38 insertions(+), 13 deletions(-)

qapi/block-core.json +2 -1

index 0793550cf2..afdd2f89a2 100644
@@ -4068,7 +4068,8 @@
 #               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
 #               monolithicFlat, only one entry is required; for
 #               twoGbMaxExtent* formats, the number of entries required is
-#               calculated as extent_number = virtual_size / 2GB.
+#               calculated as extent_number = virtual_size / 2GB. Providing
+#               more extents than will be used is an error.
 # @subformat    The subformat of the VMDK image. Default: "monolithicSparse".
 # @backing-file The path of backing file. Default: no backing file is used.
 # @adapter-type The adapter type used to fill in the descriptor. Default: ide.

block/vmdk.c +12 -1

index 5a162ee85c..682ad93aa1 100644
@@ -1970,6 +1970,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
 {
     int extent_idx;
     BlockBackend *blk = NULL;
+    BlockBackend *extent_blk;
     Error *local_err = NULL;
     char *desc = NULL;
     int ret = 0;

@@ -2107,7 +2108,6 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
     }
     extent_idx = 1;
     while (created_size < size) {
-        BlockBackend *extent_blk;
         int64_t cur_size = MIN(size - created_size, extent_size);
         extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress,
                                zeroed_grain, opaque, errp);

@@ -2121,6 +2121,17 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
         extent_idx++;
         blk_unref(extent_blk);
     }
+
+    /* Check whether we got excess extents */
+    extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
+                           opaque, NULL);
+    if (extent_blk) {
+        blk_unref(extent_blk);
+        error_setg(errp, "List of extents contains unused extents");
+        ret = -EINVAL;
+        goto exit;
+    }
+
     /* generate descriptor file */
     desc = g_strdup_printf(desc_template,
                            g_random_int(),

@@ -2181,6 +2192,12 @@ static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
     char *ext_filename = NULL;
     char *rel_filename = NULL;
 

block/vmdk.c static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx, · line 2200

Quoted 6 lines

block/vmdk.c +6 -0

+    /* We're done, don't create excess extents. */
+    if (size == -1) {
+        assert(errp == NULL);
+        return NULL;
+    }
+

I'm afraid I don't get this hunk.

Am 07.12.2018 um 14:11 hat Markus Armbruster geschrieben:

Quoted 73 lines

Quoted 71 lines · Kevin Wolf

Clarify that the number of extents provided in BlockdevCreateOptionsVmdk must match the number of extents that will actually be used. Providing more extents will result in an error now.

This requires adapting the test case to provide the right number of extents.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json       |  3 ++-
 block/vmdk.c               | 29 ++++++++++++++++++++++++-----
 tests/qemu-iotests/237     |  6 +++++-
 tests/qemu-iotests/237.out | 13 +++++++------
 4 files changed, 38 insertions(+), 13 deletions(-)

qapi/block-core.json +2 -1

index 0793550cf2..afdd2f89a2 100644
@@ -4068,7 +4068,8 @@
 #               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
 #               monolithicFlat, only one entry is required; for
 #               twoGbMaxExtent* formats, the number of entries required is
-#               calculated as extent_number = virtual_size / 2GB.
+#               calculated as extent_number = virtual_size / 2GB. Providing
+#               more extents than will be used is an error.
 # @subformat    The subformat of the VMDK image. Default: "monolithicSparse".
 # @backing-file The path of backing file. Default: no backing file is used.
 # @adapter-type The adapter type used to fill in the descriptor. Default: ide.

block/vmdk.c +12 -1

index 5a162ee85c..682ad93aa1 100644
@@ -1970,6 +1970,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
 {
     int extent_idx;
     BlockBackend *blk = NULL;
+    BlockBackend *extent_blk;
     Error *local_err = NULL;
     char *desc = NULL;
     int ret = 0;

@@ -2107,7 +2108,6 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
     }
     extent_idx = 1;
     while (created_size < size) {
-        BlockBackend *extent_blk;
         int64_t cur_size = MIN(size - created_size, extent_size);
         extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress,
                                zeroed_grain, opaque, errp);

@@ -2121,6 +2121,17 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
         extent_idx++;
         blk_unref(extent_blk);
     }
+
+    /* Check whether we got excess extents */
+    extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
+                           opaque, NULL);
+    if (extent_blk) {
+        blk_unref(extent_blk);
+        error_setg(errp, "List of extents contains unused extents");
+        ret = -EINVAL;
+        goto exit;
+    }
+
     /* generate descriptor file */
     desc = g_strdup_printf(desc_template,
                            g_random_int(),

@@ -2181,6 +2192,12 @@ static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
     char *ext_filename = NULL;
     char *rel_filename = NULL;
 

Quoted 8 lines

block/vmdk.c static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx, · line 2200

Quoted 6 lines

block/vmdk.c +6 -0

+    /* We're done, don't create excess extents. */
+    if (size == -1) {
+        assert(errp == NULL);
+        return NULL;
+    }
+

I'm afraid I don't get this hunk.

vmdk_co_create_opts_cb() is for the legacy code path, where 'qemu-img create' passes us a QemuOpts. This doesn't contain extents, so rather than returning ther next extent from the input, this function simply creates a new extent file each time it is called.

When checking whether we have too many extents, we don't know from which code path we came, so we have to call the callback uncondtionally and see whether it still returns an extra extent. In this case, creating a new one would obviously be counterproductive and trigger an error, so I just return NULL immediately.

If you have a suggestion for a comment to put somewhere, I'll gladly add it.