Skip to contentSkip to conversation

x-blockdev-create for protocols and qcow2

PATCH Superseded Kevin Wolf 3 participants 69 messages Feb 8, 2018 – Feb 21, 2018 Block layer

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

cover x-blockdev-create for protocols and qcow2

This series implements a minimal QMP command that allows to create an image file on the protocol level or an image format on a given block node.

Eventually, the interface is going to change to some kind of an async command (possibly a (non-)block job), but that will require more work on the job infrastructure first, so let's first QAPIfy image creation in the block drivers. In this series, I'm going for a synchronous command that is prefixed with x- for now.

This series converts qcow2 and all protocol drivers that allow an actual image creation. This means that drivers which only check if the already existing storage is good enough are not converted (e.g. host_device, iscsi). The old behaviour was useful because 'qemu-img create' wants to create both protocol and format layer, but with the separation in QMP, you can just leave out the protocol layer creation when the device already exists.

Please note that for some of the protocol drivers (gluster, rbd and sheepdog) I don't have a test setup ready. For those, I only tested with a fake server address to check that the option are parsed correctly up to this point and an appropriate error is returned without crashing.

If you are a maintainer of one of these protocols and you are interested in keeping image creation working for your protocol, you probably want to test this series on a real setup and give me some feedback. If you don't, I'll just merge the patches and hope that they won't break anything.

Kevin Wolf (27):
block/qapi: Introduce BlockdevCreateOptions block/qapi: Add qcow2 create options to schema
qcow2: Let qcow2_create() handle protocol layer
qcow2: Pass BlockdevCreateOptions to qcow2_create2()
qcow2: Use BlockdevRef in qcow2_create2()
qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()
qcow2: Handle full/falloc preallocation in qcow2_create2()
util: Add qemu_opts_to_qdict_filtered()
qdict: Introduce qdict_rename_keys()
qcow2: Use visitor for options in qcow2_create()
block: x-blockdev-create QMP command
file-posix: Support .bdrv_co_create
file-win32: Support .bdrv_co_create
gluster: Support .bdrv_co_create
rbd: Support .bdrv_co_create
nfs: Use QAPI options in nfs_client_open()
nfs: Support .bdrv_co_create
sheepdog: QAPIfy "redundacy" create option
sheepdog: Support .bdrv_co_create
ssh: Use QAPI BlockdevOptionsSsh object
ssh: QAPIfy host-key-check option
ssh: Pass BlockdevOptionsSsh to connect_to_ssh()
ssh: Support .bdrv_co_create
file-posix: Fix no-op bdrv_truncate() with falloc preallocation
block: Fail bdrv_truncate() with negative size
qemu-iotests: Test qcow2 over file image creation with QMP
qemu-iotests: Test ssh image creation over QMP

 qapi/block-core.json       | 326 ++++++++++++++++++++++++++++++++-
 include/block/block.h      |   2 +
 include/block/block_int.h  |   2 +
 include/qapi/qmp/qdict.h   |   6 +
 include/qemu/option.h      |   2 +
 block.c                    |  54 +++++-
 block/create.c             |  75 ++++++++
 block/file-posix.c         |  91 +++++++---
 block/file-win32.c         |  45 ++++-
 block/gluster.c            | 149 ++++++++++------
 block/nfs.c                | 236 +++++++++++-------------
 block/qcow2.c              | 386 +++++++++++++++++++++++++--------------
 block/rbd.c                | 137 +++++++++-----
 block/sheepdog.c           | 286 ++++++++++++++++++++---------
 block/ssh.c                | 298 +++++++++++++++++--------------
 qobject/qdict.c            |  30 ++++
 util/qemu-option.c         |  37 +++-
 block/Makefile.objs        |   2 +-
 tests/qemu-iotests/049.out |   8 +-
 tests/qemu-iotests/112.out |   4 +-
 tests/qemu-iotests/206     | 436 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/206.out | 209 ++++++++++++++++++++++
 tests/qemu-iotests/207     | 261 +++++++++++++++++++++++++++
 tests/qemu-iotests/207.out |  75 ++++++++
 tests/qemu-iotests/group   |   2 +
 25 files changed, 2525 insertions(+), 634 deletions(-)
 create mode 100644 block/create.c
 create mode 100755 tests/qemu-iotests/206
 create mode 100644 tests/qemu-iotests/206.out
 create mode 100755 tests/qemu-iotests/207
 create mode 100644 tests/qemu-iotests/207.out
Signature · 1 line
2.13.6
author
#1 lore raw

PATCH 1/27 block/qapi: Introduce BlockdevCreateOptions

This creates a BlockdevCreateOptions union type that will contain all of the options for image creation. We'll start out with an empty struct type BlockdevCreateDummy for all drivers.

---
 qapi/block-core.json | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
qapi/block-core.json +61 -0
index 8225308904..b59b1430bb 100644
@@ -3338,6 +3338,67 @@
 { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
 ##
+# @BlockdevCreateNotSupported:
+#
+# This is used for all drivers that don't support creating images.
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateNotSupported', 'data': {}}
+
+##
+# @BlockdevCreateOptions:
+#
+# Options for creating an image format on a given node.
+#
+# @driver           block driver to create the image format
+#
+# Since: 2.12
+##
+{ 'union': 'BlockdevCreateOptions',
+  'base': {
+      'driver':         'BlockdevDriver' },
+  'discriminator': 'driver',
+  'data': {
+      'blkdebug':       'BlockdevCreateNotSupported',
+      'blkverify':      'BlockdevCreateNotSupported',
+      'bochs':          'BlockdevCreateNotSupported',
+      'cloop':          'BlockdevCreateNotSupported',
+      'dmg':            'BlockdevCreateNotSupported',
+      'file':           'BlockdevCreateNotSupported',
+      'ftp':            'BlockdevCreateNotSupported',
+      'ftps':           'BlockdevCreateNotSupported',
+      'gluster':        'BlockdevCreateNotSupported',
+      'host_cdrom':     'BlockdevCreateNotSupported',
+      'host_device':    'BlockdevCreateNotSupported',
+      'http':           'BlockdevCreateNotSupported',
+      'https':          'BlockdevCreateNotSupported',
+      'iscsi':          'BlockdevCreateNotSupported',
+      'luks':           'BlockdevCreateNotSupported',
+      'nbd':            'BlockdevCreateNotSupported',
+      'nfs':            'BlockdevCreateNotSupported',
+      'null-aio':       'BlockdevCreateNotSupported',
+      'null-co':        'BlockdevCreateNotSupported',
+      'parallels':      'BlockdevCreateNotSupported',
+      'qcow2':          'BlockdevCreateNotSupported',
+      'qcow':           'BlockdevCreateNotSupported',
+      'qed':            'BlockdevCreateNotSupported',
+      'quorum':         'BlockdevCreateNotSupported',
+      'raw':            'BlockdevCreateNotSupported',
+      'rbd':            'BlockdevCreateNotSupported',
+      'replication':    'BlockdevCreateNotSupported',
+      'sheepdog':       'BlockdevCreateNotSupported',
+      'ssh':            'BlockdevCreateNotSupported',
+      'throttle':       'BlockdevCreateNotSupported',
+      'vdi':            'BlockdevCreateNotSupported',
+      'vhdx':           'BlockdevCreateNotSupported',
+      'vmdk':           'BlockdevCreateNotSupported',
+      'vpc':            'BlockdevCreateNotSupported',
+      'vvfat':          'BlockdevCreateNotSupported',
+      'vxhs':           'BlockdevCreateNotSupported'
+  } }
+
+##
 # @blockdev-open-tray:
 #
 # Opens a block device's tray. If there is a block driver state tree inserted as
Signature · 1 line
2.13.6
Reviewed-by on v1 1/27
#2 lore raw

Re: [Qemu-devel] [PATCH 01/27] block/qapi: Introduce BlockdevCreateOptions

Quoted 3 lines · Kevin Wolf

This creates a BlockdevCreateOptions union type that will contain all of the options for image creation. We'll start out with an empty struct type BlockdevCreateDummy for all drivers.

Name here...

Quoted 11 lines
---
  qapi/block-core.json | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 61 insertions(+)
qapi/block-core.json +0 -0
index 8225308904..b59b1430bb 100644

qapi/block-core.json line 3341

Quoted 4 lines
qapi/block-core.json +1 -0
@@ -3338,6 +3338,67 @@
  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
  ##
+# @BlockdevCreateNotSupported:

...should match what you use here.

With that obvious fix,

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

Re: [Qemu-devel] [PATCH 01/27] block/qapi: Introduce BlockdevCreateOptions

In reply to Kevin Wolf
Quoted 8 lines · Kevin Wolf

This creates a BlockdevCreateOptions union type that will contain all of the options for image creation. We'll start out with an empty struct type BlockdevCreateDummy for all drivers.

---
 qapi/block-core.json | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

With the commit message fixed:

author
#4 lore raw

PATCH 2/27 block/qapi: Add qcow2 create options to schema

In reply to Kevin Wolf
---
 qapi/block-core.json | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)
qapi/block-core.json +44 -1
index b59b1430bb..aade602a04 100644
@@ -3338,6 +3338,49 @@
 { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
 ##
+# @BlockdevQcow2Version:
+#
+# @v2:  The original QCOW2 format as introduced in qemu 0.10 (version 2)
+# @v3:  The extended QCOW2 format as introduced in qemu 1.1 (version 3)
+#
+# Since: 2.12
+##
+{ 'enum': 'BlockdevQcow2Version',
+  'data': [ 'v2', 'v3' ] }
+
+
+##
+# @BlockdevCreateOptionsQcow2:
+#
+# Driver specific image creation options for qcow2.
+#
+# @file             Node to create the image format on
+# @size             Size of the virtual disk in bytes
+# @version          Compatibility level (default: v3)
+# @backing-file     File name of the backing file if a backing file
+#                   should be used
+# @backing-fmt      Name of the block driver to use for the backing file
+# @encrypt          Encryption options if the image should be encrypted
+# @cluster-size     qcow2 cluster size in bytes (default: 65536)
+# @preallocation    Preallocation mode for the new image (default: off)
+# @lazy-refcounts   True if refcounts may be updated lazily (default: off)
+# @refcount-bits    Width of reference counts in bits (default: 16)
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsQcow2',
+  'data': { 'file':             'BlockdevRef',
+            'size':             'size',
+            '*version':         'BlockdevQcow2Version',
+            '*backing-file':    'str',
+            '*backing-fmt':     'BlockdevDriver',
+            '*encrypt':         'QCryptoBlockCreateOptions',
+            '*cluster-size':    'size',
+            '*preallocation':   'PreallocMode',
+            '*lazy-refcounts':  'bool',
+            '*refcount-bits':   'int' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.
@@ -3380,7 +3423,7 @@
       'null-aio':       'BlockdevCreateNotSupported',
       'null-co':        'BlockdevCreateNotSupported',
       'parallels':      'BlockdevCreateNotSupported',
-      'qcow2':          'BlockdevCreateNotSupported',
+      'qcow2':          'BlockdevCreateOptionsQcow2',
       'qcow':           'BlockdevCreateNotSupported',
       'qed':            'BlockdevCreateNotSupported',
       'quorum':         'BlockdevCreateNotSupported',
Signature · 1 line
2.13.6
Reviewed-by on v1 2/27
#5 lore raw

Re: [Qemu-devel] [PATCH 02/27] block/qapi: Add qcow2 create options to schema

Quoted 12 lines · Kevin Wolf
---
  qapi/block-core.json | 45 ++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 44 insertions(+), 1 deletion(-)
qapi/block-core.json +0 -0
index b59b1430bb..aade602a04 100644
@@ -3338,6 +3338,49 @@
  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 

qapi/block-core.json line 3349

Quoted 10 lines
qapi/block-core.json +9 -0
  ##
+# @BlockdevQcow2Version:
+#
+# @v2:  The original QCOW2 format as introduced in qemu 0.10 (version 2)
+# @v3:  The extended QCOW2 format as introduced in qemu 1.1 (version 3)
+#
+# Since: 2.12
+##
+{ 'enum': 'BlockdevQcow2Version',
+  'data': [ 'v2', 'v3' ] }

Yes, I think that turns out nicer.

qapi/block-core.json line 3361

Quoted 12 lines
qapi/block-core.json +12 -0
+
+
+##
+# @BlockdevCreateOptionsQcow2:
+#
+# Driver specific image creation options for qcow2.
+#
+# @file             Node to create the image format on
+# @size             Size of the virtual disk in bytes
+# @version          Compatibility level (default: v3)
+# @backing-file     File name of the backing file if a backing file
+#                   should be used

Do we need an explicit comment that the QMP command does NOT open this file (so much as this being the string recorded in the metadata header), but that the caller is expected to later connect a node visiting the backing file to this node after the create has completed? Or is that more for adding later in the QMP command that uses the overall union type (as the same rule will apply to multiple formats, not just qcow2 - the create operation is NOT creating the backing chain, but just formatting one element that will later be hooked into the chain).

Quoted 10 lines
qapi/block-core.json +10 -0
+# @backing-fmt      Name of the block driver to use for the backing file
+# @encrypt          Encryption options if the image should be encrypted
+# @cluster-size     qcow2 cluster size in bytes (default: 65536)
+# @preallocation    Preallocation mode for the new image (default: off)
+# @lazy-refcounts   True if refcounts may be updated lazily (default: off)
+# @refcount-bits    Width of reference counts in bits (default: 16)
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsQcow2',

qapi/block-core.json line 3381

Quoted 10 lines
qapi/block-core.json +10 -0
+  'data': { 'file':             'BlockdevRef',
+            'size':             'size',
+            '*version':         'BlockdevQcow2Version',
+            '*backing-file':    'str',
+            '*backing-fmt':     'BlockdevDriver',
+            '*encrypt':         'QCryptoBlockCreateOptions',
+            '*cluster-size':    'size',
+            '*preallocation':   'PreallocMode',
+            '*lazy-refcounts':  'bool',
+            '*refcount-bits':   'int' } }

Looks reasonable, and matches what qemu-img create -f qcow2 can do (the obvious differences of '-' in QMP instead of '_' in qemu-img -o spelling is fine).

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 3/27 qcow2: Let qcow2_create() handle protocol layer

In reply to Kevin Wolf

Currently, qcow2_create() only parses the QemuOpts and then calls qcow2_create2() for the actual image creation, which includes both the creation of the actual file on the file system and writing a valid empty qcow2 image into that file.

The plan is that qcow2_create2() becomes the function that implements the functionality for a future 'blockdev-create' QMP command, which only creates the qcow2 layer on an already opened file node.

This is a first step towards that goal: Let's move out anything that deals with the protocol layer from qcow2_create2() into qcow2_create(). This means that qcow2_create2() doesn't need a file name any more.

---
 block/qcow2.c | 64 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 26 deletions(-)
block/qcow2.c +38 -26
index 1f80961e1b..6134c0d40c 100644
@@ -2698,7 +2698,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
     return refcount_bits;
 }
 
-static int qcow2_create2(const char *filename, int64_t total_size,
+static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
                          const char *backing_file, const char *backing_format,
                          int flags, size_t cluster_size, PreallocMode prealloc,
                          QemuOpts *opts, int version, int refcount_order,
@@ -2724,28 +2724,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     Error *local_err = NULL;
     int ret;
 
-    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
-        int64_t prealloc_size =
-            qcow2_calc_prealloc_size(total_size, cluster_size, refcount_order);
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
-        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc),
-                     &error_abort);
-    }
-
-    ret = bdrv_create_file(filename, opts, &local_err);
+    blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+    ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
-        error_propagate(errp, local_err);
-        return ret;
-    }
-
-    blk = blk_new_open(filename, NULL, NULL,
-                       BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
-                       &local_err);
-    if (blk == NULL) {
-        error_propagate(errp, local_err);
-        return -EIO;
+        goto out;
     }
-
     blk_set_allow_write_beyond_eof(blk, true);
 
     /* Write the header */
@@ -2800,7 +2783,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      */
     options = qdict_new();
     qdict_put_str(options, "driver", "qcow2");
-    blk = blk_new_open(filename, NULL, options,
+    qdict_put_str(options, "file", bs->node_name);
+    blk = blk_new_open(NULL, NULL, options,
                        BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_NO_FLUSH,
                        &local_err);
     if (blk == NULL) {
@@ -2872,7 +2856,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      */
     options = qdict_new();
     qdict_put_str(options, "driver", "qcow2");
-    blk = blk_new_open(filename, NULL, options,
+    qdict_put_str(options, "file", bs->node_name);
+    blk = blk_new_open(NULL, NULL, options,
                        BDRV_O_RDWR | BDRV_O_NO_BACKING | BDRV_O_NO_IO,
                        &local_err);
     if (blk == NULL) {
@@ -2902,6 +2887,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     uint64_t refcount_bits;
     int refcount_order;
     char *encryptfmt = NULL;
+    BlockDriverState *bs = NULL;
     Error *local_err = NULL;
     int ret;
 
@@ -2970,12 +2956,38 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
     refcount_order = ctz32(refcount_bits);
 
-    ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
+    /* Create and open the file (protocol layer) */
+    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
+        int64_t prealloc_size =
+            qcow2_calc_prealloc_size(size, cluster_size, refcount_order);
+        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
+        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc),
+                     &error_abort);
+    }
+
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
+        goto finish;
+    }
+
+    bs = bdrv_open(filename, NULL, NULL,
+                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    if (bs == NULL) {
+        ret = -EIO;
+        goto finish;
+    }
+
+    /* Create the qcow2 image (format layer) */
+    ret = qcow2_create2(bs, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, opts, version, refcount_order,
-                        encryptfmt, &local_err);
-    error_propagate(errp, local_err);
+                        encryptfmt, errp);
+    if (ret < 0) {
+        goto finish;
+    }
 
 finish:
+    bdrv_unref(bs);
+
     g_free(backing_file);
     g_free(backing_fmt);
     g_free(encryptfmt);
Signature · 1 line
2.13.6
Reviewed-by on v1 3/27
#8 lore raw

Re: [Qemu-devel] [PATCH 03/27] qcow2: Let qcow2_create() handle protocol layer

Quoted 13 lines · Kevin Wolf

Currently, qcow2_create() only parses the QemuOpts and then calls qcow2_create2() for the actual image creation, which includes both the creation of the actual file on the file system and writing a valid empty qcow2 image into that file.

The plan is that qcow2_create2() becomes the function that implements the functionality for a future 'blockdev-create' QMP command, which only creates the qcow2 layer on an already opened file node.

This is a first step towards that goal: Let's move out anything that deals with the protocol layer from qcow2_create2() into qcow2_create(). This means that qcow2_create2() doesn't need a file name any more.

Quoted 5 lines
---
 block/qcow2.c | 64 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 26 deletions(-)
author
#9 lore raw

PATCH 4/27 qcow2: Pass BlockdevCreateOptions to qcow2_create2()

In reply to Kevin Wolf

All of the simple options are now passed to qcow2_create2() in a BlockdevCreateOptions object. Still missing: node-name and the encryption options.

---
 block/qcow2.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 152 insertions(+), 38 deletions(-)
block/qcow2.c +152 -38
index 6134c0d40c..4ab6ed15c2 100644
@@ -2638,19 +2638,26 @@ static int64_t qcow2_calc_prealloc_size(int64_t total_size,
     return meta_size + aligned_total_size;
 }
 
-static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp)
+static bool validate_cluster_size(size_t cluster_size, Error **errp)
 {
-    size_t cluster_size;
-    int cluster_bits;
-
-    cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
-                                         DEFAULT_CLUSTER_SIZE);
-    cluster_bits = ctz32(cluster_size);
+    int cluster_bits = ctz32(cluster_size);
     if (cluster_bits < MIN_CLUSTER_BITS || cluster_bits > MAX_CLUSTER_BITS ||
         (1 << cluster_bits) != cluster_size)
     {
         error_setg(errp, "Cluster size must be a power of two between %d and "
                    "%dk", 1 << MIN_CLUSTER_BITS, 1 << (MAX_CLUSTER_BITS - 10));
+        return false;
+    }
+    return true;
+}
+
+static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp)
+{
+    size_t cluster_size;
+
+    cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
+                                         DEFAULT_CLUSTER_SIZE);
+    if (!validate_cluster_size(cluster_size, errp)) {
         return 0;
     }
     return cluster_size;
@@ -2698,12 +2705,11 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
     return refcount_bits;
 }
 
-static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
-                         const char *backing_file, const char *backing_format,
-                         int flags, size_t cluster_size, PreallocMode prealloc,
-                         QemuOpts *opts, int version, int refcount_order,
-                         const char *encryptfmt, Error **errp)
+static int qcow2_create2(BlockDriverState *bs,
+                         BlockdevCreateOptions *create_options,
+                         QemuOpts *opts, const char *encryptfmt, Error **errp)
 {
+    BlockdevCreateOptionsQcow2 *qcow2_opts;
     QDict *options;
 
     /*
@@ -2720,10 +2726,92 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
      */
     BlockBackend *blk;
     QCowHeader *header;
+    size_t cluster_size;
+    int version;
+    int refcount_order;
     uint64_t* refcount_table;
     Error *local_err = NULL;
     int ret;
 
+    /* Validate options and set default values */
+    assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
+    qcow2_opts = &create_options->u.qcow2;
+
+    if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
+        error_setg(errp, "Image size must be a multiple of 512 bytes");
+        ret = -EINVAL;
+        goto out;
+    }
+
+    if (qcow2_opts->has_version) {
+        switch (qcow2_opts->version) {
+        case BLOCKDEV_QCOW2_VERSION_V2:
+            version = 2;
+            break;
+        case BLOCKDEV_QCOW2_VERSION_V3:
+            version = 3;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+    } else {
+        version = 3;
+    }
+
+    if (qcow2_opts->has_cluster_size) {
+        cluster_size = qcow2_opts->cluster_size;
+    } else {
+        cluster_size = DEFAULT_CLUSTER_SIZE;
+    }
+
+    if (!validate_cluster_size(cluster_size, errp)) {
+        return -EINVAL;
+    }
+
+    if (!qcow2_opts->has_preallocation) {
+        qcow2_opts->preallocation = PREALLOC_MODE_OFF;
+    }
+    if (qcow2_opts->has_backing_file &&
+        qcow2_opts->preallocation != PREALLOC_MODE_OFF)
+    {
+        error_setg(errp, "Backing file and preallocation cannot be used at "
+                   "the same time");
+        return -EINVAL;
+    }
+    if (qcow2_opts->has_backing_fmt && !qcow2_opts->has_backing_file) {
+        error_setg(errp, "Backing format cannot be used without backing file");
+        return -EINVAL;
+    }
+
+    if (!qcow2_opts->has_lazy_refcounts) {
+        qcow2_opts->lazy_refcounts = false;
+    }
+    if (version < 3 && qcow2_opts->lazy_refcounts) {
+        error_setg(errp, "Lazy refcounts only supported with compatibility "
+                   "level 1.1 and above (use compat=1.1 or greater)");
+        return -EINVAL;
+    }
+
+    if (!qcow2_opts->has_refcount_bits) {
+        qcow2_opts->refcount_bits = 16;
+    }
+    if (qcow2_opts->refcount_bits > 64 ||
+        !is_power_of_2(qcow2_opts->refcount_bits))
+    {
+        error_setg(errp, "Refcount width must be a power of two and may not "
+                   "exceed 64 bits");
+        return -EINVAL;
+    }
+    if (version < 3 && qcow2_opts->refcount_bits != 16) {
+        error_setg(errp, "Different refcount widths than 16 bits require "
+                   "compatibility level 1.1 or above (use compat=1.1 or "
+                   "greater)");
+        return -EINVAL;
+    }
+    refcount_order = ctz32(qcow2_opts->refcount_bits);
+
+
+    /* Create BlockBackend to write to the image */
     blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
     ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
@@ -2750,7 +2838,7 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
     /* We'll update this to correct value later */
     header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
 
-    if (flags & BLOCK_FLAG_LAZY_REFCOUNTS) {
+    if (qcow2_opts->lazy_refcounts) {
         header->compatible_features |=
             cpu_to_be64(QCOW2_COMPAT_LAZY_REFCOUNTS);
     }
@@ -2812,18 +2900,26 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
     }
 
     /* Okay, now that we have a valid image, let's give it the right size */
-    ret = blk_truncate(blk, total_size, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate(blk, qcow2_opts->size, PREALLOC_MODE_OFF, errp);
     if (ret < 0) {
         error_prepend(errp, "Could not resize image: ");
         goto out;
     }
 
     /* Want a backing file? There you go.*/
-    if (backing_file) {
-        ret = bdrv_change_backing_file(blk_bs(blk), backing_file, backing_format);
+    if (qcow2_opts->has_backing_file) {
+        const char *backing_format = NULL;
+
+        if (qcow2_opts->has_backing_fmt) {
+            backing_format = BlockdevDriver_str(qcow2_opts->backing_fmt);
+        }
+
+        ret = bdrv_change_backing_file(blk_bs(blk), qcow2_opts->backing_file,
+                                       backing_format);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not assign backing file '%s' "
-                             "with format '%s'", backing_file, backing_format);
+                             "with format '%s'", qcow2_opts->backing_file,
+                             backing_format);
             goto out;
         }
     }
@@ -2837,8 +2933,8 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
     }
 
     /* And if we're supposed to preallocate metadata, do that now */
-    if (prealloc != PREALLOC_MODE_OFF) {
-        ret = preallocate(blk_bs(blk), 0, total_size);
+    if (qcow2_opts->preallocation != PREALLOC_MODE_OFF) {
+        ret = preallocate(blk_bs(blk), 0, qcow2_opts->size);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not preallocate metadata");
             goto out;
@@ -2876,8 +2972,10 @@ out:
 
 static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 {
+    BlockdevCreateOptions create_options;
     char *backing_file = NULL;
     char *backing_fmt = NULL;
+    BlockdevDriver backing_drv;
     char *buf = NULL;
     uint64_t size = 0;
     int flags = 0;
@@ -2885,7 +2983,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     PreallocMode prealloc;
     int version;
     uint64_t refcount_bits;
-    int refcount_order;
     char *encryptfmt = NULL;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
@@ -2896,6 +2993,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
                     BDRV_SECTOR_SIZE);
     backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
     backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
+    backing_drv = qapi_enum_parse(&BlockdevDriver_lookup, backing_fmt,
+                                  0, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto finish;
+    }
     encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
     if (encryptfmt) {
         if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
@@ -2933,20 +3037,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
     }
 
-    if (backing_file && prealloc != PREALLOC_MODE_OFF) {
-        error_setg(errp, "Backing file and preallocation cannot be used at "
-                   "the same time");
-        ret = -EINVAL;
-        goto finish;
-    }
-
-    if (version < 3 && (flags & BLOCK_FLAG_LAZY_REFCOUNTS)) {
-        error_setg(errp, "Lazy refcounts only supported with compatibility "
-                   "level 1.1 and above (use compat=1.1 or greater)");
-        ret = -EINVAL;
-        goto finish;
-    }
-
     refcount_bits = qcow2_opt_get_refcount_bits_del(opts, version, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -2954,10 +3044,10 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         goto finish;
     }
 
-    refcount_order = ctz32(refcount_bits);
 
     /* Create and open the file (protocol layer) */
     if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
+        int refcount_order = ctz32(refcount_bits);
         int64_t prealloc_size =
             qcow2_calc_prealloc_size(size, cluster_size, refcount_order);
         qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
@@ -2978,9 +3068,33 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     }
 
     /* Create the qcow2 image (format layer) */
-    ret = qcow2_create2(bs, size, backing_file, backing_fmt, flags,
-                        cluster_size, prealloc, opts, version, refcount_order,
-                        encryptfmt, errp);
+    create_options = (BlockdevCreateOptions) {
+        .driver         = BLOCKDEV_DRIVER_QCOW2,
+        .u.qcow2        = {
+            .file               = &(BlockdevRef) {
+                .type               = QTYPE_QSTRING,
+                .u.reference        = bs->node_name,
+            },
+            .size               = size,
+            .has_version        = true,
+            .version            = version == 2
+                                  ? BLOCKDEV_QCOW2_VERSION_V2
+                                  : BLOCKDEV_QCOW2_VERSION_V3,
+            .has_backing_file   = (backing_file != NULL),
+            .backing_file       = backing_file,
+            .has_backing_fmt    = (backing_fmt != NULL),
+            .backing_fmt        = backing_drv,
+            .has_cluster_size   = true,
+            .cluster_size       = cluster_size,
+            .has_preallocation  = true,
+            .preallocation      = prealloc,
+            .has_lazy_refcounts = true,
+            .lazy_refcounts     = (flags & BLOCK_FLAG_LAZY_REFCOUNTS),
+            .has_refcount_bits  = true,
+            .refcount_bits      = refcount_bits,
+        },
+    };
+    ret = qcow2_create2(bs, &create_options, opts, encryptfmt, errp);
     if (ret < 0) {
         goto finish;
     }
Signature · 1 line
2.13.6
#10 lore raw

Re: [Qemu-devel] [PATCH 04/27] qcow2: Pass BlockdevCreateOptions to qcow2_create2()

Quoted 12 lines · Kevin Wolf

All of the simple options are now passed to qcow2_create2() in a BlockdevCreateOptions object. Still missing: node-name and the encryption options.

---
  block/qcow2.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++------------
  1 file changed, 152 insertions(+), 38 deletions(-)
block/qcow2.c +1 -1
-static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp)
+static bool validate_cluster_size(size_t cluster_size, Error **errp)

block/qcow2.c static int64_t qcow2_calc_prealloc_size(int64_t total_size, · line 2645

Quoted 10 lines
block/qcow2.c +1 -6
  {
-    size_t cluster_size;
-    int cluster_bits;
-
-    cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
-                                         DEFAULT_CLUSTER_SIZE);
-    cluster_bits = ctz32(cluster_size);
+    int cluster_bits = ctz32(cluster_size);
      if (cluster_bits < MIN_CLUSTER_BITS || cluster_bits > MAX_CLUSTER_BITS ||
          (1 << cluster_bits) != cluster_size)

Pre-existing, but why are we manually calling ctz32() instead of using is_power_of_2()?

Quoted 10 lines
block/qcow2.c +3 -0
@@ -2720,10 +2726,92 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
       */
      BlockBackend *blk;
      QCowHeader *header;
+    size_t cluster_size;
+    int version;
+    int refcount_order;
      uint64_t* refcount_table;
      Error *local_err = NULL;
      int ret;

block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2744

Quoted 10 lines
block/qcow2.c +9 -0
 
+    /* Validate options and set default values */
+    assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
+    qcow2_opts = &create_options->u.qcow2;
+
+    if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
+        error_setg(errp, "Image size must be a multiple of 512 bytes");
+        ret = -EINVAL;
+        goto out;
+    }

This check looks new. Does it really belong in this patch? And it does NOT match what qemu-img can currently do, nor the fact that qcow2 supports byte-based addressing:

$ qemu-img create -f qcow2 tmp 12345
Formatting 'tmp', fmt=qcow2 size=12345 cluster_size=65536 lazy_refcounts=off refcount_bits=16

block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2791

Quoted 6 lines
block/qcow2.c +6 -0
+    if (!qcow2_opts->has_lazy_refcounts) {
+        qcow2_opts->lazy_refcounts = false;
+    }
+    if (version < 3 && qcow2_opts->lazy_refcounts) {
+        error_setg(errp, "Lazy refcounts only supported with compatibility "
+                   "level 1.1 and above (use compat=1.1 or greater)");

Do we want to reword this error message at all, now that QMP spells it 'v3'? Should qemu-img be taught to accept 'compat=v3' as a synonym to 'compat=1.1'?

Quoted 7 lines
block/qcow2.c +7 -0
+        return -EINVAL;
+    }
+
+    if (!qcow2_opts->has_refcount_bits) {
+        qcow2_opts->refcount_bits = 16;
+    }
+    if (qcow2_opts->refcount_bits > 64 ||

block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2808

Quoted 10 lines
block/qcow2.c +10 -0
+        !is_power_of_2(qcow2_opts->refcount_bits))
+    {
+        error_setg(errp, "Refcount width must be a power of two and may not "
+                   "exceed 64 bits");
+        return -EINVAL;
+    }
+    if (version < 3 && qcow2_opts->refcount_bits != 16) {
+        error_setg(errp, "Different refcount widths than 16 bits require "
+                   "compatibility level 1.1 or above (use compat=1.1 or "
+                   "greater)");

and again

Quoted 12 lines
block/qcow2.c +5 -3
@@ -2978,9 +3068,33 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
      }
 
      /* Create the qcow2 image (format layer) */
-    ret = qcow2_create2(bs, size, backing_file, backing_fmt, flags,
-                        cluster_size, prealloc, opts, version, refcount_order,
-                        encryptfmt, errp);
+    create_options = (BlockdevCreateOptions) {
+        .driver         = BLOCKDEV_DRIVER_QCOW2,
+        .u.qcow2        = {
+            .file               = &(BlockdevRef) {
+                .type               = QTYPE_QSTRING,

block/qcow2.c static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) · line 3085

Quoted 10 lines
block/qcow2.c +10 -0
+                .u.reference        = bs->node_name,
+            },
+            .size               = size,
+            .has_version        = true,
+            .version            = version == 2
+                                  ? BLOCKDEV_QCOW2_VERSION_V2
+                                  : BLOCKDEV_QCOW2_VERSION_V3,
+            .has_backing_file   = (backing_file != NULL),
+            .backing_file       = backing_file,
+            .has_backing_fmt    = (backing_fmt != NULL),

I might have spelled it '= !!backing_fmt', but your way is fine too.

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

Re: [Qemu-devel] [PATCH 04/27] qcow2: Pass BlockdevCreateOptions to qcow2_create2()

Am 09.02.2018 um 00:29 hat Eric Blake geschrieben:

Quoted 11 lines · Kevin Wolf
Quoted 9 lines

All of the simple options are now passed to qcow2_create2() in a BlockdevCreateOptions object. Still missing: node-name and the encryption options.

---
  block/qcow2.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++------------
  1 file changed, 152 insertions(+), 38 deletions(-)
Quoted 15 lines

block/qcow2.c static int64_t qcow2_calc_prealloc_size(int64_t total_size, · line 2645

Quoted 12 lines
block/qcow2.c +2 -7
-static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp)
+static bool validate_cluster_size(size_t cluster_size, Error **errp)
  {
-    size_t cluster_size;
-    int cluster_bits;
-
-    cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
-                                         DEFAULT_CLUSTER_SIZE);
-    cluster_bits = ctz32(cluster_size);
+    int cluster_bits = ctz32(cluster_size);
      if (cluster_bits < MIN_CLUSTER_BITS || cluster_bits > MAX_CLUSTER_BITS ||
          (1 << cluster_bits) != cluster_size)

Pre-existing, but why are we manually calling ctz32() instead of using is_power_of_2()?

Probably because is_power_of_2() is newer than this code.

Also, if we don't call ctz32(), we'd have to use (1 << MIN_CLUSTER_BITS) and (1 << MAX_CLUSTER_BITS), so I'm not sure if that would be better.

Quoted 24 lines
Quoted 9 lines
block/qcow2.c +3 -0
@@ -2720,10 +2726,92 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
       */
      BlockBackend *blk;
      QCowHeader *header;
+    size_t cluster_size;
+    int version;
+    int refcount_order;
      uint64_t* refcount_table;
      Error *local_err = NULL;

block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2743

Quoted 10 lines
block/qcow2.c +9 -0
      int ret;
+    /* Validate options and set default values */
+    assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
+    qcow2_opts = &create_options->u.qcow2;
+
+    if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
+        error_setg(errp, "Image size must be a multiple of 512 bytes");
+        ret = -EINVAL;
+        goto out;
+    }

This check looks new. Does it really belong in this patch? And it does NOT match what qemu-img can currently do, nor the fact that qcow2 supports byte-based addressing:

Quoted 3 lines
$ qemu-img create -f qcow2 tmp 12345
Formatting 'tmp', fmt=qcow2 size=12345 cluster_size=65536 lazy_refcounts=off
refcount_bits=16

You're ignoring the result of this command:

$ ./qemu-img create -f qcow2 /tmp/test.qcow2 12345
Formatting '/tmp/test.qcow2', fmt=qcow2 size=12345 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img info -f qcow2 /tmp/test.qcow2
image: /tmp/test.qcow2
file format: qcow2
virtual size: 13K (12800 bytes)
disk size: 196K
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false

As you can see, the size got silently rounded up to the next 512 bytes. qcow2_create() still does the same even after this patch, but I chose not to extend the magic rounding to the QMP command.

In the protocol drivers, I generally just allow byte granularity image sizes, but qcow2 does use sectors internally, so it felt safer to require 512 byte alignment in qcow2.

Quoted 10 lines

block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2791

Quoted 6 lines
block/qcow2.c +6 -0
+    if (!qcow2_opts->has_lazy_refcounts) {
+        qcow2_opts->lazy_refcounts = false;
+    }
+    if (version < 3 && qcow2_opts->lazy_refcounts) {
+        error_setg(errp, "Lazy refcounts only supported with compatibility "
+                   "level 1.1 and above (use compat=1.1 or greater)");

Do we want to reword this error message at all, now that QMP spells it 'v3'? Should qemu-img be taught to accept 'compat=v3' as a synonym to 'compat=1.1'?

Actually, it does accept 'v3' when the whole series is applied, and the message is changed in a later patch that enables this.

Kevin

Reviewed-by on v1 4/27
#12 lore raw

Re: [Qemu-devel] [PATCH 04/27] qcow2: Pass BlockdevCreateOptions to qcow2_create2()

In reply to Kevin Wolf
Quoted 8 lines · Kevin Wolf

All of the simple options are now passed to qcow2_create2() in a BlockdevCreateOptions object. Still missing: node-name and the encryption options.

---
 block/qcow2.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 152 insertions(+), 38 deletions(-)
author
#13 lore raw

PATCH 5/27 qcow2: Use BlockdevRef in qcow2_create2()

In reply to Kevin Wolf

Instead of passing a separate BlockDriverState* into qcow2_create2(), make use of the BlockdevRef that is included in BlockdevCreateOptions.

---
 include/block/block.h |  1 +
 block.c               | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c         | 38 ++++++++++++++++++++++++--------------
 3 files changed, 72 insertions(+), 14 deletions(-)
include/block/block.h +1 -0
index 9b12774ddf..4b11f814a8 100644
@@ -258,6 +258,7 @@ BdrvChild *bdrv_open_child(const char *filename,
                            BlockDriverState* parent,
                            const BdrvChildRole *child_role,
                            bool allow_none, Error **errp);
+BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
                          Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
block.c +47 -0

59 lines not on this page · raw message

block/qcow2.c +24 -14
index 4ab6ed15c2..30f5fbcc37 100644
@@ -2705,8 +2705,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
     return refcount_bits;
 }
 
-static int qcow2_create2(BlockDriverState *bs,
-                         BlockdevCreateOptions *create_options,
+static int qcow2_create2(BlockdevCreateOptions *create_options,
                          QemuOpts *opts, const char *encryptfmt, Error **errp)
 {
     BlockdevCreateOptionsQcow2 *qcow2_opts;
@@ -2724,7 +2723,8 @@ static int qcow2_create2(BlockDriverState *bs,
      * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
      * size for any qcow2 image.
      */
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
+    BlockDriverState *bs = NULL;
     QCowHeader *header;
     size_t cluster_size;
     int version;
@@ -2733,10 +2733,15 @@ static int qcow2_create2(BlockDriverState *bs,
     Error *local_err = NULL;
     int ret;
 
-    /* Validate options and set default values */
     assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
     qcow2_opts = &create_options->u.qcow2;
 
+    bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp);
+    if (bs == NULL) {
+        return -EIO;
+    }
+
+    /* Validate options and set default values */
     if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
         error_setg(errp, "Image size must be a multiple of 512 bytes");
         ret = -EINVAL;
@@ -2765,7 +2770,8 @@ static int qcow2_create2(BlockDriverState *bs,
     }
 
     if (!validate_cluster_size(cluster_size, errp)) {
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
     if (!qcow2_opts->has_preallocation) {
@@ -2776,11 +2782,13 @@ static int qcow2_create2(BlockDriverState *bs,
     {
         error_setg(errp, "Backing file and preallocation cannot be used at "
                    "the same time");
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
     if (qcow2_opts->has_backing_fmt && !qcow2_opts->has_backing_file) {
         error_setg(errp, "Backing format cannot be used without backing file");
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
     if (!qcow2_opts->has_lazy_refcounts) {
@@ -2789,7 +2797,8 @@ static int qcow2_create2(BlockDriverState *bs,
     if (version < 3 && qcow2_opts->lazy_refcounts) {
         error_setg(errp, "Lazy refcounts only supported with compatibility "
                    "level 1.1 and above (use compat=1.1 or greater)");
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
     if (!qcow2_opts->has_refcount_bits) {
@@ -2800,13 +2809,15 @@ static int qcow2_create2(BlockDriverState *bs,
     {
         error_setg(errp, "Refcount width must be a power of two and may not "
                    "exceed 64 bits");
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
     if (version < 3 && qcow2_opts->refcount_bits != 16) {
         error_setg(errp, "Different refcount widths than 16 bits require "
                    "compatibility level 1.1 or above (use compat=1.1 or "
                    "greater)");
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
     refcount_order = ctz32(qcow2_opts->refcount_bits);
 
@@ -2964,9 +2975,8 @@ static int qcow2_create2(BlockDriverState *bs,
 
     ret = 0;
 out:
-    if (blk) {
-        blk_unref(blk);
-    }
+    blk_unref(blk);
+    bdrv_unref(bs);
     return ret;
 }
 
@@ -3094,7 +3104,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
             .refcount_bits      = refcount_bits,
         },
     };
-    ret = qcow2_create2(bs, &create_options, opts, encryptfmt, errp);
+    ret = qcow2_create2(&create_options, opts, encryptfmt, errp);
     if (ret < 0) {
         goto finish;
     }
Signature · 1 line
2.13.6
Reviewed-by on v1 5/27
#14 lore raw

Re: [Qemu-devel] [PATCH 05/27] qcow2: Use BlockdevRef in qcow2_create2()

Quoted 21 lines · Kevin Wolf

Instead of passing a separate BlockDriverState* into qcow2_create2(), make use of the BlockdevRef that is included in BlockdevCreateOptions.

---
  include/block/block.h |  1 +
  block.c               | 47 +++++++++++++++++++++++++++++++++++++++++++++++
  block/qcow2.c         | 38 ++++++++++++++++++++++++--------------
  3 files changed, 72 insertions(+), 14 deletions(-)
block.c +10 -0
+BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
+{
+    BlockDriverState *bs = NULL;
+    Error *local_err = NULL;
+    QObject *obj = NULL;
+    QDict *qdict = NULL;
+    const char *reference = NULL;
+    Visitor *v = NULL;
+
+    if (ref->type == QTYPE_QSTRING) {

block.c BdrvChild *bdrv_open_child(const char *filename, · line 2431

Quoted 10 lines
block.c +10 -0
+        reference = ref->u.reference;
+    } else {
+        BlockdevOptions *options = &ref->u.definition;
+        assert(ref->type == QTYPE_QDICT);
+
+        v = qobject_output_visitor_new(&obj);
+        visit_type_BlockdevOptions(v, NULL, &options, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto fail;

I like calling the label 'done' or 'out'...

Quoted 8 lines
block.c +8 -0
+        }
+        visit_complete(v, &obj);
+
+        qdict = qobject_to_qdict(obj);
+        qdict_flatten(qdict);
+
+        /* bdrv_open_inherit() defaults to the values in bdrv_flags (for
+         * compatibility with other callers) rather than what we want as the

block.c BdrvChild *bdrv_open_child(const char *filename, · line 2449

Quoted 10 lines
block.c +10 -0
+         * real defaults. Apply the defaults here instead. */
+        qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off");
+        qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off");
+        qdict_set_default_str(qdict, BDRV_OPT_READ_ONLY, "off");
+    }
+
+    bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp);
+    obj = NULL;
+
+fail:

...since it is also reached as intentional fallthrough on the success path, making 'fail' a misnomer.

block.c BdrvChild *bdrv_open_child(const char *filename, · line 2454

Quoted 5 lines
block.c +5 -0
+    qobject_decref(obj);
+    visit_free(v);
+    return bs;
+}
+

Otherwise,

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/27
#15 lore raw

Re: [Qemu-devel] [PATCH 05/27] qcow2: Use BlockdevRef in qcow2_create2()

In reply to Kevin Wolf
Quoted 9 lines · Kevin Wolf

Instead of passing a separate BlockDriverState* into qcow2_create2(), make use of the BlockdevRef that is included in BlockdevCreateOptions.

---
 include/block/block.h |  1 +
 block.c               | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c         | 38 ++++++++++++++++++++++++--------------
 3 files changed, 72 insertions(+), 14 deletions(-)
author
#16 lore raw

PATCH 6/27 qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()

In reply to Kevin Wolf

Instead of passing the encryption format name and the QemuOpts down, use the QCryptoBlockCreateOptions contained in BlockdevCreateOptions.

---
 block/qcow2.c | 62 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 17 deletions(-)
block/qcow2.c +45 -17
index 30f5fbcc37..3f08cff1fa 100644
@@ -2387,13 +2387,10 @@ static int qcow2_crypt_method_from_format(const char *encryptfmt)
     }
 }
 
-static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
-                                   QemuOpts *opts, Error **errp)
+static QCryptoBlockCreateOptions *
+qcow2_parse_encryption(const char *encryptfmt, QemuOpts *opts, Error **errp)
 {
-    BDRVQcow2State *s = bs->opaque;
     QCryptoBlockCreateOptions *cryptoopts = NULL;
-    QCryptoBlock *crypto = NULL;
-    int ret = -EINVAL;
     QDict *options, *encryptopts;
     int fmt;
 
@@ -2416,10 +2413,31 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
         error_setg(errp, "Unknown encryption format '%s'", encryptfmt);
         break;
     }
-    if (!cryptoopts) {
-        ret = -EINVAL;
-        goto out;
+
+    QDECREF(encryptopts);
+    return cryptoopts;
+}
+
+static int qcow2_set_up_encryption(BlockDriverState *bs,
+                                   QCryptoBlockCreateOptions *cryptoopts,
+                                   Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    QCryptoBlock *crypto = NULL;
+    int fmt, ret;
+
+    switch (cryptoopts->format) {
+    case Q_CRYPTO_BLOCK_FORMAT_LUKS:
+        fmt = QCOW_CRYPT_LUKS;
+        break;
+    case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+        fmt = QCOW_CRYPT_AES;
+        break;
+    default:
+        error_setg(errp, "Crypto format not supported in qcow2");
+        return -EINVAL;
     }
+
     s->crypt_method_header = fmt;
 
     crypto = qcrypto_block_create(cryptoopts, "encrypt.",
@@ -2427,8 +2445,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
                                   qcow2_crypto_hdr_write_func,
                                   bs, errp);
     if (!crypto) {
-        ret = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
     ret = qcow2_update_header(bs);
@@ -2437,10 +2454,9 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
         goto out;
     }
 
+    ret = 0;
  out:
-    QDECREF(encryptopts);
     qcrypto_block_free(crypto);
-    qapi_free_QCryptoBlockCreateOptions(cryptoopts);
     return ret;
 }
 
@@ -2705,8 +2721,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
     return refcount_bits;
 }
 
-static int qcow2_create2(BlockdevCreateOptions *create_options,
-                         QemuOpts *opts, const char *encryptfmt, Error **errp)
+static int qcow2_create2(BlockdevCreateOptions *create_options, Error **errp)
 {
     BlockdevCreateOptionsQcow2 *qcow2_opts;
     QDict *options;
@@ -2936,8 +2951,8 @@ static int qcow2_create2(BlockdevCreateOptions *create_options,
     }
 
     /* Want encryption? There you go. */
-    if (encryptfmt) {
-        ret = qcow2_set_up_encryption(blk_bs(blk), encryptfmt, opts, errp);
+    if (qcow2_opts->has_encrypt) {
+        ret = qcow2_set_up_encryption(blk_bs(blk), qcow2_opts->encrypt, errp);
         if (ret < 0) {
             goto out;
         }
@@ -2994,6 +3009,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     int version;
     uint64_t refcount_bits;
     char *encryptfmt = NULL;
+    QCryptoBlockCreateOptions *cryptoopts = NULL;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
     int ret;
@@ -3010,6 +3026,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         ret = -EINVAL;
         goto finish;
     }
+
     encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
     if (encryptfmt) {
         if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
@@ -3021,6 +3038,14 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
         encryptfmt = g_strdup("aes");
     }
+    if (encryptfmt) {
+        cryptoopts = qcow2_parse_encryption(encryptfmt, opts, errp);
+        if (cryptoopts == NULL) {
+            ret = -EINVAL;
+            goto finish;
+        }
+    }
+
     cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -3094,6 +3119,8 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
             .backing_file       = backing_file,
             .has_backing_fmt    = (backing_fmt != NULL),
             .backing_fmt        = backing_drv,
+            .has_encrypt        = (encryptfmt != NULL),
+            .encrypt            = cryptoopts,
             .has_cluster_size   = true,
             .cluster_size       = cluster_size,
             .has_preallocation  = true,
@@ -3104,7 +3131,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
             .refcount_bits      = refcount_bits,
         },
     };
-    ret = qcow2_create2(&create_options, opts, encryptfmt, errp);
+    ret = qcow2_create2(&create_options, errp);
     if (ret < 0) {
         goto finish;
     }
@@ -3112,6 +3139,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 finish:
     bdrv_unref(bs);
 
+    qapi_free_QCryptoBlockCreateOptions(cryptoopts);
     g_free(backing_file);
     g_free(backing_fmt);
     g_free(encryptfmt);
Signature · 1 line
2.13.6
#17 lore raw

Re: [Qemu-devel] [PATCH 06/27] qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()

Quoted 11 lines · Kevin Wolf

Instead of passing the encryption format name and the QemuOpts down, use the QCryptoBlockCreateOptions contained in BlockdevCreateOptions.

---
  block/qcow2.c | 62 +++++++++++++++++++++++++++++++++++++++++++----------------
  1 file changed, 45 insertions(+), 17 deletions(-)

block/qcow2.c static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) · line 3032

Quoted 7 lines
block/qcow2.c +1 -0
@@ -3010,6 +3026,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
          ret = -EINVAL;
          goto finish;
      }
+
      encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
      if (encryptfmt) {
          if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {

Should this whitespace change belong to a different patch?

R-b still stands

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Reviewed-by on v1 6/27
#18 lore raw

Re: [Qemu-devel] [PATCH 06/27] qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()

In reply to Kevin Wolf
Quoted 8 lines · Kevin Wolf

Instead of passing the encryption format name and the QemuOpts down, use the QCryptoBlockCreateOptions contained in BlockdevCreateOptions.

---
 block/qcow2.c | 62 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 17 deletions(-)
author
#19 lore raw

PATCH 7/27 qcow2: Handle full/falloc preallocation in qcow2_create2()

In reply to Kevin Wolf

Once qcow2_create2() can be called directly on an already existing node, we must provide the 'full' and 'falloc' preallocation modes outside of creating the image on the protocol layer. Fortunately, we have preallocated truncate now which can provide this functionality.

---
 block/qcow2.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)
block/qcow2.c +19 -9
index 3f08cff1fa..0316335614 100644
@@ -2845,6 +2845,25 @@ static int qcow2_create2(BlockdevCreateOptions *create_options, Error **errp)
     }
     blk_set_allow_write_beyond_eof(blk, true);
 
+    /* Clear the protocol layer and preallocate it if necessary */
+    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
+    if (ret < 0) {
+        goto out;
+    }
+
+    if (qcow2_opts->preallocation == PREALLOC_MODE_FULL ||
+        qcow2_opts->preallocation == PREALLOC_MODE_FALLOC)
+    {
+        int64_t prealloc_size =
+            qcow2_calc_prealloc_size(qcow2_opts->size, cluster_size,
+                                     refcount_order);
+
+        ret = blk_truncate(blk, prealloc_size, qcow2_opts->preallocation, errp);
+        if (ret < 0) {
+            goto out;
+        }
+    }
+
     /* Write the header */
     QEMU_BUILD_BUG_ON((1 << MIN_CLUSTER_BITS) < sizeof(*header));
     header = g_malloc0(cluster_size);
@@ -3081,15 +3100,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
 
     /* Create and open the file (protocol layer) */
-    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
-        int refcount_order = ctz32(refcount_bits);
-        int64_t prealloc_size =
-            qcow2_calc_prealloc_size(size, cluster_size, refcount_order);
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
-        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc),
-                     &error_abort);
-    }
-
     ret = bdrv_create_file(filename, opts, errp);
     if (ret < 0) {
         goto finish;
Signature · 1 line
2.13.6
Reviewed-by on v1 7/27
#20 lore raw

Re: [Qemu-devel] [PATCH 07/27] qcow2: Handle full/falloc preallocation in qcow2_create2()

Quoted 10 lines · Kevin Wolf

Once qcow2_create2() can be called directly on an already existing node, we must provide the 'full' and 'falloc' preallocation modes outside of creating the image on the protocol layer. Fortunately, we have preallocated truncate now which can provide this functionality.

---
 block/qcow2.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)
#21 lore raw

Re: [Qemu-devel] [PATCH 07/27] qcow2: Handle full/falloc preallocation in qcow2_create2()

In reply to Kevin Wolf
Quoted 4 lines · Kevin Wolf

Once qcow2_create2() can be called directly on an already existing node, we must provide the 'full' and 'falloc' preallocation modes outside of creating the image on the protocol layer. Fortunately, we have preallocated truncate now which can provide this functionality.

When reviewing the gluster patch, I noticed that this will break full/falloc preallocation on anything but the file protocol because nothing else yet supports preallocated truncate...

Max

Quoted 53 lines
---
 block/qcow2.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)
block/qcow2.c +19 -9
index 3f08cff1fa..0316335614 100644
@@ -2845,6 +2845,25 @@ static int qcow2_create2(BlockdevCreateOptions *create_options, Error **errp)
     }
     blk_set_allow_write_beyond_eof(blk, true);
 
+    /* Clear the protocol layer and preallocate it if necessary */
+    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
+    if (ret < 0) {
+        goto out;
+    }
+
+    if (qcow2_opts->preallocation == PREALLOC_MODE_FULL ||
+        qcow2_opts->preallocation == PREALLOC_MODE_FALLOC)
+    {
+        int64_t prealloc_size =
+            qcow2_calc_prealloc_size(qcow2_opts->size, cluster_size,
+                                     refcount_order);
+
+        ret = blk_truncate(blk, prealloc_size, qcow2_opts->preallocation, errp);
+        if (ret < 0) {
+            goto out;
+        }
+    }
+
     /* Write the header */
     QEMU_BUILD_BUG_ON((1 << MIN_CLUSTER_BITS) < sizeof(*header));
     header = g_malloc0(cluster_size);
@@ -3081,15 +3100,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
 
     /* Create and open the file (protocol layer) */
-    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
-        int refcount_order = ctz32(refcount_bits);
-        int64_t prealloc_size =
-            qcow2_calc_prealloc_size(size, cluster_size, refcount_order);
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
-        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc),
-                     &error_abort);
-    }
-
     ret = bdrv_create_file(filename, opts, errp);
     if (ret < 0) {
         goto finish;
author
#22 lore raw

PATCH 8/27 util: Add qemu_opts_to_qdict_filtered()

In reply to Kevin Wolf

This allows, given a QemuOpts for a QemuOptsList that was merged from multiple QemuOptsList, to only consider those options that exist in one specific list. Block drivers need this to separate format-layer create options from protocol-level options.

---
 include/qemu/option.h |  2 ++
 util/qemu-option.c    | 37 ++++++++++++++++++++++++++++++++-----
 2 files changed, 34 insertions(+), 5 deletions(-)
include/qemu/option.h +2 -0
index a88c5f02b1..197f80e79d 100644
@@ -125,6 +125,8 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
                             int permit_abbrev);
 QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
                                Error **errp);
+QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,
+                                   QemuOptsList *list, bool del);
 QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
 void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
 
util/qemu-option.c +32 -5
index 553d3dc552..ba33bbe487 100644
@@ -1004,14 +1004,18 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp)
 }
 
 /*
- * Convert from QemuOpts to QDict.
- * The QDict values are of type QString.
+ * Convert from QemuOpts to QDict. The QDict values are of type QString.
+ * If @list is given, only add those options to the QDict that are contained in
+ * the list. If @del is true, any options added to the QDict are removed from
+ * the QemuOpts, otherwise they remain there.
+ *
  * TODO We'll want to use types appropriate for opt->desc->type, but
  * this is enough for now.
  */
-QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
+QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,
+                                   QemuOptsList *list, bool del)
 {
-    QemuOpt *opt;
+    QemuOpt *opt, *next;
 
     if (!qdict) {
         qdict = qdict_new();
@@ -1019,12 +1023,35 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
     if (opts->id) {
         qdict_put_str(qdict, "id", opts->id);
     }
-    QTAILQ_FOREACH(opt, &opts->head, next) {
+    QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next) {
+        if (list) {
+            QemuOptDesc *desc;
+            bool found = false;
+            for (desc = list->desc; desc->name; desc++) {
+                if (!strcmp(desc->name, opt->name)) {
+                    found = true;
+                    break;
+                }
+            }
+            if (!found) {
+                continue;
+            }
+        }
         qdict_put_str(qdict, opt->name, opt->str);
+        if (del) {
+            qemu_opt_del_all(opts, opt->name);
+        }
     }
     return qdict;
 }
 
+/* Copy all options in a QemuOpts to the given QDict. See
+ * qemu_opts_to_qdict_filtered() for details. */
+QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
+{
+    return qemu_opts_to_qdict_filtered(opts, qdict, NULL, false);
+}
+
 /* Validate parsed opts against descriptions where no
  * descriptions were provided in the QemuOptsList.
  */
Signature · 1 line
2.13.6
Reviewed-by on v1 8/27
#23 lore raw

Re: [Qemu-devel] [PATCH 08/27] util: Add qemu_opts_to_qdict_filtered()

Quoted 10 lines · Kevin Wolf

This allows, given a QemuOpts for a QemuOptsList that was merged from multiple QemuOptsList, to only consider those options that exist in one specific list. Block drivers need this to separate format-layer create options from protocol-level options.

---
 include/qemu/option.h |  2 ++
 util/qemu-option.c    | 37 ++++++++++++++++++++++++++++++++-----
 2 files changed, 34 insertions(+), 5 deletions(-)
Reviewed-by on v1 8/27
#24 lore raw

Re: [Qemu-devel] [PATCH 08/27] util: Add qemu_opts_to_qdict_filtered()

In reply to Kevin Wolf
Quoted 10 lines · Kevin Wolf

This allows, given a QemuOpts for a QemuOptsList that was merged from multiple QemuOptsList, to only consider those options that exist in one specific list. Block drivers need this to separate format-layer create options from protocol-level options.

---
  include/qemu/option.h |  2 ++
  util/qemu-option.c    | 37 ++++++++++++++++++++++++++++++++-----
  2 files changed, 34 insertions(+), 5 deletions(-)

Is there any unit test coverage we should be adding, beyond the default we get when the list parameter is NULL?

But what you have looks sane;

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

PATCH 9/27 qdict: Introduce qdict_rename_keys()

In reply to Kevin Wolf

A few block drivers will need to rename .bdrv_create options for their QAPIfication, so let's have a helper function for that.

---
 include/qapi/qmp/qdict.h |  6 ++++++
 qobject/qdict.c          | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)
include/qapi/qmp/qdict.h +6 -0
index fc218e7be6..862441b9d3 100644
@@ -90,4 +90,10 @@ QObject *qdict_crumple(const QDict *src, Error **errp);
 
 void qdict_join(QDict *dest, QDict *src, bool overwrite);
 
+typedef struct QDictRenames {
+    const char *from;
+    const char *to;
+} QDictRenames;
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp);
+
 #endif /* QDICT_H */
qobject/qdict.c +30 -0
index e8f15f1132..07ae9489a7 100644
@@ -1051,3 +1051,33 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite)
         entry = next;
     }
 }
+
+/**
+ * qdict_rename_keys(): Rename keys in qdict according to the replacements
+ * specified in the array renames. The array must be terminated by an entry
+ * with from = NULL.
+ *
+ * Returns true for success, false in error cases.
+ */
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp)
+{
+    QObject *qobj;
+
+    while (renames->from) {
+        if (qdict_haskey(qdict, renames->from)) {
+            if (qdict_haskey(qdict, renames->to)) {
+                error_setg(errp, "'%s' and its alias '%s' can't be used at the "
+                           "same time", renames->to, renames->from);
+                return false;
+            }
+
+            qobj = qdict_get(qdict, renames->from);
+            qobject_incref(qobj);
+            qdict_put_obj(qdict, renames->to, qobj);
+            qdict_del(qdict, renames->from);
+        }
+
+        renames++;
+    }
+    return true;
+}
Signature · 1 line
2.13.6
Reviewed-by on v1 9/27
#26 lore raw

Re: [Qemu-devel] [PATCH 09/27] qdict: Introduce qdict_rename_keys()

Quoted 8 lines · Kevin Wolf

A few block drivers will need to rename .bdrv_create options for their QAPIfication, so let's have a helper function for that.

---
 include/qapi/qmp/qdict.h |  6 ++++++
 qobject/qdict.c          | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Hmmm, looks OK, but I wonder whether this is going to be any more efficient than if we simply had a qdict_rename_key() function that you call multiple times.

("efficient" both in terms of code readability and runtime efficiency)

So, I'll give you a

and let you decide.

Quoted 55 lines
include/qapi/qmp/qdict.h +6 -0
index fc218e7be6..862441b9d3 100644
@@ -90,4 +90,10 @@ QObject *qdict_crumple(const QDict *src, Error **errp);
 
 void qdict_join(QDict *dest, QDict *src, bool overwrite);
 
+typedef struct QDictRenames {
+    const char *from;
+    const char *to;
+} QDictRenames;
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp);
+
 #endif /* QDICT_H */
qobject/qdict.c +30 -0
index e8f15f1132..07ae9489a7 100644
@@ -1051,3 +1051,33 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite)
         entry = next;
     }
 }
+
+/**
+ * qdict_rename_keys(): Rename keys in qdict according to the replacements
+ * specified in the array renames. The array must be terminated by an entry
+ * with from = NULL.
+ *
+ * Returns true for success, false in error cases.
+ */
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp)
+{
+    QObject *qobj;
+
+    while (renames->from) {
+        if (qdict_haskey(qdict, renames->from)) {
+            if (qdict_haskey(qdict, renames->to)) {
+                error_setg(errp, "'%s' and its alias '%s' can't be used at the "
+                           "same time", renames->to, renames->from);
+                return false;
+            }
+
+            qobj = qdict_get(qdict, renames->from);
+            qobject_incref(qobj);
+            qdict_put_obj(qdict, renames->to, qobj);
+            qdict_del(qdict, renames->from);
+        }
+
+        renames++;
+    }
+    return true;
+}
#27 lore raw

Re: [Qemu-devel] [PATCH 09/27] qdict: Introduce qdict_rename_keys()

Quoted 14 lines · Max Reitz
Quoted 8 lines · Kevin Wolf

A few block drivers will need to rename .bdrv_create options for their QAPIfication, so let's have a helper function for that.

---
 include/qapi/qmp/qdict.h |  6 ++++++
 qobject/qdict.c          | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Hmmm, looks OK, but I wonder whether this is going to be any more efficient than if we simply had a qdict_rename_key() function that you call multiple times.

Quoted 7 lines

("efficient" both in terms of code readability and runtime efficiency)

So, I'll give you a

and let you decide.

Ah, I see, error handling is much easier this way, that's right. OK, R-b stands then.

Max

Reviewed-by on v1 9/27
#28 lore raw

Re: [Qemu-devel] [PATCH 09/27] qdict: Introduce qdict_rename_keys()

In reply to Kevin Wolf
Quoted 8 lines · Kevin Wolf

A few block drivers will need to rename .bdrv_create options for their QAPIfication, so let's have a helper function for that.

---
  include/qapi/qmp/qdict.h |  6 ++++++
  qobject/qdict.c          | 30 ++++++++++++++++++++++++++++++
  2 files changed, 36 insertions(+)

Again, unit test coverage?

Quoted 4 lines
qobject/qdict.c +4 -0
+/**
+ * qdict_rename_keys(): Rename keys in qdict according to the replacements
+ * specified in the array renames. The array must be terminated by an entry
+ * with from = NULL.

qobject/qdict.c void qdict_join(QDict *dest, QDict *src, bool overwrite) · line 1068

Quoted 10 lines
qobject/qdict.c +10 -0
+ *
+ * Returns true for success, false in error cases.
+ */
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp)
+{
+    QObject *qobj;
+
+    while (renames->from) {
+        if (qdict_haskey(qdict, renames->from)) {
+            if (qdict_haskey(qdict, renames->to)) {

Depending on how efficient qdict_haskey() is, this is a lot of looping. Good thing our lists aren't so large that we'd notice the effects of cubic scaling (I count O(m*n*n), where m is renames length, and n is worst-case performance of qdict_haskey). Definitely not worth the effort of more code to try and have a more efficient algorithm in large-term scaling but which hurts performance with increased overhead on small lists.

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

PATCH 10/27 qcow2: Use visitor for options in qcow2_create()

In reply to Kevin Wolf
---
 block/qcow2.c              | 219 ++++++++++++++++-----------------------------
 tests/qemu-iotests/049.out |   8 +-
 tests/qemu-iotests/112.out |   4 +-
 3 files changed, 84 insertions(+), 147 deletions(-)
block/qcow2.c +78 -141
index 0316335614..02e331a938 100644
@@ -36,7 +36,7 @@
 #include "qemu/option_int.h"
 #include "qemu/cutils.h"
 #include "qemu/bswap.h"
-#include "qapi/opts-visitor.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qapi-visit.h"
 #include "block/crypto.h"
 
@@ -2387,37 +2387,6 @@ static int qcow2_crypt_method_from_format(const char *encryptfmt)
     }
 }
 
-static QCryptoBlockCreateOptions *
-qcow2_parse_encryption(const char *encryptfmt, QemuOpts *opts, Error **errp)
-{
-    QCryptoBlockCreateOptions *cryptoopts = NULL;
-    QDict *options, *encryptopts;
-    int fmt;
-
-    options = qemu_opts_to_qdict(opts, NULL);
-    qdict_extract_subqdict(options, &encryptopts, "encrypt.");
-    QDECREF(options);
-
-    fmt = qcow2_crypt_method_from_format(encryptfmt);
-
-    switch (fmt) {
-    case QCOW_CRYPT_LUKS:
-        cryptoopts = block_crypto_create_opts_init(
-            Q_CRYPTO_BLOCK_FORMAT_LUKS, encryptopts, errp);
-        break;
-    case QCOW_CRYPT_AES:
-        cryptoopts = block_crypto_create_opts_init(
-            Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp);
-        break;
-    default:
-        error_setg(errp, "Unknown encryption format '%s'", encryptfmt);
-        break;
-    }
-
-    QDECREF(encryptopts);
-    return cryptoopts;
-}
-
 static int qcow2_set_up_encryption(BlockDriverState *bs,
                                    QCryptoBlockCreateOptions *cryptoopts,
                                    Error **errp)
@@ -2811,7 +2780,7 @@ static int qcow2_create2(BlockdevCreateOptions *create_options, Error **errp)
     }
     if (version < 3 && qcow2_opts->lazy_refcounts) {
         error_setg(errp, "Lazy refcounts only supported with compatibility "
-                   "level 1.1 and above (use compat=1.1 or greater)");
+                   "level 1.1 and above (use version=v3 or greater)");
         ret = -EINVAL;
         goto out;
     }
@@ -2829,7 +2798,7 @@ static int qcow2_create2(BlockdevCreateOptions *create_options, Error **errp)
     }
     if (version < 3 && qcow2_opts->refcount_bits != 16) {
         error_setg(errp, "Different refcount widths than 16 bits require "
-                   "compatibility level 1.1 or above (use compat=1.1 or "
+                   "compatibility level 1.1 or above (use version=v3 or "
                    "greater)");
         ret = -EINVAL;
         goto out;
@@ -3016,144 +2985,112 @@ out:
 
 static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-    BlockdevCreateOptions create_options;
-    char *backing_file = NULL;
-    char *backing_fmt = NULL;
-    BlockdevDriver backing_drv;
-    char *buf = NULL;
-    uint64_t size = 0;
-    int flags = 0;
-    size_t cluster_size = DEFAULT_CLUSTER_SIZE;
-    PreallocMode prealloc;
-    int version;
-    uint64_t refcount_bits;
-    char *encryptfmt = NULL;
-    QCryptoBlockCreateOptions *cryptoopts = NULL;
+    BlockdevCreateOptions *create_options = NULL;
+    QDict *qdict = NULL;
+    QObject *qobj;
+    Visitor *v;
     BlockDriverState *bs = NULL;
-    Error *local_err = NULL;
+    const char *val;
     int ret;
+    Error *local_err = NULL;
 
-    /* Read out options */
-    size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
-                    BDRV_SECTOR_SIZE);
-    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
-    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
-    backing_drv = qapi_enum_parse(&BlockdevDriver_lookup, backing_fmt,
-                                  0, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    /* Only the keyval visitor supports the dotted syntax needed for
+     * encryption, so go through a QDict before getting a QAPI type. Ignore
+     * options meant for the protocol layer so that the visitor doesn't
+     * complain. */
+    qdict = qemu_opts_to_qdict_filtered(opts, NULL, bdrv_qcow2.create_opts,
+                                        true);
+
+    /* Handle encryption options */
+    val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT);
+    if (val && !strcmp(val, "on")) {
+        qdict_put_str(qdict, BLOCK_OPT_ENCRYPT, "qcow");
+    } else if (val && !strcmp(val, "off")) {
+        qdict_del(qdict, BLOCK_OPT_ENCRYPT);
+    }
+
+    val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT);
+    if (val && !strcmp(val, "aes")) {
+        qdict_put_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT, "qcow");
+    }
+
+    /* Convert compat=0.10/1.1 into compat=v2/v3, to be renamed into
+     * version=v2/v3 below. */
+    val = qdict_get_try_str(qdict, BLOCK_OPT_COMPAT_LEVEL);
+    if (val && !strcmp(val, "0.10")) {
+        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v2");
+    } else if (val && !strcmp(val, "1.1")) {
+        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v3");
+    }
+
+    /* Change legacy command line options into QMP ones */
+    static const QDictRenames opt_renames[] = {
+        { BLOCK_OPT_BACKING_FILE,       "backing-file" },
+        { BLOCK_OPT_BACKING_FMT,        "backing-fmt" },
+        { BLOCK_OPT_CLUSTER_SIZE,       "cluster-size" },
+        { BLOCK_OPT_LAZY_REFCOUNTS,     "lazy-refcounts" },
+        { BLOCK_OPT_REFCOUNT_BITS,      "refcount-bits" },
+        { BLOCK_OPT_ENCRYPT,            BLOCK_OPT_ENCRYPT_FORMAT },
+        { BLOCK_OPT_COMPAT_LEVEL,       "version" },
+        { NULL, NULL },
+    };
+
+    if (!qdict_rename_keys(qdict, opt_renames, errp)) {
         ret = -EINVAL;
         goto finish;
     }
 
-    encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
-    if (encryptfmt) {
-        if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
-            error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
-                       BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
-            ret = -EINVAL;
-            goto finish;
-        }
-    } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-        encryptfmt = g_strdup("aes");
-    }
-    if (encryptfmt) {
-        cryptoopts = qcow2_parse_encryption(encryptfmt, opts, errp);
-        if (cryptoopts == NULL) {
-            ret = -EINVAL;
-            goto finish;
-        }
-    }
-
-    cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
+    /* Create and open the file (protocol layer) */
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
         goto finish;
     }
-    buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
-    prealloc = qapi_enum_parse(&PreallocMode_lookup, buf,
-                               PREALLOC_MODE_OFF, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
+
+    bs = bdrv_open(filename, NULL, NULL,
+                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    if (bs == NULL) {
+        ret = -EIO;
         goto finish;
     }
 
-    version = qcow2_opt_get_version_del(opts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    /* Set 'driver' and 'node' options */
+    qdict_put_str(qdict, "driver", "qcow2");
+    qdict_put_str(qdict, "file", bs->node_name);
+
+    /* Now get the QAPI type BlockdevCreateOptions */
+    qobj = qdict_crumple(qdict, errp);
+    QDECREF(qdict);
+    qdict = qobject_to_qdict(qobj);
+    if (qdict == NULL) {
         ret = -EINVAL;
         goto finish;
     }
 
-    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_LAZY_REFCOUNTS, false)) {
-        flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
-    }
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+    visit_free(v);
 
-    refcount_bits = qcow2_opt_get_refcount_bits_del(opts, version, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
         goto finish;
     }
 
-
-    /* Create and open the file (protocol layer) */
-    ret = bdrv_create_file(filename, opts, errp);
-    if (ret < 0) {
-        goto finish;
-    }
-
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
-    if (bs == NULL) {
-        ret = -EIO;
-        goto finish;
-    }
+    /* Silently round up size */
+    create_options->u.qcow2.size = ROUND_UP(create_options->u.qcow2.size,
+                                            BDRV_SECTOR_SIZE);
 
     /* Create the qcow2 image (format layer) */
-    create_options = (BlockdevCreateOptions) {
-        .driver         = BLOCKDEV_DRIVER_QCOW2,
-        .u.qcow2        = {
-            .file               = &(BlockdevRef) {
-                .type               = QTYPE_QSTRING,
-                .u.reference        = bs->node_name,
-            },
-            .size               = size,
-            .has_version        = true,
-            .version            = version == 2
-                                  ? BLOCKDEV_QCOW2_VERSION_V2
-                                  : BLOCKDEV_QCOW2_VERSION_V3,
-            .has_backing_file   = (backing_file != NULL),
-            .backing_file       = backing_file,
-            .has_backing_fmt    = (backing_fmt != NULL),
-            .backing_fmt        = backing_drv,
-            .has_encrypt        = (encryptfmt != NULL),
-            .encrypt            = cryptoopts,
-            .has_cluster_size   = true,
-            .cluster_size       = cluster_size,
-            .has_preallocation  = true,
-            .preallocation      = prealloc,
-            .has_lazy_refcounts = true,
-            .lazy_refcounts     = (flags & BLOCK_FLAG_LAZY_REFCOUNTS),
-            .has_refcount_bits  = true,
-            .refcount_bits      = refcount_bits,
-        },
-    };
-    ret = qcow2_create2(&create_options, errp);
+    ret = qcow2_create2(create_options, errp);
     if (ret < 0) {
         goto finish;
     }
 
+    ret = 0;
 finish:
+    QDECREF(qdict);
     bdrv_unref(bs);
-
-    qapi_free_QCryptoBlockCreateOptions(cryptoopts);
-    g_free(backing_file);
-    g_free(backing_fmt);
-    g_free(encryptfmt);
-    g_free(buf);
+    qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
 
tests/qemu-iotests/049.out +4 -4
index 003247023e..0871bff564 100644
@@ -166,11 +166,11 @@ qemu-img create -f qcow2 -o compat=1.1 TEST_DIR/t.qcow2 64M
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=1.1 cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 qemu-img create -f qcow2 -o compat=0.42 TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: Invalid compatibility level: '0.42'
+qemu-img: TEST_DIR/t.qcow2: Invalid parameter '0.42'
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.42 cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 qemu-img create -f qcow2 -o compat=foobar TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: Invalid compatibility level: 'foobar'
+qemu-img: TEST_DIR/t.qcow2: Invalid parameter 'foobar'
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=foobar cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 == Check preallocation option ==
@@ -182,7 +182,7 @@ qemu-img create -f qcow2 -o preallocation=metadata TEST_DIR/t.qcow2 64M
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 preallocation=metadata lazy_refcounts=off refcount_bits=16
 
 qemu-img create -f qcow2 -o preallocation=1234 TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: invalid parameter value: 1234
+qemu-img: TEST_DIR/t.qcow2: Invalid parameter '1234'
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 preallocation=1234 lazy_refcounts=off refcount_bits=16
 
 == Check encryption option ==
@@ -205,7 +205,7 @@ qemu-img create -f qcow2 -o compat=0.10,lazy_refcounts=off TEST_DIR/t.qcow2 64M
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.10 cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 qemu-img create -f qcow2 -o compat=0.10,lazy_refcounts=on TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: Lazy refcounts only supported with compatibility level 1.1 and above (use compat=1.1 or greater)
+qemu-img: TEST_DIR/t.qcow2: Lazy refcounts only supported with compatibility level 1.1 and above (use version=v3 or greater)
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.10 cluster_size=65536 lazy_refcounts=on refcount_bits=16
 
 *** done
tests/qemu-iotests/112.out +2 -2
index 81b04d1452..86f041075d 100644
@@ -21,9 +21,9 @@ refcount bits: 16
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 refcount bits: 16
-qemu-img: TEST_DIR/t.IMGFMT: Different refcount widths than 16 bits require compatibility level 1.1 or above (use or greater)
+qemu-img: TEST_DIR/t.IMGFMT: Different refcount widths than 16 bits require compatibility level 1.1 or above (use version=v3 or greater)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-img: TEST_DIR/t.IMGFMT: Different refcount widths than 16 bits require compatibility level 1.1 or above (use or greater)
+qemu-img: TEST_DIR/t.IMGFMT: Different refcount widths than 16 bits require compatibility level 1.1 or above (use version=v3 or greater)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 
 === Snapshot limit on refcount_bits=1 ===
Signature · 1 line
2.13.6
Reviewed-by on v1 10/27
#30 lore raw

Re: [Qemu-devel] [PATCH 10/27] qcow2: Use visitor for options in qcow2_create()

Quoted 6 lines · Kevin Wolf
---
 block/qcow2.c              | 219 ++++++++++++++++-----------------------------
 tests/qemu-iotests/049.out |   8 +-
 tests/qemu-iotests/112.out |   4 +-
 3 files changed, 84 insertions(+), 147 deletions(-)
Reviewed-by on v1 10/27
#31 lore raw

Re: [Qemu-devel] [PATCH 10/27] qcow2: Use visitor for options in qcow2_create()

In reply to Kevin Wolf
Quoted 10 lines · Kevin Wolf
---
  block/qcow2.c              | 219 ++++++++++++++++-----------------------------
  tests/qemu-iotests/049.out |   8 +-
  tests/qemu-iotests/112.out |   4 +-
  3 files changed, 84 insertions(+), 147 deletions(-)
      BlockDriverState *bs = NULL;

block/qcow2.c out: · line 2995

Quoted 4 lines
block/qcow2.c +2 -1
-    Error *local_err = NULL;
+    const char *val;
      int ret;
+    Error *local_err = NULL;

Worth the churn on the local_err declaration position?

Quoted 5 lines
block/qcow2.c +0 -5
-    /* Read out options */
-    size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
-                    BDRV_SECTOR_SIZE);
-    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
-    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);

block/qcow2.c out: · line 3002

Quoted 10 lines
block/qcow2.c +6 -4
-    backing_drv = qapi_enum_parse(&BlockdevDriver_lookup, backing_fmt,
-                                  0, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    /* Only the keyval visitor supports the dotted syntax needed for
+     * encryption, so go through a QDict before getting a QAPI type. Ignore
+     * options meant for the protocol layer so that the visitor doesn't
+     * complain. */
+    qdict = qemu_opts_to_qdict_filtered(opts, NULL, bdrv_qcow2.create_opts,
+                                        true);

Glue code at its finest ;)

block/qcow2.c out: · line 3024

Quoted 8 lines
block/qcow2.c +8 -0
+    /* Convert compat=0.10/1.1 into compat=v2/v3, to be renamed into
+     * version=v2/v3 below. */
+    val = qdict_get_try_str(qdict, BLOCK_OPT_COMPAT_LEVEL);
+    if (val && !strcmp(val, "0.10")) {
+        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v2");
+    } else if (val && !strcmp(val, "1.1")) {
+        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v3");
+    }

Not only does this map the old 'qemu-img create -o' spellings into the QMP form, but it means that we now also accept the new spelling via qemu-img command line. Might be worth mentioning in the commit message as an intentional enhancement.

block/qcow2.c out: · line 3036

Quoted 12 lines
block/qcow2.c +12 -0
+
+    /* Change legacy command line options into QMP ones */
+    static const QDictRenames opt_renames[] = {
+        { BLOCK_OPT_BACKING_FILE,       "backing-file" },
+        { BLOCK_OPT_BACKING_FMT,        "backing-fmt" },
+        { BLOCK_OPT_CLUSTER_SIZE,       "cluster-size" },
+        { BLOCK_OPT_LAZY_REFCOUNTS,     "lazy-refcounts" },
+        { BLOCK_OPT_REFCOUNT_BITS,      "refcount-bits" },
+        { BLOCK_OPT_ENCRYPT,            BLOCK_OPT_ENCRYPT_FORMAT },
+        { BLOCK_OPT_COMPAT_LEVEL,       "version" },
+        { NULL, NULL },
+    };

Looks reasonable to me.

block/qcow2.c out: · line 3046

Quoted 9 lines
block/qcow2.c +3 -5
-
-    cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
+    /* Create and open the file (protocol layer) */
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
          goto finish;

Git got lost on producing a sane diff. Oh well.

block/qcow2.c out: · line 3064

Quoted 12 lines
block/qcow2.c +9 -3
-    version = qcow2_opt_get_version_del(opts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    /* Set 'driver' and 'node' options */
+    qdict_put_str(qdict, "driver", "qcow2");
+    qdict_put_str(qdict, "file", bs->node_name);
+
+    /* Now get the QAPI type BlockdevCreateOptions */
+    qobj = qdict_crumple(qdict, errp);
+    QDECREF(qdict);
+    qdict = qobject_to_qdict(qobj);
+    if (qdict == NULL) {

Fun with round trips. Maybe someday we can improve things, but for now, I'm glad that it at least works.

block/qcow2.c out: · line 3071

Quoted 10 lines
          ret = -EINVAL;
          goto finish;
      }
block/qcow2.c +3 -3
-    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_LAZY_REFCOUNTS, false)) {
-        flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
-    }
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+    visit_free(v);

But this part is definitely the payout of what we wanted to get to!

block/qcow2.c out: · line 3119

Quoted 5 lines
      /* Create the qcow2 image (format layer) */
block/qcow2.c +0 -4
-    create_options = (BlockdevCreateOptions) {
-        .driver         = BLOCKDEV_DRIVER_QCOW2,
-        .u.qcow2        = {
-            .file               = &(BlockdevRef) {

And using the visitor is a lot nicer than populating the struct by hand.

tests/qemu-iotests/049.out qemu-img create -f qcow2 -o compat=1.1 TEST_DIR/t.qcow2 64M · line 170

Quoted 8 lines

+++ b/tests/qemu-iotests/049.out

tests/qemu-iotests/049.out +1 -1
@@ -166,11 +166,11 @@ qemu-img create -f qcow2 -o compat=1.1 TEST_DIR/t.qcow2 64M
  Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=1.1 cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
  qemu-img create -f qcow2 -o compat=0.42 TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: Invalid compatibility level: '0.42'
+qemu-img: TEST_DIR/t.qcow2: Invalid parameter '0.42'
  Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.42 cluster_size=65536 lazy_refcounts=off refcount_bits=16

Yep, the visitor has slightly different messages, but I'm fine with the fallout.

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

PATCH 11/27 block: x-blockdev-create QMP command

In reply to Kevin Wolf

This adds a synchronous x-blockdev-create QMP command that can create qcow2 images on a given node name.

We don't want to block while creating an image, so this is not the final interface in all aspects, but BlockdevCreateOptionsQcow2 and .bdrv_co_create() are what they actually might look like in the end. In any case, this should be good enough to test whether we interpret BlockdevCreateOptions as we should.

---
 qapi/block-core.json      | 12 ++++++++
 include/block/block.h     |  1 +
 include/block/block_int.h |  2 ++
 block.c                   |  2 +-
 block/create.c            | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |  3 +-
 block/Makefile.objs       |  2 +-
 7 files changed, 94 insertions(+), 3 deletions(-)
 create mode 100644 block/create.c
qapi/block-core.json +12 -0
index aade602a04..c0e61483af 100644
@@ -3442,6 +3442,18 @@
   } }
 
 ##
+# @x-blockdev-create:
+#
+# Create an image format on a given node.
+# TODO Replace with something asynchronous (block job?)
+#
+# Since: 2.12
+##
+{ 'command': 'x-blockdev-create',
+  'data': 'BlockdevCreateOptions',
+  'boxed': true }
+
+##
 # @blockdev-open-tray:
 #
 # Opens a block device's tray. If there is a block driver state tree inserted as
include/block/block.h +1 -0
index 4b11f814a8..fdc76f1735 100644
@@ -238,6 +238,7 @@ char *bdrv_perm_names(uint64_t perm);
 void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
 bool bdrv_uses_whitelist(void);
+int bdrv_is_whitelisted(BlockDriver *drv, bool read_only);
 BlockDriver *bdrv_find_protocol(const char *filename,
                                 bool allow_protocol_prefix,
                                 Error **errp);
include/block/block_int.h +2 -0
index 29cafa4236..a9f144d7bd 100644
@@ -130,6 +130,8 @@ struct BlockDriver {
     int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
+    int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+                                       Error **errp);
     int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp);
     int (*bdrv_make_empty)(BlockDriverState *bs);
 
block.c +1 -1
index f24d89e7de..725c33e53f 100644
@@ -369,7 +369,7 @@ BlockDriver *bdrv_find_format(const char *format_name)
     return bdrv_do_find_format(format_name);
 }
 
-static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
+int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
 {
     static const char *whitelist_rw[] = {
         CONFIG_BDRV_RW_WHITELIST
block/create.c new file +75 -0

75 lines not on this page · raw message

block/qcow2.c +2 -1
index 02e331a938..65450415f8 100644
@@ -4437,7 +4437,8 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_reopen_abort    = qcow2_reopen_abort,
     .bdrv_join_options    = qcow2_join_options,
     .bdrv_child_perm      = bdrv_format_default_perms,
-    .bdrv_create        = qcow2_create,
+    .bdrv_create          = qcow2_create,
+    .bdrv_co_create       = qcow2_create2,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_get_block_status = qcow2_co_get_block_status,
 
block/Makefile.objs +1 -1
index a73387f1bf..c7190c328e 100644
@@ -9,7 +9,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o
 block-obj-$(CONFIG_POSIX) += file-posix.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
-block-obj-y += null.o mirror.o commit.o io.o
+block-obj-y += null.o mirror.o commit.o io.o create.o
 block-obj-y += throttle-groups.o
 
 block-obj-y += nbd.o nbd-client.o sheepdog.o
Signature · 1 line
2.13.6
#33 lore raw

Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

Quoted 20 lines · Kevin Wolf

This adds a synchronous x-blockdev-create QMP command that can create qcow2 images on a given node name.

We don't want to block while creating an image, so this is not the final interface in all aspects, but BlockdevCreateOptionsQcow2 and .bdrv_co_create() are what they actually might look like in the end. In any case, this should be good enough to test whether we interpret BlockdevCreateOptions as we should.

---
 qapi/block-core.json      | 12 ++++++++
 include/block/block.h     |  1 +
 include/block/block_int.h |  2 ++
 block.c                   |  2 +-
 block/create.c            | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |  3 +-
 block/Makefile.objs       |  2 +-
 7 files changed, 94 insertions(+), 3 deletions(-)
 create mode 100644 block/create.c

[...]

Quoted 6 lines
block/create.c new file +0 -0
new file mode 100644
index 0000000000..e95446a0f3
@@ -0,0 +1,75 @@

[...]

block/create.c line 52

Quoted 11 lines
block/create.c new file +11 -0
+void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp)
+{
+    const char *fmt = BlockdevDriver_str(options->driver);
+    BlockDriver *drv = bdrv_find_format(fmt);
+    Coroutine *co;
+    BlockdevCreateCo cco;
+
+    /* If the driver is in the schema, we know that it exists. But it may not
+     * be whitelisted. */
+    assert(drv);
+    if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, true)) {

Isn't this more of an R/W case than RO?

Max

Quoted 3 lines
block/create.c new file +3 -0
+        error_setg(errp, "Driver is not whitelisted");
+        return;
+    }
#34 lore raw

Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

In reply to Kevin Wolf
Quoted 13 lines · Kevin Wolf

This adds a synchronous x-blockdev-create QMP command that can create qcow2 images on a given node name.

We don't want to block while creating an image, so this is not the final interface in all aspects, but BlockdevCreateOptionsQcow2 and .bdrv_co_create() are what they actually might look like in the end. In any case, this should be good enough to test whether we interpret BlockdevCreateOptions as we should.

---

qapi/block-core.json line 3448

Quoted 7 lines
qapi/block-core.json +4 -0
@@ -3442,6 +3442,18 @@
    } }
 
  ##
+# @x-blockdev-create:
+#
+# Create an image format on a given node.
+# TODO Replace with something asynchronous (block job?)

We've learned our lesson - don't commit to the final name on an interface that we have not yet experimented with.

qapi/block-core.json line 3455

Quoted 7 lines
qapi/block-core.json +7 -0
+#
+# Since: 2.12
+##
+{ 'command': 'x-blockdev-create',
+  'data': 'BlockdevCreateOptions',
+  'boxed': true }
+

Lots of code packed in that little description ;)

include/block/block_int.h struct BlockDriver { · line 134

Quoted 7 lines
include/block/block_int.h +3 -0
+++ b/include/block/block_int.h
@@ -130,6 +130,8 @@ struct BlockDriver {
      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                            Error **errp);
      void (*bdrv_close)(BlockDriverState *bs);
+    int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+                                       Error **errp);

I know we haven't been very good in the past, but can you add a comment here on the contract that drivers are supposed to obey when implementing this callback?

block.c BlockDriver *bdrv_find_format(const char *format_name) · line 372

Quoted 7 lines
block.c +2 -1
+++ b/block.c
@@ -369,7 +369,7 @@ BlockDriver *bdrv_find_format(const char *format_name)
      return bdrv_do_find_format(format_name);
  }
 
-static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
+int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)

Worth mentioning that bdrv_is_whitelisted had to be exported as part of the commit message? (Or even promoting it to public in a separate commit?)

block/create.c line 4

Quoted 6 lines
block/create.c new file +5 -0
+++ b/block/create.c
@@ -0,0 +1,75 @@
+/*
+ * Block layer code related to image creation
+ *
+ * Copyright (c) 2018 Kevin Wolf <[email protected]>

The question came up in another thread, but I didn't hear your answer there; I know Red Hat permits you to claim personal copyright while still using a redhat.com address for code written on personal time, but should this claim belong to Red Hat instead of you?

block/create.c line 59

Quoted 3 lines
block/create.c new file +3 -0
+    /* Call callback if it exists */
+    if (!drv->bdrv_co_create) {
+        error_setg(errp, "Driver does not support blockdev-create");

Should this error message refer to 'x-blockdev-create' in the short term?

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

Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

Am 15.02.2018 um 20:58 hat Eric Blake geschrieben:

Quoted 35 lines · Kevin Wolf
Quoted 13 lines

This adds a synchronous x-blockdev-create QMP command that can create qcow2 images on a given node name.

We don't want to block while creating an image, so this is not the final interface in all aspects, but BlockdevCreateOptionsQcow2 and .bdrv_co_create() are what they actually might look like in the end. In any case, this should be good enough to test whether we interpret BlockdevCreateOptions as we should.

---

qapi/block-core.json line 3447

Quoted 6 lines
qapi/block-core.json +4 -0
@@ -3442,6 +3442,18 @@
    } }
  ##
+# @x-blockdev-create:
+#
+# Create an image format on a given node.
+# TODO Replace with something asynchronous (block job?)

We've learned our lesson - don't commit to the final name on an interface that we have not yet experimented with.

qapi/block-core.json line 3455

Quoted 7 lines
qapi/block-core.json +7 -0
+#
+# Since: 2.12
+##
+{ 'command': 'x-blockdev-create',
+  'data': 'BlockdevCreateOptions',
+  'boxed': true }
+

Lots of code packed in that little description ;)

Quoted 11 lines

include/block/block_int.h struct BlockDriver { · line 134

Quoted 7 lines
include/block/block_int.h +3 -0
+++ b/include/block/block_int.h
@@ -130,6 +130,8 @@ struct BlockDriver {
      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                            Error **errp);
      void (*bdrv_close)(BlockDriverState *bs);
+    int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+                                       Error **errp);

I know we haven't been very good in the past, but can you add a comment here on the contract that drivers are supposed to obey when implementing this callback?

Anything specific you want to see here?

Essentially the meaning of BlockdevCreateOptions depends on the driver and is documented in the QAPI schema, how Error works is common knowledge, and I don't see much else to explain here.

I mean, I can add something like "Creates an image. See the QAPI documentation for BlockdevCreateOptions for details." if you think this is useful. But is it?

Quoted 5 lines

block/create.c line 59

Quoted 3 lines
block/create.c new file +3 -0
+    /* Call callback if it exists */
+    if (!drv->bdrv_co_create) {
+        error_setg(errp, "Driver does not support blockdev-create");

Should this error message refer to 'x-blockdev-create' in the short term?

Hm, it would be more correct. On the other hand, I'm almost sure we'd forget to rename it back when we remove the x- prefix...

Kevin

#36 lore raw

Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

Quoted 14 lines · Kevin Wolf
Quoted 11 lines

include/block/block_int.h struct BlockDriver { · line 134

Quoted 7 lines

+++ b/include/block/block_int.h

include/block/block_int.h +2 -0
@@ -130,6 +130,8 @@ struct BlockDriver {
       int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                             Error **errp);
       void (*bdrv_close)(BlockDriverState *bs);
+    int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+                                       Error **errp);

I know we haven't been very good in the past, but can you add a comment here on the contract that drivers are supposed to obey when implementing this callback?

Anything specific you want to see here?

Quoted 7 lines

Essentially the meaning of BlockdevCreateOptions depends on the driver and is documented in the QAPI schema, how Error works is common knowledge, and I don't see much else to explain here.

I mean, I can add something like "Creates an image. See the QAPI documentation for BlockdevCreateOptions for details." if you think this is useful. But is it?

I guess my concern is whether this interface MUST overwrite any existing data in order to convert existing storage into a newly-created image of this driver's type (even if the overwritten data previously probed as a different image type), or if it is only called at a point when any existing data would be probed as raw, or any other useful tidbits that a driver might need to know in implementing it. But if all you can think of is "See QAPI for BlockdevCreateOptions for details", then yeah, that's not worth a comment.

Quoted 9 lines
Quoted 6 lines

block/create.c line 59

Quoted 4 lines
block/create.c new file +3 -0
+    /* Call callback if it exists */
+    if (!drv->bdrv_co_create) {
+        error_setg(errp, "Driver does not support blockdev-create");

Should this error message refer to 'x-blockdev-create' in the short term?

Hm, it would be more correct. On the other hand, I'm almost sure we'd forget to rename it back when we remove the x- prefix...

Good point. And being an x- prefix implies that inconsistency may be expected (not to mention short-lived, if we promote the interface); while being consistent now but risking long-term inconsistency down the road when it actually becomes a stable interface is indeed worse. So keep this message as-is.

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

PATCH 12/27 file-posix: Support .bdrv_co_create

In reply to Kevin Wolf

This adds the .bdrv_co_create driver callback to file, which enables image creation over QMP.

---
 qapi/block-core.json | 20 +++++++++++++-
 block/file-posix.c   | 77 +++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 74 insertions(+), 23 deletions(-)
qapi/block-core.json +19 -1
index c0e61483af..6f3461c751 100644
@@ -3338,6 +3338,24 @@
 { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
 ##
+# @BlockdevCreateOptionsFile:
+#
+# Driver specific image creation options for file.
+#
+# @filename         Filename for the new image file
+# @size             Size of the virtual disk in bytes
+# @preallocation    Preallocation mode for the new image (default: off)
+# @nocow            Turn off copy-on-write (valid only on btrfs; default: off)
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsFile',
+  'data': { 'filename':         'str',
+            'size':             'size',
+            '*preallocation':   'PreallocMode',
+            '*nocow':           'bool' } }
+
+##
 # @BlockdevQcow2Version:
 #
 # @v2:  The original QCOW2 format as introduced in qemu 0.10 (version 2)
@@ -3408,7 +3426,7 @@
       'bochs':          'BlockdevCreateNotSupported',
       'cloop':          'BlockdevCreateNotSupported',
       'dmg':            'BlockdevCreateNotSupported',
-      'file':           'BlockdevCreateNotSupported',
+      'file':           'BlockdevCreateOptionsFile',
       'ftp':            'BlockdevCreateNotSupported',
       'ftps':           'BlockdevCreateNotSupported',
       'gluster':        'BlockdevCreateNotSupported',
block/file-posix.c +55 -22

114 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v1 12/27
#38 lore raw

Re: [Qemu-devel] [PATCH 12/27] file-posix: Support .bdrv_co_create

Quoted 8 lines · Kevin Wolf

This adds the .bdrv_co_create driver callback to file, which enables image creation over QMP.

---
 qapi/block-core.json | 20 +++++++++++++-
 block/file-posix.c   | 77 +++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 74 insertions(+), 23 deletions(-)
author
#39 lore raw

PATCH 13/27 file-win32: Support .bdrv_co_create

In reply to Kevin Wolf

This adds the .bdrv_co_create driver callback to file-win32, which enables image creation over QMP.

---
 block/file-win32.c | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)
block/file-win32.c +37 -8
index 9e02214a69..bd14b0e99f 100644
@@ -550,29 +550,58 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
     return st.st_size;
 }
 
-static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
+static int raw_co_create(BlockdevCreateOptions *options, Error **errp)
 {
+    BlockdevCreateOptionsFile *file_opts;
     int fd;
-    int64_t total_size = 0;
 
-    strstart(filename, "file:", &filename);
+    assert(options->driver == BLOCKDEV_DRIVER_FILE);
+    file_opts = &options->u.file;
 
-    /* Read out options */
-    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
-                          BDRV_SECTOR_SIZE);
+    if (file_opts->has_preallocation) {
+        error_setg(errp, "Preallocation is not supported on Windows");
+        return -EINVAL;
+    }
+    if (file_opts->has_nocow) {
+        error_setg(errp, "nocow is not supported on Windows");
+        return -EINVAL;
+    }
 
-    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+    fd = qemu_open(file_opts->filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
                    0644);
     if (fd < 0) {
         error_setg_errno(errp, errno, "Could not create file");
         return -EIO;
     }
     set_sparse(fd);
-    ftruncate(fd, total_size);
+    ftruncate(fd, file_opts->size);
     qemu_close(fd);
+
     return 0;
 }
 
+static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
+{
+    BlockdevCreateOptions options;
+    int64_t total_size = 0;
+
+    strstart(filename, "file:", &filename);
+
+    /* Read out options */
+    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+                          BDRV_SECTOR_SIZE);
+
+    options = (BlockdevCreateOptions) {
+        .driver     = BLOCKDEV_DRIVER_FILE,
+        .u.file     = {
+            .filename           = (char *) filename,
+            .size               = total_size,
+            .has_preallocation  = false,
+            .has_nocow          = false,
+        },
+    };
+    return raw_co_create(&options, errp);
+}
 
 static QemuOptsList raw_create_opts = {
     .name = "raw-create-opts",
Signature · 1 line
2.13.6
Reviewed-by on v1 13/27
#40 lore raw

Re: [Qemu-devel] [PATCH 13/27] file-win32: Support .bdrv_co_create

Quoted 7 lines · Kevin Wolf

This adds the .bdrv_co_create driver callback to file-win32, which enables image creation over QMP.

---
 block/file-win32.c | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)
author
#41 lore raw

PATCH 14/27 gluster: Support .bdrv_co_create

In reply to Kevin Wolf

This adds the .bdrv_co_create driver callback to gluster, which enables image creation over QMP.

---
 qapi/block-core.json |  18 ++++++-
 block/gluster.c      | 149 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 115 insertions(+), 52 deletions(-)
qapi/block-core.json +17 -1
index 6f3461c751..5b4cd6bd12 100644
@@ -3356,6 +3356,22 @@
             '*nocow':           'bool' } }
 
 ##
+# @BlockdevCreateOptionsGluster:
+#
+# Driver specific image creation options for gluster.
+#
+# @location         Where to store the new image file
+# @size             Size of the virtual disk in bytes
+# @preallocation    Preallocation mode for the new image (default: off)
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsGluster',
+  'data': { 'location':         'BlockdevOptionsGluster',
+            'size':             'size',
+            '*preallocation':   'PreallocMode' } }
+
+##
 # @BlockdevQcow2Version:
 #
 # @v2:  The original QCOW2 format as introduced in qemu 0.10 (version 2)
@@ -3429,7 +3445,7 @@
       'file':           'BlockdevCreateOptionsFile',
       'ftp':            'BlockdevCreateNotSupported',
       'ftps':           'BlockdevCreateNotSupported',
-      'gluster':        'BlockdevCreateNotSupported',
+      'gluster':        'BlockdevCreateOptionsGluster',
       'host_cdrom':     'BlockdevCreateNotSupported',
       'host_device':    'BlockdevCreateNotSupported',
       'http':           'BlockdevCreateNotSupported',
block/gluster.c +98 -51

246 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v1 14/27
#42 lore raw

Re: [Qemu-devel] [PATCH 14/27] gluster: Support .bdrv_co_create

Quoted 8 lines · Kevin Wolf

This adds the .bdrv_co_create driver callback to gluster, which enables image creation over QMP.

---
 qapi/block-core.json |  18 ++++++-
 block/gluster.c      | 149 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 115 insertions(+), 52 deletions(-)

[...]

Quoted 4 lines
block/gluster.c +0 -0
index 0f4265a3a4..b7e2b7fa2b 100644

[...]

block/gluster.c static coroutine_fn int qemu_gluster_co_pwrite_zeroes(BlockDriverState *bs, · line 982

Quoted 11 lines
block/gluster.c +3 -3
@@ -962,64 +976,33 @@ static coroutine_fn int qemu_gluster_co_pwrite_zeroes(BlockDriverState *bs,
 }
 #endif
 
-static int qemu_gluster_create(const char *filename,
-                               QemuOpts *opts, Error **errp)
+static int qemu_gluster_co_create(BlockdevCreateOptions *options,
+                                  Error **errp)
 {
-    BlockdevOptionsGluster *gconf;
+    BlockdevCreateOptionsGluster *opts = &options->u.gluster;

In the other drivers so far you have asserted first that options->driver is as expected and then retrieved the appropriate part of the union. I liked that a bit better, although of course it doesn't matter functionally.

Anyway:

Quoted 25 lines
block/gluster.c +1 -19
     struct glfs *glfs;
     struct glfs_fd *fd;
     int ret = 0;
-    PreallocMode prealloc;
-    int64_t total_size = 0;
-    char *tmp = NULL;
-    Error *local_err = NULL;
-
-    gconf = g_new0(BlockdevOptionsGluster, 1);
-    gconf->debug = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG,
-                                           GLUSTER_DEBUG_DEFAULT);
-    if (gconf->debug < 0) {
-        gconf->debug = 0;
-    } else if (gconf->debug > GLUSTER_DEBUG_MAX) {
-        gconf->debug = GLUSTER_DEBUG_MAX;
-    }
-    gconf->has_debug = true;
 
-    gconf->logfile = qemu_opt_get_del(opts, GLUSTER_OPT_LOGFILE);
-    if (!gconf->logfile) {
-        gconf->logfile = g_strdup(GLUSTER_LOGFILE_DEFAULT);
-    }
-    gconf->has_logfile = true;
+    assert(options->driver == BLOCKDEV_DRIVER_GLUSTER);
 
author
#43 lore raw

PATCH 15/27 rbd: Support .bdrv_co_create

In reply to Kevin Wolf

This adds the .bdrv_co_create driver callback to rbd, which enables image creation over QMP.

---
 qapi/block-core.json |  20 +++++++-
 block/rbd.c          | 137 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 108 insertions(+), 49 deletions(-)
qapi/block-core.json +19 -1
index 5b4cd6bd12..370fcd9584 100644
@@ -3415,6 +3415,24 @@
             '*refcount-bits':   'int' } }
 
 ##
+# @BlockdevCreateOptionsRbd:
+#
+# Driver specific image creation options for rbd/Ceph.
+#
+# @location         Where to store the new image file
+# @size             Size of the virtual disk in bytes
+# @password-secret  ID of secret providing the password
+# @cluster_size     RBD object size
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsRbd',
+  'data': { 'location':         'BlockdevOptionsRbd',
+            'size':             'size',
+            '*password-secret': 'str',
+            '*cluster-size' :   'size' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.
@@ -3462,7 +3480,7 @@
       'qed':            'BlockdevCreateNotSupported',
       'quorum':         'BlockdevCreateNotSupported',
       'raw':            'BlockdevCreateNotSupported',
-      'rbd':            'BlockdevCreateNotSupported',
+      'rbd':            'BlockdevCreateOptionsRbd',
       'replication':    'BlockdevCreateNotSupported',
       'sheepdog':       'BlockdevCreateNotSupported',
       'ssh':            'BlockdevCreateNotSupported',
block/rbd.c +89 -48

195 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v1 15/27
#44 lore raw

Re: [Qemu-devel] [PATCH 15/27] rbd: Support .bdrv_co_create

Quoted 8 lines · Kevin Wolf

This adds the .bdrv_co_create driver callback to rbd, which enables image creation over QMP.

---
 qapi/block-core.json |  20 +++++++-
 block/rbd.c          | 137 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 108 insertions(+), 49 deletions(-)

Some comments below.

qapi/block-core.json line 3422

Quoted 13 lines
qapi/block-core.json +5 -0
index 5b4cd6bd12..370fcd9584 100644
@@ -3415,6 +3415,24 @@
             '*refcount-bits':   'int' } }
 
 ##
+# @BlockdevCreateOptionsRbd:
+#
+# Driver specific image creation options for rbd/Ceph.
+#
+# @location         Where to store the new image file

Maybe this should mention that location.snapshot is not allowed?

(And that location.server is ignored. But is that even intended?)

qapi/block-core.json line 3425

Quoted 3 lines
qapi/block-core.json +3 -0
+# @size             Size of the virtual disk in bytes
+# @password-secret  ID of secret providing the password
+# @cluster_size     RBD object size

s/_/-/

Quoted 13 lines
qapi/block-core.json +10 -0
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsRbd',
+  'data': { 'location':         'BlockdevOptionsRbd',
+            'size':             'size',
+            '*password-secret': 'str',
+            '*cluster-size' :   'size' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.

[...]

Quoted 4 lines
block/rbd.c +0 -0
index a76a5e8755..c164f70167 100644

[...]

Quoted 1 line
block/rbd.c +0 -0
@@ -432,24 +409,87 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)

[...]

Quoted 35 lines
block/rbd.c +35 -0
+static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
+{
+    BlockdevCreateOptions *create_options;
+    BlockdevCreateOptionsRbd *rbd_opts;
+    BlockdevOptionsRbd *loc;
+    Error *local_err = NULL;
+    const char *keypairs;
+    QDict *options = NULL;
+    int ret = 0;
+
+    create_options = g_new0(BlockdevCreateOptions, 1);
+    create_options->driver = BLOCKDEV_DRIVER_RBD;
+    rbd_opts = &create_options->u.rbd;
+
+    rbd_opts->location = g_new0(BlockdevOptionsRbd, 1);
+
+    rbd_opts->password_secret = (char *) qemu_opt_get(opts, "password-secret");
+
+    /* Read out options */
+    rbd_opts->size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+                              BDRV_SECTOR_SIZE);
+    rbd_opts->cluster_size = qemu_opt_get_size_del(opts,
+                                                   BLOCK_OPT_CLUSTER_SIZE, 0);
+    rbd_opts->has_cluster_size = (rbd_opts->cluster_size != 0);
+
+    options = qdict_new();
+    qemu_rbd_parse_filename(filename, options, &local_err);
+    if (local_err) {
+        ret = -EINVAL;
+        error_propagate(errp, local_err);
+        goto exit;
+    }
+
+    /*
+     * Caution: while qdict_get_try_str() is fine, getting non-string

block/rbd.c static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp) · line 481

Quoted 10 lines
block/rbd.c +10 -0
+     * types would require more care.  When @options come from -blockdev
+     * or blockdev_add, its members are typed according to the QAPI
+     * schema, but when they come from -drive, they're all QString.
+     */
+    loc = rbd_opts->location;
+    loc->pool     = g_strdup(qdict_get_try_str(options, "pool"));
+    loc->conf     = g_strdup(qdict_get_try_str(options, "conf"));
+    loc->has_conf = !!rbd_opts->location->conf;
+    loc->user     = g_strdup(qdict_get_try_str(options, "user"));
+    loc->has_user = !!rbd_opts->location->user;

"!!loc->conf" and "!!loc->user" would be shorter and maybe a bit easier to get.

Max

Quoted 13 lines
block/rbd.c +8 -0
+    loc->image    = g_strdup(qdict_get_try_str(options, "image"));
+    keypairs      = qdict_get_try_str(options, "=keyvalue-pairs");
+
+    ret = qemu_rbd_do_create(create_options, keypairs, errp);
+    if (ret < 0) {
+        goto exit;
+    }
 
 exit:
     QDECREF(options);
+    qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
author
#45 lore raw

PATCH 16/27 nfs: Use QAPI options in nfs_client_open()

In reply to Kevin Wolf

Using the QAPI visitor to turn all options into QAPI BlockdevOptionsNfs simplifies the code a lot. It will also be useful for implementing the QAPI based .bdrv_co_create callback.

---
 block/nfs.c | 176 ++++++++++++++++++------------------------------------------
 1 file changed, 53 insertions(+), 123 deletions(-)
block/nfs.c +53 -123
index effc8719b5..f540c74c87 100644
@@ -367,49 +367,6 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
     return task.ret;
 }
 
-static QemuOptsList runtime_opts = {
-    .name = "nfs",
-    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
-    .desc = {
-        {
-            .name = "path",
-            .type = QEMU_OPT_STRING,
-            .help = "Path of the image on the host",
-        },
-        {
-            .name = "user",
-            .type = QEMU_OPT_NUMBER,
-            .help = "UID value to use when talking to the server",
-        },
-        {
-            .name = "group",
-            .type = QEMU_OPT_NUMBER,
-            .help = "GID value to use when talking to the server",
-        },
-        {
-            .name = "tcp-syn-count",
-            .type = QEMU_OPT_NUMBER,
-            .help = "Number of SYNs to send during the session establish",
-        },
-        {
-            .name = "readahead-size",
-            .type = QEMU_OPT_NUMBER,
-            .help = "Set the readahead size in bytes",
-        },
-        {
-            .name = "page-cache-size",
-            .type = QEMU_OPT_NUMBER,
-            .help = "Set the pagecache size in bytes",
-        },
-        {
-            .name = "debug",
-            .type = QEMU_OPT_NUMBER,
-            .help = "Set the NFS debug level (max 2)",
-        },
-        { /* end of list */ }
-    },
-};
-
 static void nfs_detach_aio_context(BlockDriverState *bs)
 {
     NFSClient *client = bs->opaque;
@@ -452,71 +409,16 @@ static void nfs_file_close(BlockDriverState *bs)
     nfs_client_close(client);
 }
 
-static NFSServer *nfs_config(QDict *options, Error **errp)
-{
-    NFSServer *server = NULL;
-    QDict *addr = NULL;
-    QObject *crumpled_addr = NULL;
-    Visitor *iv = NULL;
-    Error *local_error = NULL;
-
-    qdict_extract_subqdict(options, &addr, "server.");
-    if (!qdict_size(addr)) {
-        error_setg(errp, "NFS server address missing");
-        goto out;
-    }
-
-    crumpled_addr = qdict_crumple(addr, errp);
-    if (!crumpled_addr) {
-        goto out;
-    }
-
-    /*
-     * Caution: this works only because all scalar members of
-     * NFSServer are QString in @crumpled_addr.  The visitor expects
-     * @crumpled_addr to be typed according to the QAPI schema.  It
-     * is when @options come from -blockdev or blockdev_add.  But when
-     * they come from -drive, they're all QString.
-     */
-    iv = qobject_input_visitor_new(crumpled_addr);
-    visit_type_NFSServer(iv, NULL, &server, &local_error);
-    if (local_error) {
-        error_propagate(errp, local_error);
-        goto out;
-    }
-
-out:
-    QDECREF(addr);
-    qobject_decref(crumpled_addr);
-    visit_free(iv);
-    return server;
-}
-
-
-static int64_t nfs_client_open(NFSClient *client, QDict *options,
+static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
                                int flags, int open_flags, Error **errp)
 {
     int64_t ret = -EINVAL;
-    QemuOpts *opts = NULL;
-    Error *local_err = NULL;
     struct stat st;
     char *file = NULL, *strp = NULL;
 
     qemu_mutex_init(&client->mutex);
-    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(opts, options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto fail;
-    }
 
-    client->path = g_strdup(qemu_opt_get(opts, "path"));
-    if (!client->path) {
-        ret = -EINVAL;
-        error_setg(errp, "No path was specified");
-        goto fail;
-    }
+    client->path = g_strdup(opts->path);
 
     strp = strrchr(client->path, '/');
     if (strp == NULL) {
@@ -526,12 +428,10 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
     file = g_strdup(strp);
     *strp = 0;
 
-    /* Pop the config into our state object, Exit if invalid */
-    client->server = nfs_config(options, errp);
-    if (!client->server) {
-        ret = -EINVAL;
-        goto fail;
-    }
+    /* Steal the NFSServer object from opts; set the original pointer to NULL
+     * to avoid use after free and double free. */
+    client->server = opts->server;
+    opts->server = NULL;
 
     client->context = nfs_init_context();
     if (client->context == NULL) {
@@ -539,29 +439,29 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
         goto fail;
     }
 
-    if (qemu_opt_get(opts, "user")) {
-        client->uid = qemu_opt_get_number(opts, "user", 0);
+    if (opts->has_user) {
+        client->uid = opts->user;
         nfs_set_uid(client->context, client->uid);
     }
 
-    if (qemu_opt_get(opts, "group")) {
-        client->gid = qemu_opt_get_number(opts, "group", 0);
+    if (opts->has_group) {
+        client->gid = opts->group;
         nfs_set_gid(client->context, client->gid);
     }
 
-    if (qemu_opt_get(opts, "tcp-syn-count")) {
-        client->tcp_syncnt = qemu_opt_get_number(opts, "tcp-syn-count", 0);
+    if (opts->has_tcp_syn_count) {
+        client->tcp_syncnt = opts->tcp_syn_count;
         nfs_set_tcp_syncnt(client->context, client->tcp_syncnt);
     }
 
 #ifdef LIBNFS_FEATURE_READAHEAD
-    if (qemu_opt_get(opts, "readahead-size")) {
+    if (opts->has_readahead_size) {
         if (open_flags & BDRV_O_NOCACHE) {
             error_setg(errp, "Cannot enable NFS readahead "
                              "if cache.direct = on");
             goto fail;
         }
-        client->readahead = qemu_opt_get_number(opts, "readahead-size", 0);
+        client->readahead = opts->readahead_size;
         if (client->readahead > QEMU_NFS_MAX_READAHEAD_SIZE) {
             warn_report("Truncating NFS readahead size to %d",
                         QEMU_NFS_MAX_READAHEAD_SIZE);
@@ -576,13 +476,13 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
 #endif
 
 #ifdef LIBNFS_FEATURE_PAGECACHE
-    if (qemu_opt_get(opts, "page-cache-size")) {
+    if (opts->has_page_cache_size) {
         if (open_flags & BDRV_O_NOCACHE) {
             error_setg(errp, "Cannot enable NFS pagecache "
                              "if cache.direct = on");
             goto fail;
         }
-        client->pagecache = qemu_opt_get_number(opts, "page-cache-size", 0);
+        client->pagecache = opts->page_cache_size;
         if (client->pagecache > QEMU_NFS_MAX_PAGECACHE_SIZE) {
             warn_report("Truncating NFS pagecache size to %d pages",
                         QEMU_NFS_MAX_PAGECACHE_SIZE);
@@ -595,8 +495,8 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
 #endif
 
 #ifdef LIBNFS_FEATURE_DEBUG
-    if (qemu_opt_get(opts, "debug")) {
-        client->debug = qemu_opt_get_number(opts, "debug", 0);
+    if (opts->has_debug) {
+        client->debug = opts->debug;
         /* limit the maximum debug level to avoid potential flooding
          * of our log files. */
         if (client->debug > QEMU_NFS_MAX_DEBUG_LEVEL) {
@@ -647,11 +547,41 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
 fail:
     nfs_client_close(client);
 out:
-    qemu_opts_del(opts);
     g_free(file);
     return ret;
 }
 
+static int64_t nfs_client_open_qdict(NFSClient *client, QDict *options,
+                                     int flags, int open_flags, Error **errp)
+{
+    BlockdevOptionsNfs *opts;
+    QObject *crumpled = NULL;
+    Visitor *v;
+    Error *local_err = NULL;
+    int ret;
+
+    crumpled = qdict_crumple(options, errp);
+    if (crumpled == NULL) {
+        return -EINVAL;
+    }
+
+    v = qobject_input_visitor_new_keyval(crumpled);
+    visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err);
+    visit_free(v);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    ret = nfs_client_open(client, opts, flags, open_flags, errp);
+fail:
+    qobject_decref(crumpled);
+    qapi_free_BlockdevOptionsNfs(opts);
+    return ret;
+}
+
 static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp) {
     NFSClient *client = bs->opaque;
@@ -659,9 +589,9 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
 
     client->aio_context = bdrv_get_aio_context(bs);
 
-    ret = nfs_client_open(client, options,
-                          (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
-                          bs->open_flags, errp);
+    ret = nfs_client_open_qdict(client, options,
+                                (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
+                                bs->open_flags, errp);
     if (ret < 0) {
         return ret;
     }
@@ -702,7 +632,7 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp)
         goto out;
     }
 
-    ret = nfs_client_open(client, options, O_CREAT, 0, errp);
+    ret = nfs_client_open_qdict(client, options, O_CREAT, 0, errp);
     if (ret < 0) {
         goto out;
     }
Signature · 1 line
2.13.6
Reviewed-by on v1 16/27
#46 lore raw

Re: [Qemu-devel] [PATCH 16/27] nfs: Use QAPI options in nfs_client_open()

Quoted 8 lines · Kevin Wolf

Using the QAPI visitor to turn all options into QAPI BlockdevOptionsNfs simplifies the code a lot. It will also be useful for implementing the QAPI based .bdrv_co_create callback.

---
 block/nfs.c | 176 ++++++++++++++++++------------------------------------------
 1 file changed, 53 insertions(+), 123 deletions(-)
author
#47 lore raw

PATCH 17/27 nfs: Support .bdrv_co_create

In reply to Kevin Wolf

This adds the .bdrv_co_create driver callback to nfs, which enables image creation over QMP.

---
 qapi/block-core.json | 16 +++++++++++-
 block/nfs.c          | 74 +++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 74 insertions(+), 16 deletions(-)
qapi/block-core.json +15 -1
index 370fcd9584..4e419b147d 100644
@@ -3372,6 +3372,20 @@
             '*preallocation':   'PreallocMode' } }
 
 ##
+# @BlockdevCreateOptionsNfs:
+#
+# Driver specific image creation options for NFS.
+#
+# @location         Where to store the new image file
+# @size             Size of the virtual disk in bytes
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsNfs',
+  'data': { 'location':         'BlockdevOptionsNfs',
+            'size':             'size' } }
+
+##
 # @BlockdevQcow2Version:
 #
 # @v2:  The original QCOW2 format as introduced in qemu 0.10 (version 2)
@@ -3471,7 +3485,7 @@
       'iscsi':          'BlockdevCreateNotSupported',
       'luks':           'BlockdevCreateNotSupported',
       'nbd':            'BlockdevCreateNotSupported',
-      'nfs':            'BlockdevCreateNotSupported',
+      'nfs':            'BlockdevCreateOptionsNfs',
       'null-aio':       'BlockdevCreateNotSupported',
       'null-co':        'BlockdevCreateNotSupported',
       'parallels':      'BlockdevCreateNotSupported',
block/nfs.c +59 -15

130 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v1 17/27
#48 lore raw

Re: [Qemu-devel] [PATCH 17/27] nfs: Support .bdrv_co_create

Quoted 8 lines · Kevin Wolf

This adds the .bdrv_co_create driver callback to nfs, which enables image creation over QMP.

---
 qapi/block-core.json | 16 +++++++++++-
 block/nfs.c          | 74 +++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 74 insertions(+), 16 deletions(-)
author
#49 lore raw

PATCH 18/27 sheepdog: QAPIfy "redundacy" create option

In reply to Kevin Wolf

The "redundacy" option for Sheepdog image creation is currently a string that can encode one or two integers depending on its format, which at the same time implicitly selects a mode.

This patch turns it into a QAPI union and converts the string into such a QAPI object before interpreting the values.

---
 qapi/block-core.json | 45 ++++++++++++++++++++++++++
 block/sheepdog.c     | 89 ++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 107 insertions(+), 27 deletions(-)
qapi/block-core.json +45 -0
index 4e419b147d..7d004dddf9 100644
@@ -3447,6 +3447,51 @@
             '*cluster-size' :   'size' } }
 
 ##
+# @SheepdogRedundancyType:
+#
+# @full             Create a fully replicated vdi with x copies
+# @erasure-coded    Create an erasure coded vdi with x data strips and
+#                   y parity strips
+#
+# Since: 2.12
+##
+{ 'enum': 'SheepdogRedundancyType',
+  'data': [ 'full', 'erasure-coded' ] }
+
+##
+# @SheepdogRedundancyFull:
+#
+# @copies           Number of copies to use (between 1 and 31)
+#
+# Since: 2.12
+##
+{ 'struct': 'SheepdogRedundancyFull',
+  'data': { 'copies': 'int' }}
+
+##
+# @SheepdogRedundancyErasureCoded:
+#
+# @data-strips      Number of data strips to use (one of {2,4,8,16})
+# @parity-strips    Number of parity strips to use (between 1 and 15)
+#
+# Since: 2.12
+##
+{ 'struct': 'SheepdogRedundancyErasureCoded',
+  'data': { 'data-strips': 'int',
+            'parity-strips': 'int' }}
+
+##
+# @SheepdogRedundancy:
+#
+# Since: 2.12
+##
+{ 'union': 'SheepdogRedundancy',
+  'base': { 'type': 'SheepdogRedundancyType' },
+  'discriminator': 'type',
+  'data': { 'full': 'SheepdogRedundancyFull',
+            'erasure-coded': 'SheepdogRedundancyErasureCoded' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.
block/sheepdog.c +62 -27

119 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v1 18/27
#50 lore raw

Re: [Qemu-devel] [PATCH 18/27] sheepdog: QAPIfy "redundacy" create option

Quoted 4 lines · Kevin Wolf

The "redundacy" option for Sheepdog image creation is currently a string that can encode one or two integers depending on its format, which at the same time implicitly selects a mode.

Quoted 8 lines

This patch turns it into a QAPI union and converts the string into such a QAPI object before interpreting the values.

---
 qapi/block-core.json | 45 ++++++++++++++++++++++++++
 block/sheepdog.c     | 89 ++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 107 insertions(+), 27 deletions(-)
Quoted 4 lines
block/sheepdog.c +0 -0
index f684477328..dc0348f120 100644

[...]

block/sheepdog.c static int parse_redundancy(BDRVSheepdogState *s, const char *opt) · line 1957

Quoted 6 lines
block/sheepdog.c +0 -1
@@ -1912,35 +1954,28 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
         return -EINVAL;
     }
 
-    copy = strtol(n1, NULL, 10);
     /* FIXME fix error checking by switching to qemu_strtol() */

But this is not the time? ;-)

Quoted 10 lines
block/sheepdog.c +1 -8
-    if (copy > SD_MAX_COPIES || copy < 1) {
-        return -EINVAL;
-    }
-    if (!n2) {
-        inode->copy_policy = 0;
-        inode->nr_copies = copy;
-        return 0;
-    }
+    copy = strtol(n1, NULL, 10);
 
author
#51 lore raw

PATCH 19/27 sheepdog: Support .bdrv_co_create

In reply to Kevin Wolf

This adds the .bdrv_co_create driver callback to sheepdog, which enables image creation over QMP.

---
 qapi/block-core.json |  24 +++++-
 block/sheepdog.c     | 209 ++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 170 insertions(+), 63 deletions(-)
qapi/block-core.json +23 -1
index 7d004dddf9..08217e3e38 100644
@@ -3492,6 +3492,28 @@
             'erasure-coded': 'SheepdogRedundancyErasureCoded' } }
 
 ##
+# @BlockdevCreateOptionsSheepdog:
+#
+# Driver specific image creation options for Sheepdog.
+#
+# @location         Where to store the new image file
+# @size             Size of the virtual disk in bytes
+# @backing_file     File name of a base image
+# @preallocation    Preallocation mode (allowed values: off, full)
+# @redundancy       Redundancy of the image
+# @object-size      Object size of the image
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsSheepdog',
+  'data': { 'location':         'BlockdevOptionsSheepdog',
+            'size':             'size',
+            '*backing-file':    'str',
+            '*preallocation':   'PreallocMode',
+            '*redundancy':      'SheepdogRedundancy',
+            '*object-size':     'size' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.
@@ -3541,7 +3563,7 @@
       'raw':            'BlockdevCreateNotSupported',
       'rbd':            'BlockdevCreateOptionsRbd',
       'replication':    'BlockdevCreateNotSupported',
-      'sheepdog':       'BlockdevCreateNotSupported',
+      'sheepdog':       'BlockdevCreateOptionsSheepdog',
       'ssh':            'BlockdevCreateNotSupported',
       'throttle':       'BlockdevCreateNotSupported',
       'vdi':            'BlockdevCreateNotSupported',
block/sheepdog.c +147 -62

351 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v1 19/27
#52 lore raw

Re: [Qemu-devel] [PATCH 19/27] sheepdog: Support .bdrv_co_create

Quoted 14 lines · Kevin Wolf

This adds the .bdrv_co_create driver callback to sheepdog, which enables image creation over QMP.

---
 qapi/block-core.json |  24 +++++-
 block/sheepdog.c     | 209 ++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 170 insertions(+), 63 deletions(-)
qapi/block-core.json +0 -0
index 7d004dddf9..08217e3e38 100644

qapi/block-core.json line 3501

Quoted 10 lines
qapi/block-core.json +7 -0
@@ -3492,6 +3492,28 @@
             'erasure-coded': 'SheepdogRedundancyErasureCoded' } }
 
 ##
+# @BlockdevCreateOptionsSheepdog:
+#
+# Driver specific image creation options for Sheepdog.
+#
+# @location         Where to store the new image file
+# @size             Size of the virtual disk in bytes
+# @backing_file     File name of a base image

s/_/-/

With that fixed:

Quoted 18 lines
qapi/block-core.json +15 -0
+# @preallocation    Preallocation mode (allowed values: off, full)
+# @redundancy       Redundancy of the image
+# @object-size      Object size of the image
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsSheepdog',
+  'data': { 'location':         'BlockdevOptionsSheepdog',
+            'size':             'size',
+            '*backing-file':    'str',
+            '*preallocation':   'PreallocMode',
+            '*redundancy':      'SheepdogRedundancy',
+            '*object-size':     'size' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.

[...]

Quoted 3 lines

index dc0348f120..05129dc809 100644

block/sheepdog.c +0 -0

[...]

Quoted 1 line
block/sheepdog.c +0 -0
@@ -2142,15 +2144,95 @@ static int sd_create(const char *filename, QemuOpts *opts,

[...]

Quoted 3 lines
block/sheepdog.c +3 -0
+static int sd_create(const char *filename, QemuOpts *opts,
+                     Error **errp)
+{

[...]

block/sheepdog.c static int sd_create(const char *filename, QemuOpts *opts, · line 2216

Quoted 13 lines
block/sheepdog.c +13 -0
+    v = qobject_input_visitor_new_keyval(crumpled);
+    visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+    visit_free(v);
+    qobject_decref(crumpled);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    create_options->u.sheepdog.size =
+        ROUND_UP(create_options->u.sheepdog.size, BDRV_SECTOR_SIZE);

I think I'd prefer an assertion that the type is indeed sheepdog before this.

Quoted 11 lines
block/sheepdog.c +11 -0
+
+    if (redundancy) {
+        create_options->u.sheepdog.has_redundancy = true;
+        create_options->u.sheepdog.redundancy =
+            parse_redundancy_str(redundancy);
+        if (create_options->u.sheepdog.redundancy == NULL) {
+            error_setg(errp, "Invalid redundancy mode");
+            ret = -EINVAL;
+            goto fail;
+        }
+    }
author
#53 lore raw

PATCH 20/27 ssh: Use QAPI BlockdevOptionsSsh object

In reply to Kevin Wolf

Create a BlockdevOptionsSsh object in connect_to_ssh() and take the options from there. 'host_key_check' is still processed separately because it's not in the schema yet.

---
 block/ssh.c | 136 +++++++++++++++++++++++++++---------------------------------
 1 file changed, 61 insertions(+), 75 deletions(-)
block/ssh.c +61 -75
index b049a16eb9..691080b560 100644
@@ -541,21 +541,6 @@ static QemuOptsList ssh_runtime_opts = {
             .type = QEMU_OPT_NUMBER,
             .help = "Port to connect to",
         },
-        {
-            .name = "path",
-            .type = QEMU_OPT_STRING,
-            .help = "Path of the image on the host",
-        },
-        {
-            .name = "user",
-            .type = QEMU_OPT_STRING,
-            .help = "User as which to connect",
-        },
-        {
-            .name = "host_key_check",
-            .type = QEMU_OPT_STRING,
-            .help = "Defines how and what to check the host key against",
-        },
     },
 };
 
@@ -579,23 +564,31 @@ static bool ssh_process_legacy_socket_options(QDict *output_opts,
     return true;
 }
 
-static InetSocketAddress *ssh_config(QDict *options, Error **errp)
+static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp)
 {
-    InetSocketAddress *inet = NULL;
-    QDict *addr = NULL;
-    QObject *crumpled_addr = NULL;
-    Visitor *iv = NULL;
-    Error *local_error = NULL;
-
-    qdict_extract_subqdict(options, &addr, "server.");
-    if (!qdict_size(addr)) {
-        error_setg(errp, "SSH server address missing");
-        goto out;
+    BlockdevOptionsSsh *result = NULL;
+    QemuOpts *opts = NULL;
+    Error *local_err = NULL;
+    QObject *crumpled;
+    const QDictEntry *e;
+    Visitor *v;
+
+    /* Translate legacy options */
+    opts = qemu_opts_create(&ssh_runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto fail;
     }
 
-    crumpled_addr = qdict_crumple(addr, errp);
-    if (!crumpled_addr) {
-        goto out;
+    if (!ssh_process_legacy_socket_options(options, opts, errp)) {
+        goto fail;
+    }
+
+    /* Create the QAPI object */
+    crumpled = qdict_crumple(options, errp);
+    if (crumpled == NULL) {
+        goto fail;
     }
 
     /*
@@ -606,51 +599,50 @@ static InetSocketAddress *ssh_config(QDict *options, Error **errp)
      * but when they come from -drive, they're all QString.  The
      * visitor expects the former.
      */
-    iv = qobject_input_visitor_new(crumpled_addr);
-    visit_type_InetSocketAddress(iv, NULL, &inet, &local_error);
-    if (local_error) {
-        error_propagate(errp, local_error);
-        goto out;
+    v = qobject_input_visitor_new(crumpled);
+    visit_type_BlockdevOptionsSsh(v, NULL, &result, &local_err);
+    visit_free(v);
+    qobject_decref(crumpled);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto fail;
     }
 
-out:
-    QDECREF(addr);
-    qobject_decref(crumpled_addr);
-    visit_free(iv);
-    return inet;
+    /* Remove the processed options from the QDict (the visitor processes
+     * _all_ options in the QDict) */
+    while ((e = qdict_first(options))) {
+        qdict_del(options, e->key);
+    }
+
+fail:
+    qemu_opts_del(opts);
+    return result;
 }
 
 static int connect_to_ssh(BDRVSSHState *s, QDict *options,
                           int ssh_flags, int creat_mode, Error **errp)
 {
+    BlockdevOptionsSsh *opts;
     int r, ret;
-    QemuOpts *opts = NULL;
-    Error *local_err = NULL;
-    const char *user, *path, *host_key_check;
+    const char *user, *host_key_check;
     long port = 0;
 
-    opts = qemu_opts_create(&ssh_runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(opts, options, &local_err);
-    if (local_err) {
-        ret = -EINVAL;
-        error_propagate(errp, local_err);
-        goto err;
-    }
-
-    if (!ssh_process_legacy_socket_options(options, opts, errp)) {
-        ret = -EINVAL;
-        goto err;
+    host_key_check = qdict_get_try_str(options, "host_key_check");
+    if (!host_key_check) {
+        host_key_check = "yes";
+    } else {
+        qdict_del(options, "host_key_check");
     }
 
-    path = qemu_opt_get(opts, "path");
-    if (!path) {
-        ret = -EINVAL;
-        error_setg(errp, "No path was specified");
-        goto err;
+    opts = ssh_parse_options(options, errp);
+    if (opts == NULL) {
+        return -EINVAL;
     }
 
-    user = qemu_opt_get(opts, "user");
-    if (!user) {
+    if (opts->has_user) {
+        user = opts->user;
+    } else {
         user = g_get_user_name();
         if (!user) {
             error_setg_errno(errp, errno, "Can't get user name");
@@ -659,17 +651,9 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
         }
     }
 
-    host_key_check = qemu_opt_get(opts, "host_key_check");
-    if (!host_key_check) {
-        host_key_check = "yes";
-    }
-
     /* Pop the config into our state object, Exit if invalid */
-    s->inet = ssh_config(options, errp);
-    if (!s->inet) {
-        ret = -EINVAL;
-        goto err;
-    }
+    s->inet = opts->server;
+    opts->server = NULL;
 
     if (qemu_strtol(s->inet->port, NULL, 10, &port) < 0) {
         error_setg(errp, "Use only numeric port value");
@@ -726,15 +710,17 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 
     /* Open the remote file. */
     DPRINTF("opening file %s flags=0x%x creat_mode=0%o",
-            path, ssh_flags, creat_mode);
-    s->sftp_handle = libssh2_sftp_open(s->sftp, path, ssh_flags, creat_mode);
+            opts->path, ssh_flags, creat_mode);
+    s->sftp_handle = libssh2_sftp_open(s->sftp, opts->path, ssh_flags,
+                                       creat_mode);
     if (!s->sftp_handle) {
-        session_error_setg(errp, s, "failed to open remote file '%s'", path);
+        session_error_setg(errp, s, "failed to open remote file '%s'",
+                           opts->path);
         ret = -EINVAL;
         goto err;
     }
 
-    qemu_opts_del(opts);
+    qapi_free_BlockdevOptionsSsh(opts);
 
     r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs);
     if (r < 0) {
@@ -761,7 +747,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     }
     s->session = NULL;
 
-    qemu_opts_del(opts);
+    qapi_free_BlockdevOptionsSsh(opts);
 
     return ret;
 }
Signature · 1 line
2.13.6
Reviewed-by on v1 20/27
#54 lore raw

Re: [Qemu-devel] [PATCH 20/27] ssh: Use QAPI BlockdevOptionsSsh object

Quoted 8 lines · Kevin Wolf

Create a BlockdevOptionsSsh object in connect_to_ssh() and take the options from there. 'host_key_check' is still processed separately because it's not in the schema yet.

---
 block/ssh.c | 136 +++++++++++++++++++++++++++---------------------------------
 1 file changed, 61 insertions(+), 75 deletions(-)

You might want to base this patch on Pino's patch to use libssh instead of libssh2.

Anyway:

author
#55 lore raw

PATCH 21/27 ssh: QAPIfy host-key-check option

In reply to Kevin Wolf

This makes the host-key-check option available in blockdev-add.

---
 qapi/block-core.json | 63 +++++++++++++++++++++++++++++++++++--
 block/ssh.c          | 88 +++++++++++++++++++++++++++++++++-------------------
 2 files changed, 117 insertions(+), 34 deletions(-)
qapi/block-core.json +61 -2
index 08217e3e38..7ad25ce372 100644
@@ -2533,6 +2533,63 @@
             '*encrypt': 'BlockdevQcow2Encryption' } }
 
 ##
+# @SshHostKeyCheckMode:
+#
+# @none             Don't check the host key at all
+# @hash             Compare the host key with a given hash
+# @known_hosts      Check the host key against the known_hosts file
+#
+# Since: 2.12
+##
+{ 'enum': 'SshHostKeyCheckMode',
+  'data': [ 'none', 'hash', 'known_hosts' ] }
+
+##
+# @SshHostKeyCheckHashType:
+#
+# @md5              The given hash is an md5 hash
+# @sha1             The given hash is an sha1 hash
+#
+# Since: 2.12
+##
+{ 'enum': 'SshHostKeyCheckHashType',
+  'data': [ 'md5', 'sha1' ] }
+
+##
+# @SshHostKeyHash:
+#
+# @type             The hash algorithm used for the hash
+# @hash             The expected hash value
+#
+# Since: 2.12
+##
+{ 'struct': 'SshHostKeyHash',
+  'data': { 'type': 'SshHostKeyCheckHashType',
+            'hash': 'str' }}
+
+##
+# @SshHostKeyDummy:
+#
+# For those union branches that don't need additional fields.
+#
+# Since: 2.12
+##
+{ 'struct': 'SshHostKeyDummy',
+  'data': {} }
+
+##
+# @SshHostKeyCheck:
+#
+# Since: 2.12
+##
+{ 'union': 'SshHostKeyCheck',
+  'base': { 'mode': 'SshHostKeyCheckMode' },
+  'discriminator': 'mode',
+  'data': { 'none': 'SshHostKeyDummy',
+            'hash': 'SshHostKeyHash',
+            'known_hosts': 'SshHostKeyDummy' } }
+
+##
 # @BlockdevOptionsSsh:
 #
 # @server:              host address
@@ -2542,14 +2599,16 @@
 # @user:                user as which to connect, defaults to current
 #                       local user name
 #
-# TODO: Expose the host_key_check option in QMP
+# @host-key-check:      Defines how and what to check the host key against
+#                       (default: known_hosts)
 #
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsSsh',
   'data': { 'server': 'InetSocketAddress',
             'path': 'str',
-            '*user': 'str' } }
+            '*user': 'str',
+            '*host-key-check': 'SshHostKeyCheck' } }
 
 
 ##
block/ssh.c +56 -32

139 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v1 21/27
#56 lore raw

Re: [Qemu-devel] [PATCH 21/27] ssh: QAPIfy host-key-check option

Quoted 7 lines · Kevin Wolf

This makes the host-key-check option available in blockdev-add.

---
 qapi/block-core.json | 63 +++++++++++++++++++++++++++++++++++--
 block/ssh.c          | 88 +++++++++++++++++++++++++++++++++-------------------
 2 files changed, 117 insertions(+), 34 deletions(-)

(And it looks like even with the libssh patch only md5 and sha1 are available.)

author
#57 lore raw

PATCH 22/27 ssh: Pass BlockdevOptionsSsh to connect_to_ssh()

In reply to Kevin Wolf

Move the parsing of the QDict options up to the callers, in preparation for the .bdrv_co_create implementation that directly gets a QAPI type.

---
 block/ssh.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)
block/ssh.c +21 -13
index d565d876d3..776d722353 100644
@@ -652,19 +652,13 @@ fail:
     return result;
 }
 
-static int connect_to_ssh(BDRVSSHState *s, QDict *options,
+static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
                           int ssh_flags, int creat_mode, Error **errp)
 {
-    BlockdevOptionsSsh *opts;
     int r, ret;
     const char *user;
     long port = 0;
 
-    opts = ssh_parse_options(options, errp);
-    if (opts == NULL) {
-        return -EINVAL;
-    }
-
     if (opts->has_user) {
         user = opts->user;
     } else {
@@ -744,8 +738,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
         goto err;
     }
 
-    qapi_free_BlockdevOptionsSsh(opts);
-
     r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs);
     if (r < 0) {
         sftp_error_setg(errp, s, "failed to read file attributes");
@@ -771,8 +763,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     }
     s->session = NULL;
 
-    qapi_free_BlockdevOptionsSsh(opts);
-
     return ret;
 }
 
@@ -780,6 +770,7 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
                          Error **errp)
 {
     BDRVSSHState *s = bs->opaque;
+    BlockdevOptionsSsh *opts;
     int ret;
     int ssh_flags;
 
@@ -790,8 +781,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
         ssh_flags |= LIBSSH2_FXF_WRITE;
     }
 
+    opts = ssh_parse_options(options, errp);
+    if (opts == NULL) {
+        return -EINVAL;
+    }
+
     /* Start up SSH. */
-    ret = connect_to_ssh(s, options, ssh_flags, 0, errp);
+    ret = connect_to_ssh(s, opts, ssh_flags, 0, errp);
     if (ret < 0) {
         goto err;
     }
@@ -799,6 +795,8 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
     /* Go non-blocking. */
     libssh2_session_set_blocking(s->session, 0);
 
+    qapi_free_BlockdevOptionsSsh(opts);
+
     return 0;
 
  err:
@@ -807,6 +805,8 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
     }
     s->sock = -1;
 
+    qapi_free_BlockdevOptionsSsh(opts);
+
     return ret;
 }
 
@@ -828,6 +828,7 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp)
     int r, ret;
     int64_t total_size = 0;
     QDict *uri_options = NULL;
+    BlockdevOptionsSsh *ssh_opts = NULL;
     BDRVSSHState s;
     ssize_t r2;
     char c[1] = { '\0' };
@@ -846,7 +847,13 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp)
         goto out;
     }
 
-    r = connect_to_ssh(&s, uri_options,
+    ssh_opts = ssh_parse_options(uri_options, errp);
+    if (ssh_opts == NULL) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    r = connect_to_ssh(&s, ssh_opts,
                        LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
                        LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
                        0644, errp);
@@ -873,6 +880,7 @@ static int ssh_create(const char *filename, QemuOpts *opts, Error **errp)
     if (uri_options != NULL) {
         QDECREF(uri_options);
     }
+    qapi_free_BlockdevOptionsSsh(ssh_opts);
     return ret;
 }
 
Signature · 1 line
2.13.6
Reviewed-by on v1 22/27
#58 lore raw

Re: [Qemu-devel] [PATCH 22/27] ssh: Pass BlockdevOptionsSsh to connect_to_ssh()

Quoted 7 lines · Kevin Wolf

Move the parsing of the QDict options up to the callers, in preparation for the .bdrv_co_create implementation that directly gets a QAPI type.

---
 block/ssh.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)
author
#59 lore raw

PATCH 23/27 ssh: Support .bdrv_co_create

In reply to Kevin Wolf

This adds the .bdrv_co_create driver callback to ssh, which enables image creation over QMP.

---
 qapi/block-core.json | 16 ++++++++-
 block/ssh.c          | 92 +++++++++++++++++++++++++++++-----------------------
 2 files changed, 67 insertions(+), 41 deletions(-)
qapi/block-core.json +15 -1
index 7ad25ce372..9b90efb00e 100644
@@ -3573,6 +3573,20 @@
             '*object-size':     'size' } }
 
 ##
+# @BlockdevCreateOptionsSsh:
+#
+# Driver specific image creation options for SSH.
+#
+# @location         Where to store the new image file
+# @size             Size of the virtual disk in bytes
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsSsh',
+  'data': { 'location':         'BlockdevOptionsSsh',
+            'size':             'size' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.
@@ -3623,7 +3637,7 @@
       'rbd':            'BlockdevCreateOptionsRbd',
       'replication':    'BlockdevCreateNotSupported',
       'sheepdog':       'BlockdevCreateOptionsSheepdog',
-      'ssh':            'BlockdevCreateNotSupported',
+      'ssh':            'BlockdevCreateOptionsSsh',
       'throttle':       'BlockdevCreateNotSupported',
       'vdi':            'BlockdevCreateNotSupported',
       'vhdx':           'BlockdevCreateNotSupported',
block/ssh.c +52 -40

122 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v1 23/27
#60 lore raw

Re: [Qemu-devel] [PATCH 23/27] ssh: Support .bdrv_co_create

Quoted 8 lines · Kevin Wolf

This adds the .bdrv_co_create driver callback to ssh, which enables image creation over QMP.

---
 qapi/block-core.json | 16 ++++++++-
 block/ssh.c          | 92 +++++++++++++++++++++++++++++-----------------------
 2 files changed, 67 insertions(+), 41 deletions(-)
author
#61 lore raw

PATCH 24/27 file-posix: Fix no-op bdrv_truncate() with falloc preallocation

In reply to Kevin Wolf

If bdrv_truncate() is called, but the requested size is the same as before, don't call posix_fallocate(), which returns -EINVAL for length zero and would therefore make bdrv_truncate() fail.

The problem can be triggered by creating a zero-sized raw image with 'falloc' preallocation mode.

---
 block/file-posix.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
block/file-posix.c +9 -5
index 9ae5b7dcdf..15819a00b9 100644
@@ -1683,11 +1683,15 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
          * file systems that do not support fallocate(), trying to check if a
          * block is allocated before allocating it, so don't do that here.
          */
-        result = -posix_fallocate(fd, current_length, offset - current_length);
-        if (result != 0) {
-            /* posix_fallocate() doesn't set errno. */
-            error_setg_errno(errp, -result,
-                             "Could not preallocate new data");
+        if (offset != current_length) {
+            result = -posix_fallocate(fd, current_length, offset - current_length);
+            if (result != 0) {
+                /* posix_fallocate() doesn't set errno. */
+                error_setg_errno(errp, -result,
+                                 "Could not preallocate new data");
+            }
+        } else {
+            result = 0;
         }
         goto out;
 #endif
Signature · 1 line
2.13.6
Reviewed-by on v1 24/27
#62 lore raw

Re: [Qemu-devel] [PATCH 24/27] file-posix: Fix no-op bdrv_truncate() with falloc preallocation

Quoted 11 lines · Kevin Wolf

If bdrv_truncate() is called, but the requested size is the same as before, don't call posix_fallocate(), which returns -EINVAL for length zero and would therefore make bdrv_truncate() fail.

The problem can be triggered by creating a zero-sized raw image with 'falloc' preallocation mode.

---
 block/file-posix.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
author
#63 lore raw

PATCH 25/27 block: Fail bdrv_truncate() with negative size

In reply to Kevin Wolf

Most callers have their own checks, but something like this should also be checked centrally. As it happens, x-blockdev-create can pass negative image sizes to format drivers (because there is no QAPI type that would reject negative numbers) and triggers the check added by this patch.

---
 block.c | 5 +++++
 1 file changed, 5 insertions(+)
block.c +5 -0
index 725c33e53f..59e74a5ebc 100644
@@ -3681,6 +3681,11 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,
         error_setg(errp, "No medium inserted");
         return -ENOMEDIUM;
     }
+    if (offset < 0) {
+        error_setg(errp, "Image size cannot be negative");
+        return -EINVAL;
+    }
+
     if (!drv->bdrv_truncate) {
         if (bs->file && drv->is_filter) {
             return bdrv_truncate(bs->file, offset, prealloc, errp);
Signature · 1 line
2.13.6
Reviewed-by on v1 25/27
#64 lore raw

Re: [Qemu-devel] [PATCH 25/27] block: Fail bdrv_truncate() with negative size

Quoted 9 lines · Kevin Wolf

Most callers have their own checks, but something like this should also be checked centrally. As it happens, x-blockdev-create can pass negative image sizes to format drivers (because there is no QAPI type that would reject negative numbers) and triggers the check added by this patch.

---
 block.c | 5 +++++
 1 file changed, 5 insertions(+)
author
#65 lore raw

PATCH 26/27 qemu-iotests: Test qcow2 over file image creation with QMP

In reply to Kevin Wolf
---
 tests/qemu-iotests/206     | 436 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/206.out | 209 ++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 646 insertions(+)
 create mode 100755 tests/qemu-iotests/206
 create mode 100644 tests/qemu-iotests/206.out
tests/qemu-iotests/206 new file +436 -0
new file mode 100755
index 0000000000..0a18b2b19a
@@ -0,0 +1,436 @@
+#!/bin/bash
+#
+# Test qcow2 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 qcow2
+_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
+
+echo
+echo "=== Successful image creation (inline blockdev-add, 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,
+      "preallocation": "off",
+      "nocow": false
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "version": "v3",
+      "cluster-size": 65536,
+      "preallocation": "off",
+      "lazy-refcounts": false,
+      "refcount-bits": 16
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific
+
+echo
+echo "=== Successful image creation (v3 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,
+      "preallocation": "falloc",
+      "nocow": true
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "version": "v3",
+      "cluster-size": 2097152,
+      "preallocation": "metadata",
+      "lazy-refcounts": true,
+      "refcount-bits": 1
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific
+
+echo
+echo "=== Successful image creation (v2 non-default options) ==="
+echo
+
+mv $TEST_IMG $TEST_IMG.base
+
+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,
+      "backing-file": "$TEST_IMG.base",
+      "backing-fmt": "qcow2",
+      "version": "v2",
+      "cluster-size": 512
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific
+
+echo
+echo "=== Successful image creation (encrypted) ==="
+echo
+
+run_qemu -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "size": $size,
+      "encrypt": {
+          "format": "luks",
+          "key-secret": "keysec0",
+          "cipher-alg": "twofish-128",
+          "cipher-mode": "ctr",
+          "ivgen-alg": "plain64",
+          "ivgen-hash-alg": "md5",
+          "hash-alg": "sha1",
+          "iter-time": 10
+      }
+  }
+}
+{ "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 "=== 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 qcow2 can't handle images this 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": "quit" }
+EOF
+
+echo
+echo "=== Invalid version ==="
+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,
+      "version": "v1"
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "version": "v2",
+      "lazy-refcounts": true
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "version": "v2",
+      "refcount-bits": 8
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Invalid backing file options ==="
+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,
+      "backing-file": "/dev/null",
+      "preallocation": "full"
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "backing-fmt": "$IMGFMT"
+  }
+}
+{ "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": 4194304
+  }
+}
+{ "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
+
+echo
+echo "=== Invalid refcount width ==="
+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,
+      "refcount-bits": 128
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "refcount-bits": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 67108864,
+      "refcount-bits": 7
+  }
+}
+{ "execute": "quit" }
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
tests/qemu-iotests/206.out new file +209 -0

209 lines not on this page · raw message

tests/qemu-iotests/group +1 -0
index a2dfe79d86..69b6504f67 100644
@@ -202,3 +202,4 @@
 203 rw auto
 204 rw auto quick
 205 rw auto quick
+206 rw auto
Signature · 1 line
2.13.6
Reviewed-by on v1 26/27
#66 lore raw

Re: [Qemu-devel] [PATCH 26/27] qemu-iotests: Test qcow2 over file image creation with QMP

Quoted 8 lines · Kevin Wolf
---
 tests/qemu-iotests/206     | 436 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/206.out | 209 ++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 646 insertions(+)
 create mode 100755 tests/qemu-iotests/206
 create mode 100644 tests/qemu-iotests/206.out
Quoted 6 lines
tests/qemu-iotests/206 new file +0 -0
new file mode 100755
index 0000000000..0a18b2b19a
@@ -0,0 +1,436 @@

[...]

tests/qemu-iotests/206 line 28

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

Hmmm... Didn't we want to remove this boilerplate at some point?

Quoted 9 lines
tests/qemu-iotests/206 new file +9 -0
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+

[...]

Quoted 17 lines
tests/qemu-iotests/206 new file +17 -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 qcow2 can't handle images this size)
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {

tests/qemu-iotests/206 line 270

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

I was about to propose $((2**64 - 512)), but then I noticed that yields -512. Nice.

Quoted 18 lines
tests/qemu-iotests/206 new file +18 -0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775808
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "size": 9223372036854775296
+  }
+}
+{ "execute": "quit" }
+EOF
author
#67 lore raw

PATCH 27/27 qemu-iotests: Test ssh image creation over QMP

In reply to Kevin Wolf
---
 tests/qemu-iotests/207     | 261 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/207.out |  75 +++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 337 insertions(+)
 create mode 100755 tests/qemu-iotests/207
 create mode 100644 tests/qemu-iotests/207.out
tests/qemu-iotests/207 new file +261 -0
new file mode 100755
index 0000000000..f5c77852d1
@@ -0,0 +1,261 @@
+#!/bin/bash
+#
+# Test ssh 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 raw
+_supported_proto ssh
+_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
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "$TEST_IMG_FILE",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          }
+      },
+      "size": 4194304
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+echo
+TEST_IMG=$TEST_IMG_FILE _img_info | _filter_img_info
+
+echo
+echo "=== Test host-key-check options ==="
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "$TEST_IMG_FILE",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          },
+          "host-key-check": {
+              "mode": "none"
+          }
+      },
+      "size": 8388608
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "$TEST_IMG_FILE",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          },
+          "host-key-check": {
+              "mode": "known_hosts"
+          }
+      },
+      "size": 4194304
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+
+key=$(ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" |
+      cut -d" " -f3 | base64 -d | md5sum -b | cut -d" " -f1)
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "$TEST_IMG_FILE",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          },
+          "host-key-check": {
+              "mode": "hash",
+              "type": "md5",
+              "hash": "wrong"
+          }
+      },
+      "size": 8388608
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "$TEST_IMG_FILE",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          },
+          "host-key-check": {
+              "mode": "hash",
+              "type": "md5",
+              "hash": "$key"
+          }
+      },
+      "size": 8388608
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+
+key=$(ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" |
+      cut -d" " -f3 | base64 -d | sha1sum -b | cut -d" " -f1)
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "$TEST_IMG_FILE",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          },
+          "host-key-check": {
+              "mode": "hash",
+              "type": "sha1",
+              "hash": "wrong"
+          }
+      },
+      "size": 4194304
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "$TEST_IMG_FILE",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          },
+          "host-key-check": {
+              "mode": "hash",
+              "type": "sha1",
+              "hash": "$key"
+          }
+      },
+      "size": 4194304
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+echo
+echo "=== Invalid path and user ==="
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "/this/is/not/an/existing/path",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          }
+      },
+      "size": 4194304
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "ssh",
+      "location": {
+          "path": "$TEST_IMG_FILE",
+          "user": "invalid user",
+          "server": {
+              "host": "127.0.0.1",
+              "port": "22"
+          }
+      },
+      "size": 4194304
+  }
+}
+{ "execute": "quit" }
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
tests/qemu-iotests/207.out new file +75 -0

75 lines not on this page · raw message

tests/qemu-iotests/group +1 -0
index 69b6504f67..c401791fcd 100644
@@ -203,3 +203,4 @@
 204 rw auto quick
 205 rw auto quick
 206 rw auto
+207 rw auto
Signature · 1 line
2.13.6
Reviewed-by on v1 27/27
#68 lore raw

Re: [Qemu-devel] [PATCH 27/27] qemu-iotests: Test ssh image creation over QMP

Quoted 8 lines · Kevin Wolf
---
 tests/qemu-iotests/207     | 261 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/207.out |  75 +++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 337 insertions(+)
 create mode 100755 tests/qemu-iotests/207
 create mode 100644 tests/qemu-iotests/207.out