Skip to contentSkip to conversation

block: Follow-up for .bdrv_co_create (part 1)

PATCH Superseded Kevin Wolf 4 participants 34 messages Mar 20, 2018 – Mar 21, 2018 Block layer

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

cover block: Follow-up for .bdrv_co_create (part 1)

This series adds qemu-iotests for a few more block drivers (yet more to come in another series) and fixes a few things that previous review and these tests brought up.

The only major design change is that I converted the vdi block driver from a boolean 'static' create option to the standard 'preallocation' one that other drivers are using. This seems like a good move to make while the interface isn't stable yet.

Kevin Wolf (12):
vdi: Change 'static' create option to 'preallocation' in QMP
vdi: Fix build with CONFIG_VDI_DEBUG
qemu-iotests: Test vdi image creation with QMP
qemu-iotests: Enable 025 for luks
luks: Turn another invalid assertion into check
qemu-iotests: Test invalid resize on luks
parallels: Check maximum cluster size on create
qemu-iotests: Test parallels image creation with QMP
vhdx: Require power-of-two block size on create
vhdx: Don't use error_setg_errno() with constant errno
vhdx: Check for 4 GB maximum log size on creation
qemu-iotests: Test vhdx image creation with QMP

 qapi/block-core.json       |   6 +-
 block/crypto.c             |   6 +-
 block/parallels.c          |   5 +
 block/vdi.c                |  46 ++++--
 block/vhdx.c               |  17 ++-
 tests/qemu-iotests/025     |   9 +-
 tests/qemu-iotests/210     |  37 +++++
 tests/qemu-iotests/211     | 256 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/211.out |  98 +++++++++++++
 tests/qemu-iotests/212     | 326 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/212.out | 111 ++++++++++++++
 tests/qemu-iotests/213     | 349 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/213.out | 121 ++++++++++++++++
 tests/qemu-iotests/group   |   3 +
 14 files changed, 1365 insertions(+), 25 deletions(-)
 create mode 100755 tests/qemu-iotests/211
 create mode 100644 tests/qemu-iotests/211.out
 create mode 100755 tests/qemu-iotests/212
 create mode 100644 tests/qemu-iotests/212.out
 create mode 100755 tests/qemu-iotests/213
 create mode 100644 tests/qemu-iotests/213.out
Signature · 1 line
2.13.6
author
#1 lore raw

PATCH 1/12 vdi: Change 'static' create option to 'preallocation' in QMP

What static=on really does is what we call metadata preallocation for other block drivers. While we can still change the QMP interface, make it more consistent by using 'preallocation' for VDI, too.

This doesn't implement any new functionality, so the only supported preallocation modes are 'off' and 'metadata' for now.

---
 qapi/block-core.json |  6 ++----
 block/vdi.c          | 24 ++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 6 deletions(-)
qapi/block-core.json +2 -4
index 5b0ad1a8b7..f6673f2b13 100644
@@ -3834,16 +3834,14 @@
 #
 # @file             Node to create the image format on
 # @size             Size of the virtual disk in bytes
-# @static           Whether to create a statically (true) or
-#                   dynamically (false) allocated image
-#                   (default: false, i.e. dynamic)
+# @preallocation    Preallocation mode for the new image (default: off)
 #
 # Since: 2.12
 ##
 { 'struct': 'BlockdevCreateOptionsVdi',
   'data': { 'file':             'BlockdevRef',
             'size':             'size',
-            '*static':          'bool' } }
+            '*preallocation':   'PreallocMode' } }
 
 ##
 # @BlockdevVhdxSubformat:
block/vdi.c +22 -2
index d939b034c4..73c059e69d 100644
@@ -728,7 +728,7 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
     int ret = 0;
     uint64_t bytes = 0;
     uint32_t blocks;
-    uint32_t image_type = VDI_TYPE_DYNAMIC;
+    uint32_t image_type;
     VdiHeader header;
     size_t i;
     size_t bmap_size;
@@ -744,9 +744,22 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
 
     /* Validate options and set default values */
     bytes = vdi_opts->size;
-    if (vdi_opts->q_static) {
+
+    if (!vdi_opts->has_preallocation) {
+        vdi_opts->preallocation = PREALLOC_MODE_OFF;
+    }
+    switch (vdi_opts->preallocation) {
+    case PREALLOC_MODE_OFF:
+        image_type = VDI_TYPE_DYNAMIC;
+        break;
+    case PREALLOC_MODE_METADATA:
         image_type = VDI_TYPE_STATIC;
+        break;
+    default:
+        error_setg(errp, "Preallocation mode not supported for vdi");
+        return -EINVAL;
     }
+
 #ifndef CONFIG_VDI_STATIC_IMAGE
     if (image_type == VDI_TYPE_STATIC) {
         ret = -ENOTSUP;
@@ -874,6 +887,7 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
     BlockdevCreateOptions *create_options = NULL;
     BlockDriverState *bs_file = NULL;
     uint64_t block_size = DEFAULT_CLUSTER_SIZE;
+    bool is_static = false;
     Visitor *v;
     Error *local_err = NULL;
     int ret;
@@ -895,6 +909,9 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
         goto done;
     }
 #endif
+    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_STATIC, false)) {
+        is_static = true;
+    }
 
     qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vdi_create_opts, true);
 
@@ -913,6 +930,9 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
 
     qdict_put_str(qdict, "driver", "vdi");
     qdict_put_str(qdict, "file", bs_file->node_name);
+    if (is_static) {
+        qdict_put_str(qdict, "preallocation", "metadata");
+    }
 
     /* Get the QAPI object */
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
Signature · 1 line
2.13.6
Reviewed-by on v1 1/12
#2 lore raw

Re: [Qemu-devel] [PATCH for-2.12 01/12] vdi: Change 'static' create option to 'preallocation' in QMP

Quoted 3 lines · Kevin Wolf

What static=on really does is what we call metadata preallocation for other block drivers. While we can still change the QMP interface, make it more consistent by using 'preallocation' for VDI, too.

The x- naming of the command means we don't HAVE to get this patch in for 2.12, but I agree that sooner is better than later and that doing this in 2.12 is worth the effort.

Quoted 13 lines

This doesn't implement any new functionality, so the only supported preallocation modes are 'off' and 'metadata' for now.

---
  qapi/block-core.json |  6 ++----
  block/vdi.c          | 24 ++++++++++++++++++++++--
  2 files changed, 24 insertions(+), 6 deletions(-)

+++ b/qapi/block-core.json

qapi/block-core.json line 3837

Quoted 7 lines
qapi/block-core.json +1 -3
@@ -3834,16 +3834,14 @@
  #
  # @file             Node to create the image format on
  # @size             Size of the virtual disk in bytes
-# @static           Whether to create a statically (true) or
-#                   dynamically (false) allocated image
-#                   (default: false, i.e. dynamic)
+# @preallocation    Preallocation mode for the new image (default: off)

Do we want to document that 'full' is not supported yet? Or is just relying on the error message good enough?

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#3 lore raw

PATCH 2/12 vdi: Fix build with CONFIG_VDI_DEBUG

In reply to Kevin Wolf

Use qemu_uuid_unparse() instead of uuid_unparse() to make vdi.c compile again when CONFIG_VDI_DEBUG is set. In order to prevent future bitrot, replace '#ifdef CONFIG_VDI_DEBUG' by 'if (VDI_DEBUG)' so that the compiler always sees the code.

---
 block/vdi.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
block/vdi.c +10 -12
index 73c059e69d..4a2d1ff88d 100644
@@ -235,7 +235,6 @@ static void vdi_header_to_le(VdiHeader *header)
     qemu_uuid_bswap(&header->uuid_parent);
 }
 
-#if defined(CONFIG_VDI_DEBUG)
 static void vdi_header_print(VdiHeader *header)
 {
     char uuid[37];
@@ -257,16 +256,15 @@ static void vdi_header_print(VdiHeader *header)
     logout("block extra 0x%04x\n", header->block_extra);
     logout("blocks tot. 0x%04x\n", header->blocks_in_image);
     logout("blocks all. 0x%04x\n", header->blocks_allocated);
-    uuid_unparse(header->uuid_image, uuid);
+    qemu_uuid_unparse(&header->uuid_image, uuid);
     logout("uuid image  %s\n", uuid);
-    uuid_unparse(header->uuid_last_snap, uuid);
+    qemu_uuid_unparse(&header->uuid_last_snap, uuid);
     logout("uuid snap   %s\n", uuid);
-    uuid_unparse(header->uuid_link, uuid);
+    qemu_uuid_unparse(&header->uuid_link, uuid);
     logout("uuid link   %s\n", uuid);
-    uuid_unparse(header->uuid_parent, uuid);
+    qemu_uuid_unparse(&header->uuid_parent, uuid);
     logout("uuid parent %s\n", uuid);
 }
-#endif
 
 static int coroutine_fn vdi_co_check(BlockDriverState *bs, BdrvCheckResult *res,
                                      BdrvCheckMode fix)
@@ -387,9 +385,9 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     vdi_header_to_cpu(&header);
-#if defined(CONFIG_VDI_DEBUG)
-    vdi_header_print(&header);
-#endif
+    if (VDI_DEBUG) {
+        vdi_header_print(&header);
+    }
 
     if (header.disk_size > VDI_DISK_SIZE_MAX) {
         error_setg(errp, "Unsupported VDI image size (size is 0x%" PRIx64
@@ -825,9 +823,9 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
     qemu_uuid_generate(&header.uuid_image);
     qemu_uuid_generate(&header.uuid_last_snap);
     /* There is no need to set header.uuid_link or header.uuid_parent here. */
-#if defined(CONFIG_VDI_DEBUG)
-    vdi_header_print(&header);
-#endif
+    if (VDI_DEBUG) {
+        vdi_header_print(&header);
+    }
     vdi_header_to_le(&header);
     ret = blk_pwrite(blk, offset, &header, sizeof(header), 0);
     if (ret < 0) {
Signature · 1 line
2.13.6
Reviewed-by on v1 2/12
#4 lore raw

Re: [Qemu-devel] [PATCH for-2.12 02/12] vdi: Fix build with CONFIG_VDI_DEBUG

Quoted 10 lines · Kevin Wolf

Use qemu_uuid_unparse() instead of uuid_unparse() to make vdi.c compile again when CONFIG_VDI_DEBUG is set. In order to prevent future bitrot, replace '#ifdef CONFIG_VDI_DEBUG' by 'if (VDI_DEBUG)' so that the compiler always sees the code.

---
  block/vdi.c | 22 ++++++++++------------
  1 file changed, 10 insertions(+), 12 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#5 lore raw

PATCH 3/12 qemu-iotests: Test vdi image creation with QMP

In reply to Kevin Wolf
---
 tests/qemu-iotests/211     | 256 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/211.out |  98 +++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 355 insertions(+)
 create mode 100755 tests/qemu-iotests/211
 create mode 100644 tests/qemu-iotests/211.out
tests/qemu-iotests/211 new file +256 -0
new file mode 100755
index 0000000000..6bf1ca784c
@@ -0,0 +1,256 @@
+#!/bin/bash
+#
+# Test VDI and file image creation
+#
+# Copyright (C) 2018 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
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt vdi
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@"
+    $QEMU -nographic -qmp stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+                          | _filter_qemu | _filter_imgfmt \
+                          | _filter_actual_image_size
+}
+
+echo
+echo "=== Successful image creation (defaults) ==="
+echo
+
+size=$((128 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "blockdev-add",
+  "arguments": {
+      "driver": "file",
+      "node-name": "imgfile",
+      "filename": "$TEST_IMG"
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "imgfile",
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+echo
+echo "=== Successful image creation (explicit defaults) ==="
+echo
+
+# Choose a different size to show that we got a new image
+size=$((64 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "preallocation": "off"
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+echo
+echo "=== Successful image creation (with non-default options) ==="
+echo
+
+# Choose a different size to show that we got a new image
+size=$((32 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "preallocation": "metadata"
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+echo
+echo "=== Invalid BlockdevRef ==="
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "this doesn't exist",
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Zero size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 0
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+echo
+echo "=== Maximum size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 562949819203584
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)
+# 4. 0x1fffff8000000 (maximum image size for VDI)
+# 5. (one byte more than maximum image size for VDI)
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 18446744073709551104
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775808
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775296
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 562949819203585
+  }
+}
+{ "execute": "quit" }
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
tests/qemu-iotests/211.out new file +98 -0

98 lines not on this page · raw message

tests/qemu-iotests/group +1 -0
index 0c2983f4cd..0d11d40aec 100644
@@ -209,3 +209,4 @@
 208 rw auto quick
 209 rw auto quick
 210 rw auto
+211 rw auto quick
Signature · 1 line
2.13.6
#6 lore raw

Re: [Qemu-devel] [PATCH for-2.12 03/12] qemu-iotests: Test vdi image creation with QMP

Quoted 10 lines · Kevin Wolf
---
  tests/qemu-iotests/211     | 256 +++++++++++++++++++++++++++++++++++++++++++++
  tests/qemu-iotests/211.out |  98 +++++++++++++++++
  tests/qemu-iotests/group   |   1 +
  3 files changed, 355 insertions(+)

create mode 100755 tests/qemu-iotests/211
create mode 100644 tests/qemu-iotests/211.out

tests/qemu-iotests/211 line 28

Quoted 9 lines
tests/qemu-iotests/211 new file +9 -0
+
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!

More of the useless boilerplate we've been meaning to clean up. Oh well.

tests/qemu-iotests/211 line 218

Quoted 10 lines
tests/qemu-iotests/211 new file +10 -0
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)
+# 4. 0x1fffff8000000 (maximum image size for VDI)
+# 5. (one byte more than maximum image size for VDI)

You list 5 cases...

Quoted 20 lines
tests/qemu-iotests/211 new file +20 -0
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 18446744073709551104
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775808
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",

tests/qemu-iotests/211 line 248

Quoted 10 lines
tests/qemu-iotests/211 new file +10 -0
+      "file": "node0",
+      "size": 9223372036854775296
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 562949819203585
+  }

...but only four x-blockdev-create. Why?

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#7 lore raw

PATCH 4/12 qemu-iotests: Enable 025 for luks

In reply to Kevin Wolf

We want to test resizing even for luks. The only change that is needed is to explicitly zero out new space for luks because it's undefined.

---
 tests/qemu-iotests/025 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
tests/qemu-iotests/025 +8 -1
index f5e672e6b3..70dd5f10aa 100755
@@ -38,7 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 . ./common.pattern
 
-_supported_fmt raw qcow2 qed
+_supported_fmt raw qcow2 qed luks
 _supported_proto file sheepdog rbd nfs
 _supported_os Linux
 
@@ -62,6 +62,13 @@ length
 EOF
 _check_test_img
 
+# bdrv_truncate() doesn't zero the new space, so we need to do that explicitly.
+# We still want to test automatic zeroing for other formats even though
+# bdrv_truncate() doesn't guarantee it.
+if [ "$IMGFMT" == "luks" ]; then
+    $QEMU_IO -c "write -z $small_size $((big_size - small_size))" "$TEST_IMG" > /dev/null
+fi
+
 echo
 echo "=== Verifying image size after reopen"
 $QEMU_IO -c "length" "$TEST_IMG"
Signature · 1 line
2.13.6
Reviewed-by on v1 4/12
#8 lore raw

Re: [Qemu-devel] [PATCH for-2.12 04/12] qemu-iotests: Enable 025 for luks

Quoted 8 lines · Kevin Wolf

We want to test resizing even for luks. The only change that is needed is to explicitly zero out new space for luks because it's undefined.

---
  tests/qemu-iotests/025 | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Reviewed-by on v1 4/12
#9 lore raw

Re: [Qemu-devel] [PATCH for-2.12 04/12] qemu-iotests: Enable 025 for luks

In reply to Kevin Wolf
Quoted 7 lines · Kevin Wolf

We want to test resizing even for luks. The only change that is needed is to explicitly zero out new space for luks because it's undefined.

---
 tests/qemu-iotests/025 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Regards,
Daniel

Signature · 3 lines
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
author
#10 lore raw

PATCH 5/12 luks: Turn another invalid assertion into check

In reply to Kevin Wolf

Commit e39e959e fixed an invalid assertion in the .bdrv_length implementation, but left a similar assertion in place for .bdrv_truncate. Instead of crashing when the user requests a too large image size, fail gracefully.

A file size of exactly INT64_MAX caused failure before, but is actually legal.

---
 block/crypto.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
block/crypto.c +5 -1
index e0b8856f74..bc6c7e3795 100644
@@ -357,7 +357,11 @@ static int block_crypto_truncate(BlockDriverState *bs, int64_t offset,
     BlockCrypto *crypto = bs->opaque;
     uint64_t payload_offset =
         qcrypto_block_get_payload_offset(crypto->block);
-    assert(payload_offset < (INT64_MAX - offset));
+
+    if (payload_offset > INT64_MAX - offset) {
+        error_setg(errp, "The requested file size is too large");
+        return -EFBIG;
+    }
 
     offset += payload_offset;
 
Signature · 1 line
2.13.6
Reviewed-by on v1 5/12
#11 lore raw

Re: [Qemu-devel] [PATCH for-2.12 05/12] luks: Turn another invalid assertion into check

Quoted 5 lines · Kevin Wolf

Commit e39e959e fixed an invalid assertion in the .bdrv_length implementation, but left a similar assertion in place for .bdrv_truncate. Instead of crashing when the user requests a too large image size, fail gracefully.

Quoted 7 lines

A file size of exactly INT64_MAX caused failure before, but is actually legal.

---
  block/crypto.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Reviewed-by on v1 5/12
#12 lore raw

Re: [Qemu-devel] [PATCH for-2.12 05/12] luks: Turn another invalid assertion into check

In reply to Kevin Wolf
Quoted 5 lines · Kevin Wolf

Commit e39e959e fixed an invalid assertion in the .bdrv_length implementation, but left a similar assertion in place for .bdrv_truncate. Instead of crashing when the user requests a too large image size, fail gracefully.

Quoted 7 lines

A file size of exactly INT64_MAX caused failure before, but is actually legal.

---
 block/crypto.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Regards,
Daniel

Signature · 3 lines
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
author
#13 lore raw

PATCH 6/12 qemu-iotests: Test invalid resize on luks

In reply to Kevin Wolf

This tests that the .bdrv_truncate implementation for luks doesn't crash for invalid image sizes.

---
 tests/qemu-iotests/210 | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
tests/qemu-iotests/210 +37 -0
index 96a5213e77..e607c0d296 100755
@@ -204,6 +204,43 @@ run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
 { "execute": "quit" }
 EOF
 
+echo
+echo "=== Resize image with invalid sizes ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
+         -blockdev driver=luks,file=node0,key-secret=keysec0,node-name=node1 \
+         -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "block_resize",
+  "arguments": {
+      "node-name": "node1",
+      "size": 9223372036854775296
+  }
+}
+{ "execute": "block_resize",
+  "arguments": {
+      "node-name": "node1",
+      "size": 9223372036854775808
+  }
+}
+{ "execute": "block_resize",
+  "arguments": {
+      "node-name": "node1",
+      "size": 18446744073709551104
+  }
+}
+{ "execute": "block_resize",
+  "arguments": {
+      "node-name": "node1",
+      "size": -9223372036854775808
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
Signature · 1 line
2.13.6
#14 lore raw

Re: [Qemu-devel] [PATCH for-2.12 06/12] qemu-iotests: Test invalid resize on luks

Quoted 7 lines · Kevin Wolf

This tests that the .bdrv_truncate implementation for luks doesn't crash for invalid image sizes.

---
  tests/qemu-iotests/210 | 37 +++++++++++++++++++++++++++++++++++++
  1 file changed, 37 insertions(+)

Missing a change to 210.out?

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#15 lore raw

Re: [Qemu-devel] [PATCH for-2.12 06/12] qemu-iotests: Test invalid resize on luks

Am 20.03.2018 um 19:33 hat Eric Blake geschrieben:

Quoted 10 lines · Kevin Wolf
Quoted 7 lines

This tests that the .bdrv_truncate implementation for luks doesn't crash for invalid image sizes.

---
  tests/qemu-iotests/210 | 37 +++++++++++++++++++++++++++++++++++++
  1 file changed, 37 insertions(+)

Missing a change to 210.out?

Obviously. I wonder how that happened... Thanks!

Kevin

Reviewed-by on v1 6/12
#16 lore raw

Re: [Qemu-devel] [PATCH for-2.12 06/12] qemu-iotests: Test invalid resize on luks

In reply to Kevin Wolf
Quoted 7 lines · Kevin Wolf

This tests that the .bdrv_truncate implementation for luks doesn't crash for invalid image sizes.

---
 tests/qemu-iotests/210 | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Regards,
Daniel

Signature · 3 lines
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
author
#17 lore raw

PATCH 7/12 parallels: Check maximum cluster size on create

In reply to Kevin Wolf

It's unclear what the real maximum cluster size is for the Parallels format, but let's at least make sure that we don't get integer overflows in our .bdrv_co_create implementation.

---
 block/parallels.c | 5 +++++
 1 file changed, 5 insertions(+)
block/parallels.c +5 -0
index 2da5e56a9d..e4ca018c2e 100644
@@ -526,6 +526,11 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
         cl_size = DEFAULT_CLUSTER_SIZE;
     }
 
+    /* XXX What is the real limit here? This is an insanely large maximum. */
+    if (cl_size >= UINT64_MAX / MAX_PARALLELS_IMAGE_FACTOR) {
+        error_setg(errp, "Cluster size is too large");
+        return -EINVAL;
+    }
     if (total_size >= MAX_PARALLELS_IMAGE_FACTOR * cl_size) {
         error_setg(errp, "Image size is too large for this cluster size");
         return -E2BIG;
Signature · 1 line
2.13.6
Reviewed-by on v1 7/12
#18 lore raw

Re: [Qemu-devel] [PATCH for-2.12 07/12] parallels: Check maximum cluster size on create

Quoted 14 lines · Kevin Wolf

It's unclear what the real maximum cluster size is for the Parallels format, but let's at least make sure that we don't get integer overflows in our .bdrv_co_create implementation.

---
  block/parallels.c | 5 +++++
  1 file changed, 5 insertions(+)
block/parallels.c +0 -0
index 2da5e56a9d..e4ca018c2e 100644

block/parallels.c static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, · line 530

Quoted 5 lines
block/parallels.c +2 -0
@@ -526,6 +526,11 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
          cl_size = DEFAULT_CLUSTER_SIZE;
      }
 
+    /* XXX What is the real limit here? This is an insanely large maximum. */
+    if (cl_size >= UINT64_MAX / MAX_PARALLELS_IMAGE_FACTOR) {

INT64_MAX is probably a saner starting point for the division...

block/parallels.c static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, · line 534

Quoted 4 lines
block/parallels.c +3 -0
+        error_setg(errp, "Cluster size is too large");
+        return -EINVAL;
+    }
      if (total_size >= MAX_PARALLELS_IMAGE_FACTOR * cl_size) {

since total_size still has to fit within off_t (63 bits, not 64)

block/parallels.c static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, · line 536

Quoted 3 lines
block/parallels.c +0 -0
          error_setg(errp, "Image size is too large for this cluster size");
          return -E2BIG;

With that change,

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#19 lore raw

PATCH 8/12 qemu-iotests: Test parallels image creation with QMP

In reply to Kevin Wolf
---
 tests/qemu-iotests/212     | 326 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/212.out | 111 +++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 438 insertions(+)
 create mode 100755 tests/qemu-iotests/212
 create mode 100644 tests/qemu-iotests/212.out
tests/qemu-iotests/212 new file +326 -0
new file mode 100755
index 0000000000..112a8420ce
@@ -0,0 +1,326 @@
+#!/bin/bash
+#
+# Test parallels and file image creation
+#
+# Copyright (C) 2018 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
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt parallels
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@"
+    $QEMU -nographic -qmp stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+                          | _filter_qemu | _filter_imgfmt \
+                          | _filter_actual_image_size
+}
+
+echo
+echo "=== Successful image creation (defaults) ==="
+echo
+
+size=$((128 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "blockdev-add",
+  "arguments": {
+      "driver": "file",
+      "node-name": "imgfile",
+      "filename": "$TEST_IMG"
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "imgfile",
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Successful image creation (explicit defaults) ==="
+echo
+
+# Choose a different size to show that we got a new image
+size=$((64 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "cluster-size": 1048576
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Successful image creation (with non-default options) ==="
+echo
+
+# Choose a different size to show that we got a new image
+size=$((32 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "cluster-size": 65536
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Invalid BlockdevRef ==="
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "this doesn't exist",
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Zero size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 0
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+echo
+echo "=== Maximum size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 4503599627369984
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. Misaligned image size
+# 2. 2^64 - 512
+# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 4. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)
+# 5. 2^52 (512 bytes more than maximum image size)
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 1234
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 18446744073709551104
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775808
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775296
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 4503599627370497
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Invalid cluster size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "cluster-size": 1234
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "cluster-size": 128
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "cluster-size": 4294967296
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "cluster-size": 9223372036854775808
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "cluster-size": 18446744073709551104
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "cluster-size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 281474976710656,
+      "cluster-size": 512
+  }
+}
+{ "execute": "quit" }
+EOF
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
tests/qemu-iotests/212.out new file +111 -0

111 lines not on this page · raw message

tests/qemu-iotests/group +1 -0
index 0d11d40aec..f706346f86 100644
@@ -210,3 +210,4 @@
 209 rw auto quick
 210 rw auto
 211 rw auto quick
+212 rw auto quick
Signature · 1 line
2.13.6
#20 lore raw

Re: [Qemu-devel] [PATCH for-2.12 08/12] qemu-iotests: Test parallels image creation with QMP

Quoted 13 lines · Kevin Wolf
---
  tests/qemu-iotests/212     | 326 +++++++++++++++++++++++++++++++++++++++++++++
  tests/qemu-iotests/212.out | 111 +++++++++++++++
  tests/qemu-iotests/group   |   1 +
  3 files changed, 438 insertions(+)

create mode 100755 tests/qemu-iotests/212
create mode 100644 tests/qemu-iotests/212.out

tests/qemu-iotests/212 new file +3 -0
+echo
+echo "=== Invalid sizes ==="
+echo

tests/qemu-iotests/212 line 214

Quoted 10 lines
tests/qemu-iotests/212 new file +10 -0
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. Misaligned image size
+# 2. 2^64 - 512
+# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 4. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)

Is this part of the comment stale copy-and-paste? There's no crypto header, and the real max is much smaller...

Quoted 7 lines
tests/qemu-iotests/212 new file +6 -0
+# 5. 2^52 (512 bytes more than maximum image size)
+
 
+echo
+echo "=== Invalid cluster size ==="
+echo
+
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#21 lore raw

Re: [Qemu-devel] [PATCH for-2.12 08/12] qemu-iotests: Test parallels image creation with QMP

Am 20.03.2018 um 19:42 hat Eric Blake geschrieben:

Quoted 11 lines · Kevin Wolf
Quoted 9 lines
---
  tests/qemu-iotests/212     | 326 +++++++++++++++++++++++++++++++++++++++++++++
  tests/qemu-iotests/212.out | 111 +++++++++++++++
  tests/qemu-iotests/group   |   1 +
  3 files changed, 438 insertions(+)

create mode 100755 tests/qemu-iotests/212
create mode 100644 tests/qemu-iotests/212.out

Quoted 16 lines

tests/qemu-iotests/212 line 214

Quoted 13 lines
tests/qemu-iotests/212 new file +13 -0
+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. Misaligned image size
+# 2. 2^64 - 512
+# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 4. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)

Is this part of the comment stale copy-and-paste? There's no crypto header, and the real max is much smaller...

Yeah... Not sure if the case is all that useful, but it can't hurt, so I'll just s/crypto header/image header/ rather than removing it.

Kevin

author
#22 lore raw

PATCH 9/12 vhdx: Require power-of-two block size on create

In reply to Kevin Wolf

Images with a non-power-of-two block size are invalid and cannot be opened. Reject such block sizes when creating an image.

---
 block/vhdx.c | 4 ++++
 1 file changed, 4 insertions(+)
block/vhdx.c +4 -0
index f1b97f4b49..ca211a3196 100644
@@ -1877,6 +1877,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
         error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 MB");
         return -EINVAL;
     }
+    if (!is_power_of_2(block_size)) {
+        error_setg(errp, "Block size must be a power of two");
+        return -EINVAL;
+    }
     if (block_size > VHDX_BLOCK_SIZE_MAX) {
         error_setg_errno(errp, EINVAL, "Block size must not exceed %d",
                          VHDX_BLOCK_SIZE_MAX);
Signature · 1 line
2.13.6
Reviewed-by on v1 9/12
#23 lore raw

Re: [Qemu-devel] [PATCH for-2.12 09/12] vhdx: Require power-of-two block size on create

Quoted 8 lines · Kevin Wolf

Images with a non-power-of-two block size are invalid and cannot be opened. Reject such block sizes when creating an image.

---
  block/vhdx.c | 4 ++++
  1 file changed, 4 insertions(+)
Quoted 16 lines
block/vhdx.c +4 -0
index f1b97f4b49..ca211a3196 100644
@@ -1877,6 +1877,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
          error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 MB");
          return -EINVAL;
      }
+    if (!is_power_of_2(block_size)) {
+        error_setg(errp, "Block size must be a power of two");
+        return -EINVAL;
+    }
      if (block_size > VHDX_BLOCK_SIZE_MAX) {
          error_setg_errno(errp, EINVAL, "Block size must not exceed %d",
                           VHDX_BLOCK_SIZE_MAX);
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Reviewed-by on v1 9/12
#24 lore raw

Re: [Qemu-devel] [PATCH for-2.12 09/12] vhdx: Require power-of-two block size on create

In reply to Kevin Wolf
Quoted 3 lines · Kevin Wolf

Images with a non-power-of-two block size are invalid and cannot be opened. Reject such block sizes when creating an image.

Good catch.

Quoted 23 lines
---
 block/vhdx.c | 4 ++++
 1 file changed, 4 insertions(+)
block/vhdx.c +4 -0
index f1b97f4b49..ca211a3196 100644
@@ -1877,6 +1877,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
         error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 MB");
         return -EINVAL;
     }
+    if (!is_power_of_2(block_size)) {
+        error_setg(errp, "Block size must be a power of two");
+        return -EINVAL;
+    }
     if (block_size > VHDX_BLOCK_SIZE_MAX) {
         error_setg_errno(errp, EINVAL, "Block size must not exceed %d",
                          VHDX_BLOCK_SIZE_MAX);
Signature · 1 line
2.13.6
author
#25 lore raw

PATCH 10/12 vhdx: Don't use error_setg_errno() with constant errno

In reply to Kevin Wolf

error_setg_errno() is meant for cases where we got an errno from the OS that can add useful extra information to an error message. It's pointless if we pass a constant errno, these cases should use plain error_setg().

---
 block/vhdx.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/vhdx.c +4 -5
index ca211a3196..0e48179b81 100644
@@ -1822,7 +1822,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
     /* Validate options and set default values */
     image_size = vhdx_opts->size;
     if (image_size > VHDX_MAX_IMAGE_SIZE) {
-        error_setg_errno(errp, EINVAL, "Image size too large; max of 64TB");
+        error_setg(errp, "Image size too large; max of 64TB");
         return -EINVAL;
     }
 
@@ -1832,7 +1832,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
         log_size = vhdx_opts->log_size;
     }
     if (log_size < MiB || (log_size % MiB) != 0) {
-        error_setg_errno(errp, EINVAL, "Log size must be a multiple of 1 MB");
+        error_setg(errp, "Log size must be a multiple of 1 MB");
         return -EINVAL;
     }
 
@@ -1874,7 +1874,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
     }
 
     if (block_size < MiB || (block_size % MiB) != 0) {
-        error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 MB");
+        error_setg(errp, "Block size must be a multiple of 1 MB");
         return -EINVAL;
     }
     if (!is_power_of_2(block_size)) {
@@ -1882,8 +1882,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
         return -EINVAL;
     }
     if (block_size > VHDX_BLOCK_SIZE_MAX) {
-        error_setg_errno(errp, EINVAL, "Block size must not exceed %d",
-                         VHDX_BLOCK_SIZE_MAX);
+        error_setg(errp, "Block size must not exceed %d", VHDX_BLOCK_SIZE_MAX);
         return -EINVAL;
     }
 
Signature · 1 line
2.13.6
Reviewed-by on v1 10/12
#26 lore raw

Re: [Qemu-devel] [PATCH for-2.12 10/12] vhdx: Don't use error_setg_errno() with constant errno

Quoted 4 lines · Kevin Wolf

error_setg_errno() is meant for cases where we got an errno from the OS that can add useful extra information to an error message. It's pointless if we pass a constant errno, these cases should use plain error_setg().

Well, it DOES let you get the libc-specific spelling of the strerror for that errno (not all libc report the same string) - but you're right that it didn't add much here.

Quoted 5 lines
---
  block/vhdx.c | 9 ++++-----
  1 file changed, 4 insertions(+), 5 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Reviewed-by on v1 10/12
#27 lore raw

Re: [Qemu-devel] [PATCH for-2.12 10/12] vhdx: Don't use error_setg_errno() with constant errno

In reply to Kevin Wolf
Quoted 54 lines · Kevin Wolf

error_setg_errno() is meant for cases where we got an errno from the OS that can add useful extra information to an error message. It's pointless if we pass a constant errno, these cases should use plain error_setg().

---
 block/vhdx.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/vhdx.c +4 -5
index ca211a3196..0e48179b81 100644
@@ -1822,7 +1822,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
     /* Validate options and set default values */
     image_size = vhdx_opts->size;
     if (image_size > VHDX_MAX_IMAGE_SIZE) {
-        error_setg_errno(errp, EINVAL, "Image size too large; max of 64TB");
+        error_setg(errp, "Image size too large; max of 64TB");
         return -EINVAL;
     }
 
@@ -1832,7 +1832,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
         log_size = vhdx_opts->log_size;
     }
     if (log_size < MiB || (log_size % MiB) != 0) {
-        error_setg_errno(errp, EINVAL, "Log size must be a multiple of 1 MB");
+        error_setg(errp, "Log size must be a multiple of 1 MB");
         return -EINVAL;
     }
 
@@ -1874,7 +1874,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
     }
 
     if (block_size < MiB || (block_size % MiB) != 0) {
-        error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 MB");
+        error_setg(errp, "Block size must be a multiple of 1 MB");
         return -EINVAL;
     }
     if (!is_power_of_2(block_size)) {
@@ -1882,8 +1882,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
         return -EINVAL;
     }
     if (block_size > VHDX_BLOCK_SIZE_MAX) {
-        error_setg_errno(errp, EINVAL, "Block size must not exceed %d",
-                         VHDX_BLOCK_SIZE_MAX);
+        error_setg(errp, "Block size must not exceed %d", VHDX_BLOCK_SIZE_MAX);
         return -EINVAL;
     }
 
Signature · 1 line
2.13.6
author
#28 lore raw

PATCH 11/12 vhdx: Check for 4 GB maximum log size on creation

In reply to Kevin Wolf

It's unclear what the real maximum is, but we use an uint32_t to store the log size in vhdx_co_create(), so we should check that the given value fits in 32 bits.

---
 block/vhdx.c | 4 ++++
 1 file changed, 4 insertions(+)
block/vhdx.c +4 -0
index 0e48179b81..a1a0302799 100644
@@ -1829,6 +1829,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
     if (!vhdx_opts->has_log_size) {
         log_size = DEFAULT_LOG_SIZE;
     } else {
+        if (vhdx_opts->log_size > UINT32_MAX) {
+            error_setg(errp, "Log size must be smaller than 4 GB");
+            return -EINVAL;
+        }
         log_size = vhdx_opts->log_size;
     }
     if (log_size < MiB || (log_size % MiB) != 0) {
Signature · 1 line
2.13.6
Reviewed-by on v1 11/12
#29 lore raw

Re: [Qemu-devel] [PATCH for-2.12 11/12] vhdx: Check for 4 GB maximum log size on creation

Quoted 8 lines · Kevin Wolf

It's unclear what the real maximum is, but we use an uint32_t to store the log size in vhdx_co_create(), so we should check that the given value fits in 32 bits.

---
  block/vhdx.c | 4 ++++
  1 file changed, 4 insertions(+)
Quoted 17 lines
block/vhdx.c +4 -0
index 0e48179b81..a1a0302799 100644
@@ -1829,6 +1829,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
      if (!vhdx_opts->has_log_size) {
          log_size = DEFAULT_LOG_SIZE;
      } else {
+        if (vhdx_opts->log_size > UINT32_MAX) {
+            error_setg(errp, "Log size must be smaller than 4 GB");
+            return -EINVAL;
+        }
          log_size = vhdx_opts->log_size;
      }
      if (log_size < MiB || (log_size % MiB) != 0) {
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Reviewed-by on v1 11/12
#30 lore raw

Re: [Qemu-devel] [PATCH for-2.12 11/12] vhdx: Check for 4 GB maximum log size on creation

In reply to Kevin Wolf
Quoted 4 lines · Kevin Wolf

It's unclear what the real maximum is, but we use an uint32_t to store the log size in vhdx_co_create(), so we should check that the given value fits in 32 bits.

It's a uint32 in the on-disk header per spec, so I agree the implied max is UINT32_MAX

Quoted 23 lines
---
 block/vhdx.c | 4 ++++
 1 file changed, 4 insertions(+)
block/vhdx.c +4 -0
index 0e48179b81..a1a0302799 100644
@@ -1829,6 +1829,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
     if (!vhdx_opts->has_log_size) {
         log_size = DEFAULT_LOG_SIZE;
     } else {
+        if (vhdx_opts->log_size > UINT32_MAX) {
+            error_setg(errp, "Log size must be smaller than 4 GB");
+            return -EINVAL;
+        }
         log_size = vhdx_opts->log_size;
     }
     if (log_size < MiB || (log_size % MiB) != 0) {
Signature · 1 line
2.13.6
author
#31 lore raw

PATCH 12/12 qemu-iotests: Test vhdx image creation with QMP

In reply to Kevin Wolf
---
 tests/qemu-iotests/213     | 349 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/213.out | 121 ++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 471 insertions(+)
 create mode 100755 tests/qemu-iotests/213
 create mode 100644 tests/qemu-iotests/213.out
tests/qemu-iotests/213 new file +349 -0
new file mode 100755
index 0000000000..7e44accb06
@@ -0,0 +1,349 @@
+#!/bin/bash
+#
+# Test vhdx and file image creation
+#
+# Copyright (C) 2018 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
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt vhdx
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@"
+    $QEMU -nographic -qmp stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+                          | _filter_qemu | _filter_imgfmt \
+                          | _filter_actual_image_size
+}
+
+echo
+echo "=== Successful image creation (defaults) ==="
+echo
+
+size=$((128 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "blockdev-add",
+  "arguments": {
+      "driver": "file",
+      "node-name": "imgfile",
+      "filename": "$TEST_IMG"
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "imgfile",
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Successful image creation (explicit defaults) ==="
+echo
+
+# Choose a different size to show that we got a new image
+size=$((64 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "log-size": 1048576,
+      "block-size": 8388608,
+      "subformat": "dynamic",
+      "block-state-zero": true
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Successful image creation (with non-default options) ==="
+echo
+
+# Choose a different size to show that we got a new image
+size=$((32 * 1024 * 1024))
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG",
+      "size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "log-size": 8388608,
+      "block-size": 268435456,
+      "subformat": "fixed",
+      "block-state-zero": false
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Invalid BlockdevRef ==="
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "this doesn't exist",
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Zero size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 0
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+echo
+echo "=== Maximum size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 70368744177664
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)
+# 4. 2^46 + 1 (512 bytes more than maximum image size)
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 18446744073709551104
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775808
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775296
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 70368744177665
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Invalid block size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "block-size": 1234567
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "block-size": 128
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "block-size": 3145728
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "block-size": 536870912
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "block-size": 0
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Invalid log size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "log-size": 1234567
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "log-size": 128
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "log-size": 4294967296
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "log-size": 0
+  }
+}
+{ "execute": "quit" }
+EOF
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
tests/qemu-iotests/213.out new file +121 -0

121 lines not on this page · raw message

tests/qemu-iotests/group +1 -0
index f706346f86..52a80f3f9e 100644
@@ -211,3 +211,4 @@
 210 rw auto
 211 rw auto quick
 212 rw auto quick
+213 rw auto quick
Signature · 1 line
2.13.6
#32 lore raw

Re: [Qemu-devel] [PATCH for-2.12 12/12] qemu-iotests: Test vhdx image creation with QMP

Quoted 13 lines · Kevin Wolf
---
  tests/qemu-iotests/213     | 349 +++++++++++++++++++++++++++++++++++++++++++++
  tests/qemu-iotests/213.out | 121 ++++++++++++++++
  tests/qemu-iotests/group   |   1 +
  3 files changed, 471 insertions(+)

create mode 100755 tests/qemu-iotests/213
create mode 100644 tests/qemu-iotests/213.out

tests/qemu-iotests/213 new file +3 -0
+
+echo
+echo "=== Invalid sizes ==="

tests/qemu-iotests/213 line 219

Quoted 10 lines
tests/qemu-iotests/213 new file +10 -0
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)

Same comments as before on whether this comment is slightly stale after copy-and-paste.

Quoted 1 line

+# 4. 2^46 + 1 (512 bytes more than maximum image size)

Does this image format require 512-byte alignment? If so, are you missing a test of unaligned sizes? If not, why not just 1 byte more than the maximum?

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#33 lore raw

Re: [Qemu-devel] [PATCH for-2.12 12/12] qemu-iotests: Test vhdx image creation with QMP

Am 20.03.2018 um 19:53 hat Eric Blake geschrieben:

Quoted 11 lines · Kevin Wolf
Quoted 9 lines
---
  tests/qemu-iotests/213     | 349 +++++++++++++++++++++++++++++++++++++++++++++
  tests/qemu-iotests/213.out | 121 ++++++++++++++++
  tests/qemu-iotests/group   |   1 +
  3 files changed, 471 insertions(+)

create mode 100755 tests/qemu-iotests/213
create mode 100644 tests/qemu-iotests/213.out

Quoted 16 lines

tests/qemu-iotests/213 line 219

Quoted 13 lines
tests/qemu-iotests/213 new file +13 -0
+
+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)

Same comments as before on whether this comment is slightly stale after copy-and-paste.

Will do the same thing as there ("image header").

Quoted 4 lines
Quoted 1 line

+# 4. 2^46 + 1 (512 bytes more than maximum image size)

Does this image format require 512-byte alignment? If so, are you missing a test of unaligned sizes? If not, why not just 1 byte more than the maximum?

The comment is wrong, the code already does just 1 byte more.

Kevin