Skip to contentSkip to conversation

x-blockdev-create for protocols and qcow2

PATCH Superseded Kevin Wolf 3 participants 77 messages Feb 21, 2018 – Feb 26, 2018 Block layer

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

v2 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.

v2:
- Patch 1 ('block/qapi: Introduce BlockdevCreateOptions'):
Added nvme as unsupported driver

- Patch 8 ('util: Add qemu_opts_to_qdict_filtered'):
Fixed use after free with QemuOpts that contained more than one option with the same name, documented the behaviour with them

- Patches 9 and 10 (new):
Added unit tests for qemu_opts_append() and
qemu_opts_to_qdict_filtered()

- Patch 11 ('qdict: Introduce qdict_rename_keys()'):
Added unit test, improved documentation

- Patch 12 ('qcow2: Use visitor for options in qcow2_create()'):
Improved commit message, removed unnecessary movement of declaration

- Patches 13 and 14 ('block: x-blockdev-create QMP command'):
Move making bdrv_is_whitelisted() public into a separate patch, use read-write driver whitelist instead of read-only

- Patch 17 ('gluster: Support .bdrv_co_create'):
Rebased on top of preallocated truncate

- Patches 18-24 ('rbd: Support .bdrv_co_create'):
QAPIfied .bdrv_open() implementation so that it can be shared with .bdrv_co_create() and specified servers are actually used instead of silently ignored

- Patch 25 ('nfs: Use QAPI options in nfs_client_open()'):
Fixed use of uninitialised variable in the error path

- Patch 27 ('sheepdog: QAPIfy "redundacy" create option'):
Addressed FIXME to use qemu_strtol()

- Patch 28 ('sheepdog: Support .bdrv_co_create'):
Renamed 'backing_file' to 'backing-file', rebased on top of preallocated truncate

git-backport-diff compared to v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/36:[0001] [FC] 'block/qapi: Introduce BlockdevCreateOptions' 002/36:[----] [-C] 'block/qapi: Add qcow2 create options to schema' 003/36:[----] [--] 'qcow2: Let qcow2_create() handle protocol layer' 004/36:[----] [--] 'qcow2: Pass BlockdevCreateOptions to qcow2_create2()' 005/36:[----] [-C] 'qcow2: Use BlockdevRef in qcow2_create2()' 006/36:[----] [--] 'qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()' 007/36:[----] [--] 'qcow2: Handle full/falloc preallocation in qcow2_create2()' 008/36:[0007] [FC] 'util: Add qemu_opts_to_qdict_filtered()' 009/36:[down] 'test-qemu-opts: Test qemu_opts_append()'
010/36:[down] 'test-qemu-opts: Test qemu_opts_to_qdict_filtered()' 011/36:[0117] [FC] 'qdict: Introduce qdict_rename_keys()' 012/36:[0002] [FC] 'qcow2: Use visitor for options in qcow2_create()' 013/36:[down] 'block: Make bdrv_is_whitelisted() public'
014/36:[0006] [FC] 'block: x-blockdev-create QMP command' 015/36:[----] [-C] 'file-posix: Support .bdrv_co_create'
016/36:[----] [--] 'file-win32: Support .bdrv_co_create'
017/36:[0130] [FC] 'gluster: Support .bdrv_co_create'
018/36:[down] 'rbd: Fix use after free in qemu_rbd_set_keypairs() error path' 019/36:[down] 'rbd: Factor out qemu_rbd_connect()'
020/36:[down] 'rbd: Remove non-schema options from runtime_opts' 021/36:[down] 'rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()' 022/36:[0034] [FC] 'rbd: Support .bdrv_co_create'
023/36:[down] 'rbd: Assing s->snap/image_name in qemu_rbd_open()' 024/36:[down] 'rbd: Use qemu_rbd_connect() in qemu_rbd_do_create()' 025/36:[0002] [FC] 'nfs: Use QAPI options in nfs_client_open()' 026/36:[----] [-C] 'nfs: Support .bdrv_co_create'
027/36:[0013] [FC] 'sheepdog: QAPIfy "redundacy" create option' 028/36:[0049] [FC] 'sheepdog: Support .bdrv_co_create'
029/36:[----] [-C] 'ssh: Use QAPI BlockdevOptionsSsh object' 030/36:[----] [-C] 'ssh: QAPIfy host-key-check option'
031/36:[----] [--] 'ssh: Pass BlockdevOptionsSsh to connect_to_ssh()' 032/36:[----] [--] 'ssh: Support .bdrv_co_create'
033/36:[----] [--] 'file-posix: Fix no-op bdrv_truncate() with falloc preallocation' 034/36:[----] [--] 'block: Fail bdrv_truncate() with negative size' 035/36:[----] [--] 'qemu-iotests: Test qcow2 over file image creation with QMP' 036/36:[----] [--] 'qemu-iotests: Test ssh image creation over QMP'

Kevin Wolf (36):
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()
test-qemu-opts: Test qemu_opts_append()
test-qemu-opts: Test qemu_opts_to_qdict_filtered()
qdict: Introduce qdict_rename_keys()
qcow2: Use visitor for options in qcow2_create()
block: Make bdrv_is_whitelisted() public
block: x-blockdev-create QMP command
file-posix: Support .bdrv_co_create
file-win32: Support .bdrv_co_create
gluster: Support .bdrv_co_create
rbd: Fix use after free in qemu_rbd_set_keypairs() error path
rbd: Factor out qemu_rbd_connect()
rbd: Remove non-schema options from runtime_opts
rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()
rbd: Support .bdrv_co_create
rbd: Assing s->snap/image_name in qemu_rbd_open()
rbd: Use qemu_rbd_connect() in qemu_rbd_do_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             |  76 ++++++++
 block/file-posix.c         |  91 +++++++---
 block/file-win32.c         |  45 ++++-
 block/gluster.c            | 135 +++++++++-----
 block/nfs.c                | 236 +++++++++++-------------
 block/qcow2.c              | 384 +++++++++++++++++++++++++--------------
 block/rbd.c                | 380 +++++++++++++++++++++------------------
 block/sheepdog.c           | 318 +++++++++++++++++++++++----------
 block/ssh.c                | 298 +++++++++++++++++--------------
 qobject/qdict.c            |  34 ++++
 tests/check-qdict.c        | 113 ++++++++++++
 tests/test-qemu-opts.c     | 253 ++++++++++++++++++++++++++
 util/qemu-option.c         |  42 ++++-
 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 +
 27 files changed, 3030 insertions(+), 764 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 v2 1/36 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 BlockdevCreateNotSupported for all drivers.

---
 qapi/block-core.json | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
qapi/block-core.json +62 -0
index 5c5921bfb7..d256cefc79 100644
@@ -3359,6 +3359,68 @@
 { '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',
+      'nvme':           '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
author
#2 lore raw

PATCH v2 2/36 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 d256cefc79..74b864d64e 100644
@@ -3359,6 +3359,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.
@@ -3402,7 +3445,7 @@
       'null-co':        'BlockdevCreateNotSupported',
       'nvme':           'BlockdevCreateNotSupported',
       'parallels':      'BlockdevCreateNotSupported',
-      'qcow2':          'BlockdevCreateNotSupported',
+      'qcow2':          'BlockdevCreateOptionsQcow2',
       'qcow':           'BlockdevCreateNotSupported',
       'qed':            'BlockdevCreateNotSupported',
       'quorum':         'BlockdevCreateNotSupported',
Signature · 1 line
2.13.6
author
#3 lore raw

PATCH v2 3/36 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 288b5299d8..dc6cdea113 100644
@@ -2725,7 +2725,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,
@@ -2751,28 +2751,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 */
@@ -2827,7 +2810,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) {
@@ -2899,7 +2883,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) {
@@ -2929,6 +2914,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;
 
@@ -2997,12 +2983,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
author
#4 lore raw

PATCH v2 4/36 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 dc6cdea113..22194180c6 100644
@@ -2665,19 +2665,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;
@@ -2725,12 +2732,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;
 
     /*
@@ -2747,10 +2753,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) {
@@ -2777,7 +2865,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);
     }
@@ -2839,18 +2927,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;
         }
     }
@@ -2864,8 +2960,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;
@@ -2903,8 +2999,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;
@@ -2912,7 +3010,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;
@@ -2923,6 +3020,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)) {
@@ -2960,20 +3064,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);
@@ -2981,10 +3071,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);
@@ -3005,9 +3095,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
author
#5 lore raw

PATCH v2 5/36 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 947e8876cd..54fe8b7a0e 100644
@@ -245,6 +245,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 22194180c6..b34924b0f0 100644
@@ -2732,8 +2732,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;
@@ -2751,7 +2750,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;
@@ -2760,10 +2760,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;
@@ -2792,7 +2797,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) {
@@ -2803,11 +2809,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) {
@@ -2816,7 +2824,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) {
@@ -2827,13 +2836,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);
 
@@ -2991,9 +3002,8 @@ static int qcow2_create2(BlockDriverState *bs,
 
     ret = 0;
 out:
-    if (blk) {
-        blk_unref(blk);
-    }
+    blk_unref(blk);
+    bdrv_unref(bs);
     return ret;
 }
 
@@ -3121,7 +3131,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
author
#6 lore raw

PATCH v2 6/36 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 b34924b0f0..9a2028b3cf 100644
@@ -2414,13 +2414,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;
 
@@ -2443,10 +2440,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.",
@@ -2454,8 +2472,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);
@@ -2464,10 +2481,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;
 }
 
@@ -2732,8 +2748,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;
@@ -2963,8 +2978,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;
         }
@@ -3021,6 +3036,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;
@@ -3037,6 +3053,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)) {
@@ -3048,6 +3065,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);
@@ -3121,6 +3146,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,
@@ -3131,7 +3158,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;
     }
@@ -3139,6 +3166,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
author
#7 lore raw

PATCH v2 7/36 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 9a2028b3cf..64bf2863cd 100644
@@ -2872,6 +2872,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);
@@ -3108,15 +3127,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
author
#8 lore raw

PATCH v2 8/36 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    | 42 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 39 insertions(+), 5 deletions(-)
include/qemu/option.h +2 -0
index b127fb6db6..306fdb5f7a 100644
@@ -124,6 +124,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 +37 -5

63 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v2 8/36
#9 lore raw

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

Quoted 11 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    | 42 +++++++++++++++++++++++++++++++++++++-----
  2 files changed, 39 insertions(+), 5 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Reviewed-by on v2 8/36
#10 lore raw

Re: [Qemu-devel] [PATCH v2 08/36] 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    | 42 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 39 insertions(+), 5 deletions(-)
author
#11 lore raw

PATCH v2 9/36 test-qemu-opts: Test qemu_opts_append()

In reply to Kevin Wolf

Basic test for merging two QemuOptsLists.

---
 tests/test-qemu-opts.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)
tests/test-qemu-opts.c +128 -0
index 5d5a3daa7b..6c3183390b 100644
@@ -23,6 +23,8 @@ static QemuOptsList opts_list_01 = {
         {
             .name = "str1",
             .type = QEMU_OPT_STRING,
+            .help = "Help texts are preserved in qemu_opts_append",
+            .def_value_str = "default",
         },{
             .name = "str2",
             .type = QEMU_OPT_STRING,
@@ -32,6 +34,7 @@ static QemuOptsList opts_list_01 = {
         },{
             .name = "number1",
             .type = QEMU_OPT_NUMBER,
+            .help = "Having help texts only for some options is okay",
         },{
             .name = "number2",
             .type = QEMU_OPT_NUMBER,
@@ -743,6 +746,129 @@ static void test_opts_parse_size(void)
     qemu_opts_reset(&opts_list_02);
 }
 
+static void append_verify_list_01(QemuOptDesc *desc, bool with_overlapping)
+{
+    int i = 0;
+
+    if (with_overlapping) {
+        g_assert_cmpstr(desc[i].name, ==, "str1");
+        g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING);
+        g_assert_cmpstr(desc[i].help, ==,
+                        "Help texts are preserved in qemu_opts_append");
+        g_assert_cmpstr(desc[i].def_value_str, ==, "default");
+        i++;
+
+        g_assert_cmpstr(desc[i].name, ==, "str2");
+        g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING);
+        g_assert_cmpstr(desc[i].help, ==, NULL);
+        g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+        i++;
+    }
+
+    g_assert_cmpstr(desc[i].name, ==, "str3");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, "number1");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_NUMBER);
+    g_assert_cmpstr(desc[i].help, ==,
+                    "Having help texts only for some options is okay");
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, "number2");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_NUMBER);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, NULL);
+}
+
+static void append_verify_list_02(QemuOptDesc *desc)
+{
+    int i = 0;
+
+    g_assert_cmpstr(desc[i].name, ==, "str1");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, "str2");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, "bool1");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_BOOL);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, "bool2");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_BOOL);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, "size1");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_SIZE);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, "size2");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_SIZE);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+    i++;
+
+    g_assert_cmpstr(desc[i].name, ==, "size3");
+    g_assert_cmpint(desc[i].type, ==, QEMU_OPT_SIZE);
+    g_assert_cmpstr(desc[i].help, ==, NULL);
+    g_assert_cmpstr(desc[i].def_value_str, ==, NULL);
+}
+
+static void test_opts_append_to_null(void)
+{
+    QemuOptsList *merged;
+
+    merged = qemu_opts_append(NULL, &opts_list_01);
+    g_assert(merged != &opts_list_01);
+
+    g_assert_cmpstr(merged->name, ==, NULL);
+    g_assert_cmpstr(merged->implied_opt_name, ==, NULL);
+    g_assert_false(merged->merge_lists);
+
+    append_verify_list_01(merged->desc, true);
+
+    qemu_opts_free(merged);
+}
+
+static void test_opts_append(void)
+{
+    QemuOptsList *first, *merged;
+
+    first = qemu_opts_append(NULL, &opts_list_02);
+    merged = qemu_opts_append(first, &opts_list_01);
+    g_assert(first != &opts_list_02);
+    g_assert(merged != &opts_list_01);
+
+    g_assert_cmpstr(merged->name, ==, NULL);
+    g_assert_cmpstr(merged->implied_opt_name, ==, NULL);
+    g_assert_false(merged->merge_lists);
+
+    append_verify_list_02(&merged->desc[0]);
+    append_verify_list_01(&merged->desc[7], false);
+
+    qemu_opts_free(merged);
+}
+
+
 int main(int argc, char *argv[])
 {
     register_opts();
@@ -761,6 +887,8 @@ int main(int argc, char *argv[])
     g_test_add_func("/qemu-opts/opts_parse/bool", test_opts_parse_bool);
     g_test_add_func("/qemu-opts/opts_parse/number", test_opts_parse_number);
     g_test_add_func("/qemu-opts/opts_parse/size", test_opts_parse_size);
+    g_test_add_func("/qemu-opts/append_to_null", test_opts_append_to_null);
+    g_test_add_func("/qemu-opts/append", test_opts_append);
     g_test_run();
     return 0;
 }
Signature · 1 line
2.13.6
Reviewed-by on v2 9/36
#12 lore raw

Re: [Qemu-devel] [PATCH v2 09/36] test-qemu-opts: Test qemu_opts_append()

Quoted 7 lines · Kevin Wolf

Basic test for merging two QemuOptsLists.

---
  tests/test-qemu-opts.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 128 insertions(+)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#14 lore raw

PATCH v2 10/36 test-qemu-opts: Test qemu_opts_to_qdict_filtered()

In reply to Kevin Wolf
---
 tests/test-qemu-opts.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)
tests/test-qemu-opts.c +125 -0
index 6c3183390b..2c422abcd4 100644
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "qemu/option.h"
+#include "qemu/option_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
@@ -868,6 +869,127 @@ static void test_opts_append(void)
     qemu_opts_free(merged);
 }
 
+static void test_opts_to_qdict_basic(void)
+{
+    QemuOpts *opts;
+    QDict *dict;
+
+    opts = qemu_opts_parse(&opts_list_01, "str1=foo,str2=,str3=bar,number1=42",
+                           false, &error_abort);
+    g_assert(opts != NULL);
+
+    dict = qemu_opts_to_qdict(opts, NULL);
+    g_assert(dict != NULL);
+
+    g_assert_cmpstr(qdict_get_str(dict, "str1"), ==, "foo");
+    g_assert_cmpstr(qdict_get_str(dict, "str2"), ==, "");
+    g_assert_cmpstr(qdict_get_str(dict, "str3"), ==, "bar");
+    g_assert_cmpstr(qdict_get_str(dict, "number1"), ==, "42");
+    g_assert_false(qdict_haskey(dict, "number2"));
+
+    QDECREF(dict);
+    qemu_opts_del(opts);
+}
+
+static void test_opts_to_qdict_filtered(void)
+{
+    QemuOptsList *first, *merged;
+    QemuOpts *opts;
+    QDict *dict;
+
+    first = qemu_opts_append(NULL, &opts_list_02);
+    merged = qemu_opts_append(first, &opts_list_01);
+
+    opts = qemu_opts_parse(merged,
+                           "str1=foo,str2=,str3=bar,bool1=off,number1=42",
+                           false, &error_abort);
+    g_assert(opts != NULL);
+
+    /* Convert to QDict without deleting from opts */
+    dict = qemu_opts_to_qdict_filtered(opts, NULL, &opts_list_01, false);
+    g_assert(dict != NULL);
+    g_assert_cmpstr(qdict_get_str(dict, "str1"), ==, "foo");
+    g_assert_cmpstr(qdict_get_str(dict, "str2"), ==, "");
+    g_assert_cmpstr(qdict_get_str(dict, "str3"), ==, "bar");
+    g_assert_cmpstr(qdict_get_str(dict, "number1"), ==, "42");
+    g_assert_false(qdict_haskey(dict, "number2"));
+    g_assert_false(qdict_haskey(dict, "bool1"));
+    QDECREF(dict);
+
+    dict = qemu_opts_to_qdict_filtered(opts, NULL, &opts_list_02, false);
+    g_assert(dict != NULL);
+    g_assert_cmpstr(qdict_get_str(dict, "str1"), ==, "foo");
+    g_assert_cmpstr(qdict_get_str(dict, "str2"), ==, "");
+    g_assert_cmpstr(qdict_get_str(dict, "bool1"), ==, "off");
+    g_assert_false(qdict_haskey(dict, "str3"));
+    g_assert_false(qdict_haskey(dict, "number1"));
+    g_assert_false(qdict_haskey(dict, "number2"));
+    QDECREF(dict);
+
+    /* Now delete converted options from opts */
+    dict = qemu_opts_to_qdict_filtered(opts, NULL, &opts_list_01, true);
+    g_assert(dict != NULL);
+    g_assert_cmpstr(qdict_get_str(dict, "str1"), ==, "foo");
+    g_assert_cmpstr(qdict_get_str(dict, "str2"), ==, "");
+    g_assert_cmpstr(qdict_get_str(dict, "str3"), ==, "bar");
+    g_assert_cmpstr(qdict_get_str(dict, "number1"), ==, "42");
+    g_assert_false(qdict_haskey(dict, "number2"));
+    g_assert_false(qdict_haskey(dict, "bool1"));
+    QDECREF(dict);
+
+    dict = qemu_opts_to_qdict_filtered(opts, NULL, &opts_list_02, true);
+    g_assert(dict != NULL);
+    g_assert_cmpstr(qdict_get_str(dict, "bool1"), ==, "off");
+    g_assert_false(qdict_haskey(dict, "str1"));
+    g_assert_false(qdict_haskey(dict, "str2"));
+    g_assert_false(qdict_haskey(dict, "str3"));
+    g_assert_false(qdict_haskey(dict, "number1"));
+    g_assert_false(qdict_haskey(dict, "number2"));
+    QDECREF(dict);
+
+    g_assert_true(QTAILQ_EMPTY(&opts->head));
+
+    qemu_opts_del(opts);
+    qemu_opts_free(merged);
+}
+
+static void test_opts_to_qdict_duplicates(void)
+{
+    QemuOpts *opts;
+    QemuOpt *opt;
+    QDict *dict;
+
+    opts = qemu_opts_parse(&opts_list_03, "foo=a,foo=b", false, &error_abort);
+    g_assert(opts != NULL);
+
+    /* Verify that opts has two options with the same name */
+    opt = QTAILQ_FIRST(&opts->head);
+    g_assert_cmpstr(opt->name, ==, "foo");
+    g_assert_cmpstr(opt->str , ==, "a");
+
+    opt = QTAILQ_NEXT(opt, next);
+    g_assert_cmpstr(opt->name, ==, "foo");
+    g_assert_cmpstr(opt->str , ==, "b");
+
+    opt = QTAILQ_NEXT(opt, next);
+    g_assert(opt == NULL);
+
+    /* In the conversion to QDict, the last one wins */
+    dict = qemu_opts_to_qdict(opts, NULL);
+    g_assert(dict != NULL);
+    g_assert_cmpstr(qdict_get_str(dict, "foo"), ==, "b");
+    QDECREF(dict);
+
+    /* The last one still wins if entries are deleted, and both are deleted */
+    dict = qemu_opts_to_qdict_filtered(opts, NULL, NULL, true);
+    g_assert(dict != NULL);
+    g_assert_cmpstr(qdict_get_str(dict, "foo"), ==, "b");
+    QDECREF(dict);
+
+    g_assert_true(QTAILQ_EMPTY(&opts->head));
+
+    qemu_opts_del(opts);
+}
 
 int main(int argc, char *argv[])
 {
@@ -889,6 +1011,9 @@ int main(int argc, char *argv[])
     g_test_add_func("/qemu-opts/opts_parse/size", test_opts_parse_size);
     g_test_add_func("/qemu-opts/append_to_null", test_opts_append_to_null);
     g_test_add_func("/qemu-opts/append", test_opts_append);
+    g_test_add_func("/qemu-opts/to_qdict/basic", test_opts_to_qdict_basic);
+    g_test_add_func("/qemu-opts/to_qdict/filtered", test_opts_to_qdict_filtered);
+    g_test_add_func("/qemu-opts/to_qdict/duplicates", test_opts_to_qdict_duplicates);
     g_test_run();
     return 0;
 }
Signature · 1 line
2.13.6
Reviewed-by on v2 10/36
#15 lore raw

Re: [Qemu-devel] [PATCH v2 10/36] test-qemu-opts: Test qemu_opts_to_qdict_filtered()

Quoted 18 lines · Kevin Wolf
---
  tests/test-qemu-opts.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 125 insertions(+)
tests/test-qemu-opts.c +1 -0
index 6c3183390b..2c422abcd4 100644
@@ -10,6 +10,7 @@
  #include "qemu/osdep.h"
  #include "qemu/cutils.h"
  #include "qemu/option.h"
+#include "qemu/option_int.h"
  #include "qapi/error.h"
  #include "qapi/qmp/qdict.h"
  #include "qapi/qmp/qstring.h"

tests/test-qemu-opts.c static void test_opts_append(void) · line 878

Quoted 10 lines
tests/test-qemu-opts.c +7 -0
@@ -868,6 +869,127 @@ static void test_opts_append(void)
      qemu_opts_free(merged);
  }
 
+static void test_opts_to_qdict_basic(void)
+{
+    QemuOpts *opts;
+    QDict *dict;
+
+    opts = qemu_opts_parse(&opts_list_01, "str1=foo,str2=,str3=bar,number1=42",
+                           false, &error_abort);

Worth any additional craziness in regards to our QemuOpts parsing, like str1=foo,,bar,str2... for an option containing commas, or str2=,str1=foo, for supplying options in a different order than the list? But what you have is a good addition even if you don't tweak it.

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

Re: [Qemu-devel] [PATCH v2 10/36] test-qemu-opts: Test qemu_opts_to_qdict_filtered()

Am 21.02.2018 um 21:57 hat Eric Blake geschrieben:

Quoted 25 lines · Kevin Wolf
Quoted 24 lines
---
  tests/test-qemu-opts.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 125 insertions(+)
tests/test-qemu-opts.c +5 -0
index 6c3183390b..2c422abcd4 100644
@@ -10,6 +10,7 @@
  #include "qemu/osdep.h"
  #include "qemu/cutils.h"
  #include "qemu/option.h"
+#include "qemu/option_int.h"
  #include "qapi/error.h"
  #include "qapi/qmp/qdict.h"
  #include "qapi/qmp/qstring.h"
@@ -868,6 +869,127 @@ static void test_opts_append(void)
      qemu_opts_free(merged);
  }
+static void test_opts_to_qdict_basic(void)
+{
+    QemuOpts *opts;
+    QDict *dict;
Quoted 8 lines

tests/test-qemu-opts.c static void test_opts_append(void) · line 878

Quoted 3 lines
tests/test-qemu-opts.c +3 -0
+
+    opts = qemu_opts_parse(&opts_list_01, "str1=foo,str2=,str3=bar,number1=42",
+                           false, &error_abort);

Worth any additional craziness in regards to our QemuOpts parsing, like str1=foo,,bar,str2... for an option containing commas, or str2=,str1=foo, for supplying options in a different order than the list? But what you have is a good addition even if you don't tweak it.

This is not a test for parsing options string, but for converting an already existing QemuOpts to a QDict. Parsing is already extensively tested in /qemu-opts/opts_parse/*. I'm only using qemu_opts_parse() here because it's the most convenient way to create a QemuOpts with multiple options.

So the only things we need to consider in this test case are different QemuOpts that result from the parsing. Escaped commas don't exist in this representation any more and the associated QemuOptsList isn't involved in the conversion to QDicts, so these wouldn't actually be new cases for the thing we're testing here.

Kevin

#17 lore raw

Re: [Qemu-devel] [PATCH v2 10/36] test-qemu-opts: Test qemu_opts_to_qdict_filtered()

Quoted 14 lines · Kevin Wolf
Quoted 7 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+    opts = qemu_opts_parse(&opts_list_01, "str1=foo,str2=,str3=bar,number1=42",
+                           false, &error_abort);

Worth any additional craziness in regards to our QemuOpts parsing, like str1=foo,,bar,str2... for an option containing commas, or str2=,str1=foo, for supplying options in a different order than the list? But what you have is a good addition even if you don't tweak it.

This is not a test for parsing options string, but for converting an already existing QemuOpts to a QDict. Parsing is already extensively tested in /qemu-opts/opts_parse/*. I'm only using qemu_opts_parse() here because it's the most convenient way to create a QemuOpts with multiple options.

Quoted 5 lines

So the only things we need to consider in this test case are different QemuOpts that result from the parsing. Escaped commas don't exist in this representation any more and the associated QemuOptsList isn't involved in the conversion to QDicts, so these wouldn't actually be new cases for the thing we're testing here.

Alright, you've convinced me that this isn't the test to be hammering on QemuOpts.

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

PATCH v2 11/36 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          |  34 ++++++++++++++
 tests/check-qdict.c      | 113 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)
include/qapi/qmp/qdict.h +6 -0
index ff6f7842c3..7c6d844549 100644
@@ -81,4 +81,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 +34 -0
index 23df84f9cd..229b8c840b 100644
@@ -1072,3 +1072,37 @@ 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.
+ *
+ * The renames are performed individually in the order of the array, so entries
+ * may be renamed multiple times and may or may not conflict depending on the
+ * order of the renames array.
+ *
+ * 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;
+}
tests/check-qdict.c +113 -0

125 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v2 11/36
#20 lore raw

Re: [Qemu-devel] [PATCH v2 11/36] qdict: Introduce qdict_rename_keys()

Quoted 9 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          |  34 ++++++++++++++
 tests/check-qdict.c      | 113 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)

[...]

Quoted 5 lines
tests/check-qdict.c +0 -0
index ec628f3453..5f8f3be9ff 100644
@@ -665,6 +665,117 @@ static void qdict_crumple_test_empty(void)

[...]

tests/check-qdict.c static void qdict_crumple_test_empty(void) · line 731

Quoted 10 lines
tests/check-qdict.c +10 -0
+    /* Renames are processed top to bottom */
+    renames = (QDictRenames[]) {
+        { "abc",        "tmp" },
+        { "abcdef",     "abc" },
+        { "number",     "abcdef" },
+        { "flag",       "number" },
+        { "nothing",    "flag" },
+        { "tmp",        "nothing" },
+        { NULL , NULL }
+    };

A bit confusing to follow, but I guess nobody will have to follow it after me and Eric.

tests/check-qdict.c static void qdict_crumple_test_empty(void) · line 744

Quoted 13 lines
tests/check-qdict.c +13 -0
+    copy = qdict_clone_shallow(dict);
+    qdict_rename_keys(copy, renames, &error_abort);
+
+    g_assert_cmpstr(qdict_get_str(copy, "nothing"), ==, "foo");
+    g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "bar");
+    g_assert_cmpint(qdict_get_int(copy, "abcdef"), ==, 42);
+    g_assert_cmpint(qdict_get_bool(copy, "number"), ==, true);
+    g_assert(qobject_type(qdict_get(copy, "flag")) == QTYPE_QNULL);
+    g_assert(!qdict_haskey(copy, "tmp"));
+
+    QDECREF(copy);
+
+    /* Conflicting renam */

*rename

With that fixed:

Reviewed-by on v2 11/36
#21 lore raw

Re: [Qemu-devel] [PATCH v2 11/36] qdict: Introduce qdict_rename_keys()

In reply to Kevin Wolf
Quoted 11 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          |  34 ++++++++++++++
  tests/check-qdict.c      | 113 +++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 153 insertions(+)
Quoted 8 lines

+/**
+ * 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.
+ *
+ * The renames are performed individually in the order of the array, so entries
+ * may be renamed multiple times and may or may not conflict depending on the
+ * order of the renames array.

Oh interesting - so I could rename a->tmp, b->a, tmp->b in the classic strategy to intentionally avoid conflicts. But I hope none of our actual clients ever abuse the interface that directly.

Quoted 2 lines

+ *
+ * Returns true for success, false in error cases.

I won't make you change it, but is 0/-1 any easier to understand intuitively? With bool, it's often a case of "I'd better check the docs for whether true meant the sense I wanted"

Quoted 4 lines

+ */

tests/check-qdict.c +2 -0
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp)
+{
 

tests/check-qdict.c static void qdict_crumple_test_empty(void) · line 692

Quoted 10 lines
tests/check-qdict.c +9 -0
+++ b/tests/check-qdict.c
 
+    copy = qdict_clone_shallow(dict);
+    qdict_rename_keys(copy, renames, &error_abort);
+
+    g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo");
+    g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar");
+    g_assert_cmpint(qdict_get_int(copy, "number"), ==, 42);
+    g_assert_cmpint(qdict_get_bool(copy, "flag"), ==, true);
+    g_assert(qobject_type(qdict_get(copy, "nothing")) == QTYPE_QNULL);

Also worth an assert that there are exactly 5 keys, so that rename didn't botch something to leave a different straggler key behind?

Quoted 10 lines
tests/check-qdict.c +10 -0
+
+    QDECREF(copy);
+
+    /* Simple rename of all entries */
+    renames = (QDictRenames[]) {
+        { "abc",        "str1" },
+        { "abcdef",     "str2" },
+        { "number",     "int" },
+        { "flag",       "bool" },
+        { "nothing",    "null" },

tests/check-qdict.c static void qdict_crumple_test_empty(void) · line 712

Quoted 10 lines
tests/check-qdict.c +10 -0
+        { NULL , NULL }
+    };
+    copy = qdict_clone_shallow(dict);
+    qdict_rename_keys(copy, renames, &error_abort);
+
+    g_assert(!qdict_haskey(copy, "abc"));
+    g_assert(!qdict_haskey(copy, "abcdef"));
+    g_assert(!qdict_haskey(copy, "number"));
+    g_assert(!qdict_haskey(copy, "flag"));
+    g_assert(!qdict_haskey(copy, "nothing"));

Direct check for the obvious keys that should have been renamed,

tests/check-qdict.c static void qdict_crumple_test_empty(void) · line 718

Quoted 6 lines
tests/check-qdict.c +6 -0
+
+    g_assert_cmpstr(qdict_get_str(copy, "str1"), ==, "foo");
+    g_assert_cmpstr(qdict_get_str(copy, "str2"), ==, "bar");
+    g_assert_cmpint(qdict_get_int(copy, "int"), ==, 42);
+    g_assert_cmpint(qdict_get_bool(copy, "bool"), ==, true);
+    g_assert(qobject_type(qdict_get(copy, "null")) == QTYPE_QNULL);

but again, an assert that there are 5 keys rules out all other mistakes, too.

Up to you whether to further tweak the tests; and with the spelling fix Max already found,

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

PATCH v2 12/36 qcow2: Use visitor for options in qcow2_create()

In reply to Kevin Wolf

Instead of manually creating the BlockdevCreateOptions object, use a visitor to parse the given options into the QAPI object.

This involves translation from the old command line syntax to the syntax mandated by the QAPI schema. Option names are still checked against qcow2_create_opts, so only the old option names are allowed on the command line, even if they are translated in qcow2_create().

In contrast, new option values are optionally recognised besides the old
values: 'compat' accepts 'v2'/'v3' as an alias for '0.10'/'1.1', and 'encrypt.format' accepts 'qcow' as an alias for 'aes' now.

---
 block/qcow2.c              | 217 ++++++++++++++++-----------------------------
 tests/qemu-iotests/049.out |   8 +-
 tests/qemu-iotests/112.out |   4 +-
 3 files changed, 83 insertions(+), 146 deletions(-)
block/qcow2.c +77 -140
index 64bf2863cd..58737d0833 100644
@@ -38,7 +38,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"
 
@@ -2414,37 +2414,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)
@@ -2838,7 +2807,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;
     }
@@ -2856,7 +2825,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;
@@ -3043,144 +3012,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;
 
-    /* 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
author
#23 lore raw

PATCH v2 13/36 block: Make bdrv_is_whitelisted() public

In reply to Kevin Wolf

We'll use a separate source file for image creation, and we need to check there whether the requested driver is whitelisted.

---
 include/block/block.h | 1 +
 block.c               | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
include/block/block.h +1 -0
index 54fe8b7a0e..cfce88cbda 100644
@@ -225,6 +225,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);
block.c +1 -1
index c0e343d278..4a7e448226 100644
@@ -372,7 +372,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
Signature · 1 line
2.13.6
Reviewed-by on v2 13/36
#24 lore raw

Re: [Qemu-devel] [PATCH v2 13/36] block: Make bdrv_is_whitelisted() public

Quoted 8 lines · Kevin Wolf

We'll use a separate source file for image creation, and we need to check there whether the requested driver is whitelisted.

---
 include/block/block.h | 1 +
 block.c               | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
Reviewed-by on v2 13/36
#25 lore raw

Re: [Qemu-devel] [PATCH v2 13/36] block: Make bdrv_is_whitelisted() public

In reply to Kevin Wolf
Quoted 9 lines · Kevin Wolf

We'll use a separate source file for image creation, and we need to check there whether the requested driver is whitelisted.

---
  include/block/block.h | 1 +
  block.c               | 2 +-
  2 files changed, 2 insertions(+), 1 deletion(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#26 lore raw

PATCH v2 14/36 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_int.h |  2 ++
 block/create.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |  3 +-
 block/Makefile.objs       |  2 +-
 5 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 block/create.c
qapi/block-core.json +12 -0
index 74b864d64e..359195a1a3 100644
@@ -3464,6 +3464,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_int.h +2 -0
index 5ae7738cf8..0b43fae782 100644
@@ -128,6 +128,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/create.c new file +76 -0

76 lines not on this page · raw message

block/qcow2.c +2 -1
index 58737d0833..8acb36b0af 100644
@@ -4463,7 +4463,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_block_status = qcow2_co_block_status,
 
block/Makefile.objs +1 -1
index aede94f105..d644bac60a 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-$(CONFIG_LINUX) += nvme.o
 
Signature · 1 line
2.13.6
Reviewed-by on v2 14/36
#27 lore raw

Re: [Qemu-devel] [PATCH v2 14/36] block: x-blockdev-create QMP command

Quoted 10 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.

Quoted 8 lines
---
 qapi/block-core.json      | 12 ++++++++
 include/block/block_int.h |  2 ++
 block/create.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |  3 +-
 block/Makefile.objs       |  2 +-
 5 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 block/create.c
Reviewed-by on v2 14/36
#28 lore raw

Re: [Qemu-devel] [PATCH v2 14/36] block: x-blockdev-create QMP command

In reply to Kevin Wolf
Quoted 10 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.

Quoted 9 lines
---
  qapi/block-core.json      | 12 ++++++++
  include/block/block_int.h |  2 ++
  block/create.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++
  block/qcow2.c             |  3 +-
  block/Makefile.objs       |  2 +-
  5 files changed, 93 insertions(+), 2 deletions(-)

create mode 100644 block/create.c

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 v2 15/36 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 359195a1a3..0040795603 100644
@@ -3359,6 +3359,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)
@@ -3429,7 +3447,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 v2 15/36
#30 lore raw

Re: [Qemu-devel] [PATCH v2 15/36] file-posix: Support .bdrv_co_create

Quoted 15 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(-)
qapi/block-core.json +0 -0
index 359195a1a3..0040795603 100644

qapi/block-core.json line 3366

Quoted 8 lines
qapi/block-core.json +5 -0
@@ -3359,6 +3359,24 @@
  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
  ##
+# @BlockdevCreateOptionsFile:
+#
+# Driver specific image creation options for file.
+#
+# @filename         Filename for the new image file

Does this allow /dev/fdset magic filenames, for when libvirt has to pre-create a file and assign correct SELinux permissions, then hand the fd over the monitor, but where qemu then takes over the rest of the creation task? What tasks remain in file-posix creation, you ask?...

qapi/block-core.json line 3368

Quoted 2 lines
qapi/block-core.json +2 -0
+# @size             Size of the virtual disk in bytes
+# @preallocation    Preallocation mode for the new image (default: off)

...why, of course, a non-default preallocation. It would be nice to hand a 0-byte fd to qemu, then let qemu uniformly truncate it to its desired size, using whatever preallocation strategy is supported, rather than having to have libvirt worry about preallocation in addition to SELinux labeling. Especially once we get an async command variant working, where we can track progress, given that some forms of preallocation take a while. And we may someday still reach the policy decision where we can block qemu from directly calling open().

qapi/block-core.json line 3378

Quoted 10 lines
qapi/block-core.json +10 -0
+# @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' } }
+

I think I asked earlier why size is mandatory at this layer, but I'm still okay with that.

Hmm, since "creating" a file can be a destructive operation (if size requires a downwards truncation, or even if we intentionally wipe the first sector so that any prior bits that resemble a different format are no longer visible, or if preallocation explicitly wipes the entire image...), do we want to have any safeguards in place so that creation is attempted only on a newly-opened BDS, or with a force flag if size does not match the current size, or so on? That's more a question for the x-blockdev-create command though, so it doesn't stop review of this patch.

qapi/block-core.json line 3381

Quoted 5 lines
qapi/block-core.json +2 -1
-    fd = qemu_open(filename, O_RDWR | O_CREAT | O_TRUNC | O_BINARY,
+    /* Create file */
+    fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_TRUNC | O_BINARY,
                     0644);

At any rate, qemu_open() means that we be supporting /dev/fdset magic on a passed-in fd.

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

PATCH v2 16/36 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 f24c7bb92c..d572cde357 100644
@@ -553,29 +553,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 v2 16/36
#32 lore raw

Re: [Qemu-devel] [PATCH v2 16/36] file-win32: Support .bdrv_co_create

Quoted 10 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(-)

block/file-win32.c static int64_t raw_get_allocated_file_size(BlockDriverState *bs) · line 603

Quoted 8 lines
block/file-win32.c +8 -0
+
+    options = (BlockdevCreateOptions) {
+        .driver     = BLOCKDEV_DRIVER_FILE,
+        .u.file     = {
+            .filename           = (char *) filename,
+            .size               = total_size,
+            .has_preallocation  = false,
+            .has_nocow          = false,

Technically, these last two lines aren't needed (you get false by default), but it doesn't hurt to list them either.

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

PATCH v2 17/36 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      | 135 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 108 insertions(+), 45 deletions(-)
qapi/block-core.json +17 -1
index 0040795603..74021c51d7 100644
@@ -3377,6 +3377,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)
@@ -3450,7 +3466,7 @@
       'file':           'BlockdevCreateOptionsFile',
       'ftp':            'BlockdevCreateNotSupported',
       'ftps':           'BlockdevCreateNotSupported',
-      'gluster':        'BlockdevCreateNotSupported',
+      'gluster':        'BlockdevCreateOptionsGluster',
       'host_cdrom':     'BlockdevCreateNotSupported',
       'host_device':    'BlockdevCreateNotSupported',
       'http':           'BlockdevCreateNotSupported',
block/gluster.c +91 -44

203 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v2 17/36
#34 lore raw

Re: [Qemu-devel] [PATCH v2 17/36] 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      | 135 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 108 insertions(+), 45 deletions(-)
Reviewed-by on v2 17/36
#35 lore raw

Re: [Qemu-devel] [PATCH v2 17/36] gluster: Support .bdrv_co_create

In reply to Kevin Wolf
Quoted 9 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      | 135 ++++++++++++++++++++++++++++++++++-----------------
  2 files changed, 108 insertions(+), 45 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#36 lore raw

PATCH v2 18/36 rbd: Fix use after free in qemu_rbd_set_keypairs() error path

In reply to Kevin Wolf

If we want to include the invalid option name in the error message, we can't free the string earlier than that.

---
 block/rbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
block/rbd.c +2 -1
index 8474b0ba11..27fa11b473 100644
@@ -268,13 +268,14 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
         key = qstring_get_str(name);
 
         ret = rados_conf_set(cluster, key, qstring_get_str(value));
-        QDECREF(name);
         QDECREF(value);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "invalid conf option %s", key);
+            QDECREF(name);
             ret = -EINVAL;
             break;
         }
+        QDECREF(name);
     }
 
     QDECREF(keypairs);
Signature · 1 line
2.13.6
Reviewed-by on v2 18/36
#37 lore raw

Re: [Qemu-devel] [PATCH v2 18/36] rbd: Fix use after free in qemu_rbd_set_keypairs() error path

Quoted 7 lines · Kevin Wolf

If we want to include the invalid option name in the error message, we can't free the string earlier than that.

---
 block/rbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by on v2 18/36
#38 lore raw

Re: [Qemu-devel] [PATCH v2 18/36] rbd: Fix use after free in qemu_rbd_set_keypairs() error path

In reply to Kevin Wolf
Quoted 7 lines · Kevin Wolf

If we want to include the invalid option name in the error message, we can't free the string earlier than that.

---
  block/rbd.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

D'oh. Should this one be cc'd to qemu-stable?

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

Re: [Qemu-devel] [PATCH v2 18/36] rbd: Fix use after free in qemu_rbd_set_keypairs() error path

Am 23.02.2018 um 16:15 hat Eric Blake geschrieben:

Quoted 10 lines · Kevin Wolf
Quoted 7 lines

If we want to include the invalid option name in the error message, we can't free the string earlier than that.

---
  block/rbd.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

D'oh. Should this one be cc'd to qemu-stable?

Yes, good point. Adding the CC to this reply, and also adding a Cc: line in the commit message.

Kevin

author
#40 lore raw

PATCH v2 19/36 rbd: Factor out qemu_rbd_connect()

In reply to Kevin Wolf

The code to establish an RBD connection is duplicated between open and create. In order to be able to share the code, factor out the code from qemu_rbd_open() as a first step.

---
 block/rbd.c | 100 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 60 insertions(+), 40 deletions(-)
block/rbd.c +60 -40
index 27fa11b473..4bbcce4eca 100644
@@ -544,32 +544,17 @@ out:
     return rados_str;
 }
 
-static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
-                         Error **errp)
+static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
+                            char **s_snap, char **s_image_name,
+                            QDict *options, bool cache, Error **errp)
 {
-    BDRVRBDState *s = bs->opaque;
-    const char *pool, *snap, *conf, *user, *image_name, *keypairs;
-    const char *secretid, *filename;
     QemuOpts *opts;
-    Error *local_err = NULL;
     char *mon_host = NULL;
+    const char *pool, *snap, *conf, *user, *image_name, *keypairs;
+    const char *secretid;
+    Error *local_err = NULL;
     int r;
 
-    /* If we are given a filename, parse the filename, with precedence given to
-     * filename encoded options */
-    filename = qdict_get_try_str(options, "filename");
-    if (filename) {
-        warn_report("'filename' option specified. "
-                    "This is an unsupported option, and may be deprecated "
-                    "in the future");
-        qemu_rbd_parse_filename(filename, options, &local_err);
-        if (local_err) {
-            r = -EINVAL;
-            error_propagate(errp, local_err);
-            goto exit;
-        }
-    }
-
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
@@ -600,35 +585,35 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_opts;
     }
 
-    r = rados_create(&s->cluster, user);
+    r = rados_create(cluster, user);
     if (r < 0) {
         error_setg_errno(errp, -r, "error initializing");
         goto failed_opts;
     }
 
-    s->snap = g_strdup(snap);
-    s->image_name = g_strdup(image_name);
+    *s_snap = g_strdup(snap);
+    *s_image_name = g_strdup(image_name);
 
     /* try default location when conf=NULL, but ignore failure */
-    r = rados_conf_read_file(s->cluster, conf);
+    r = rados_conf_read_file(*cluster, conf);
     if (conf && r < 0) {
         error_setg_errno(errp, -r, "error reading conf file %s", conf);
         goto failed_shutdown;
     }
 
-    r = qemu_rbd_set_keypairs(s->cluster, keypairs, errp);
+    r = qemu_rbd_set_keypairs(*cluster, keypairs, errp);
     if (r < 0) {
         goto failed_shutdown;
     }
 
     if (mon_host) {
-        r = rados_conf_set(s->cluster, "mon_host", mon_host);
+        r = rados_conf_set(*cluster, "mon_host", mon_host);
         if (r < 0) {
             goto failed_shutdown;
         }
     }
 
-    if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
+    if (qemu_rbd_set_auth(*cluster, secretid, errp) < 0) {
         r = -EIO;
         goto failed_shutdown;
     }
@@ -640,24 +625,65 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
      * librbd defaults to no caching. If write through caching cannot
      * be set up, fall back to no caching.
      */
-    if (flags & BDRV_O_NOCACHE) {
-        rados_conf_set(s->cluster, "rbd_cache", "false");
+    if (cache) {
+        rados_conf_set(*cluster, "rbd_cache", "true");
     } else {
-        rados_conf_set(s->cluster, "rbd_cache", "true");
+        rados_conf_set(*cluster, "rbd_cache", "false");
     }
 
-    r = rados_connect(s->cluster);
+    r = rados_connect(*cluster);
     if (r < 0) {
         error_setg_errno(errp, -r, "error connecting");
         goto failed_shutdown;
     }
 
-    r = rados_ioctx_create(s->cluster, pool, &s->io_ctx);
+    r = rados_ioctx_create(*cluster, pool, io_ctx);
     if (r < 0) {
         error_setg_errno(errp, -r, "error opening pool %s", pool);
         goto failed_shutdown;
     }
 
+    qemu_opts_del(opts);
+    return 0;
+
+failed_shutdown:
+    rados_shutdown(*cluster);
+    g_free(*s_snap);
+    g_free(*s_image_name);
+failed_opts:
+    qemu_opts_del(opts);
+    g_free(mon_host);
+    return r;
+}
+
+static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
+                         Error **errp)
+{
+    BDRVRBDState *s = bs->opaque;
+    Error *local_err = NULL;
+    const char *filename;
+    int r;
+
+    /* If we are given a filename, parse the filename, with precedence given to
+     * filename encoded options */
+    filename = qdict_get_try_str(options, "filename");
+    if (filename) {
+        warn_report("'filename' option specified. "
+                    "This is an unsupported option, and may be deprecated "
+                    "in the future");
+        qemu_rbd_parse_filename(filename, options, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return -EINVAL;
+        }
+    }
+
+    r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name,
+                         options, !(flags & BDRV_O_NOCACHE), errp);
+    if (r < 0) {
+        return r;
+    }
+
     /* rbd_open is always r/w */
     r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
     if (r < 0) {
@@ -682,19 +708,13 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    qemu_opts_del(opts);
     return 0;
 
 failed_open:
     rados_ioctx_destroy(s->io_ctx);
-failed_shutdown:
-    rados_shutdown(s->cluster);
     g_free(s->snap);
     g_free(s->image_name);
-failed_opts:
-    qemu_opts_del(opts);
-    g_free(mon_host);
-exit:
+    rados_shutdown(s->cluster);
     return r;
 }
 
Signature · 1 line
2.13.6
Reviewed-by on v2 19/36
#41 lore raw

Re: [Qemu-devel] [PATCH v2 19/36] rbd: Factor out qemu_rbd_connect()

Quoted 14 lines · Kevin Wolf

The code to establish an RBD connection is duplicated between open and create. In order to be able to share the code, factor out the code from qemu_rbd_open() as a first step.

---
 block/rbd.c | 100 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 60 insertions(+), 40 deletions(-)
block/rbd.c +0 -0
index 27fa11b473..4bbcce4eca 100644

block/rbd.c out: · line 549

Quoted 8 lines
block/rbd.c +3 -2
@@ -544,32 +544,17 @@ out:
     return rados_str;
 }
 
-static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
-                         Error **errp)
+static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
+                            char **s_snap, char **s_image_name,
+                            QDict *options, bool cache, Error **errp)

Bikeshedding ahead: Maybe this should be called qemu_rados_connect()? I don't know anything about this, but there seems to be a distinction between rados_* functions and rbd_* functions -- the former work on the pool, the latter on the single block device.

Since this function only connects to the pool and not to a single device within, I think it should be called qemu_rados_connect() instead of qemu_rbd_connect().

(Also because qemu_rbd_connect() seems so similar to qemu_rbd_open().)

Up to you:

author
#42 lore raw

Re: [Qemu-devel] [PATCH v2 19/36] rbd: Factor out qemu_rbd_connect()

Am 23.02.2018 um 00:10 hat Max Reitz geschrieben:

Quoted 29 lines · Kevin Wolf
Quoted 14 lines

The code to establish an RBD connection is duplicated between open and create. In order to be able to share the code, factor out the code from qemu_rbd_open() as a first step.

---
 block/rbd.c | 100 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 60 insertions(+), 40 deletions(-)
block/rbd.c +0 -0
index 27fa11b473..4bbcce4eca 100644

block/rbd.c out: · line 549

Quoted 8 lines
block/rbd.c +3 -2
@@ -544,32 +544,17 @@ out:
     return rados_str;
 }
 
-static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
-                         Error **errp)
+static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
+                            char **s_snap, char **s_image_name,
+                            QDict *options, bool cache, Error **errp)

Bikeshedding ahead: Maybe this should be called qemu_rados_connect()? I don't know anything about this, but there seems to be a distinction between rados_* functions and rbd_* functions -- the former work on the pool, the latter on the single block device.

Quoted 5 lines

Since this function only connects to the pool and not to a single device within, I think it should be called qemu_rados_connect() instead of qemu_rbd_connect().

(Also because qemu_rbd_connect() seems so similar to qemu_rbd_open().)

I think librados is the lower level interface, and librbd builds a higher level interface on top of it. But I don't know anything about the details either.

However, for functions in the block driver, qemu_rbd_* is the only prefix used, there is no qemu_rados_* function. So I assume the prefix comes from the block driver name 'rbd' rather than which library it accesses, and that it would be better to keep qemu_rbd_connect().

Quoted 3 lines

Up to you:

Thanks.

Kevin

author
#43 lore raw

PATCH v2 20/36 rbd: Remove non-schema options from runtime_opts

In reply to Kevin Wolf

Instead of the QemuOpts in qemu_rbd_connect(), we want to use QAPI objects. As a preparation, fetch those options directly from the QDict that .bdrv_open() supports in the rbd driver and that are not in the schema.

---
 block/rbd.c | 55 ++++++++++++++++++++++++-------------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)
block/rbd.c +24 -31
index 4bbcce4eca..2e79c2d1fd 100644
@@ -326,28 +326,6 @@ static QemuOptsList runtime_opts = {
         /*
          * server.* extracted manually, see qemu_rbd_mon_host()
          */
-        {
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of secret providing the password",
-        },
-
-        /*
-         * Keys for qemu_rbd_parse_filename(), not in the QAPI schema
-         */
-        {
-            /*
-             * HACK: name starts with '=' so that qemu_opts_parse()
-             * can't set it
-             */
-            .name = "=keyvalue-pairs",
-            .type = QEMU_OPT_STRING,
-            .help = "Legacy rados key/value option parameters",
-        },
-        {
-            .name = "filename",
-            .type = QEMU_OPT_STRING,
-        },
         { /* end of list */ }
     },
 };
@@ -546,12 +524,13 @@ out:
 
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
                             char **s_snap, char **s_image_name,
-                            QDict *options, bool cache, Error **errp)
+                            QDict *options, bool cache,
+                            const char *keypairs, const char *secretid,
+                            Error **errp)
 {
     QemuOpts *opts;
     char *mon_host = NULL;
-    const char *pool, *snap, *conf, *user, *image_name, *keypairs;
-    const char *secretid;
+    const char *pool, *snap, *conf, *user, *image_name;
     Error *local_err = NULL;
     int r;
 
@@ -570,14 +549,11 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
         goto failed_opts;
     }
 
-    secretid = qemu_opt_get(opts, "password-secret");
-
     pool           = qemu_opt_get(opts, "pool");
     conf           = qemu_opt_get(opts, "conf");
     snap           = qemu_opt_get(opts, "snapshot");
     user           = qemu_opt_get(opts, "user");
     image_name     = qemu_opt_get(opts, "image");
-    keypairs       = qemu_opt_get(opts, "=keyvalue-pairs");
 
     if (!pool || !image_name) {
         error_setg(errp, "Parameters 'pool' and 'image' are required");
@@ -662,6 +638,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVRBDState *s = bs->opaque;
     Error *local_err = NULL;
     const char *filename;
+    char *keypairs, *secretid;
     int r;
 
     /* If we are given a filename, parse the filename, with precedence given to
@@ -672,16 +649,28 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                     "This is an unsupported option, and may be deprecated "
                     "in the future");
         qemu_rbd_parse_filename(filename, options, &local_err);
+        qdict_del(options, "filename");
         if (local_err) {
             error_propagate(errp, local_err);
             return -EINVAL;
         }
     }
 
+    keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+    if (keypairs) {
+        qdict_del(options, "=keyvalue-pairs");
+    }
+
+    secretid = g_strdup(qdict_get_try_str(options, "password-secret"));
+    if (secretid) {
+        qdict_del(options, "password-secret");
+    }
+
     r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name,
-                         options, !(flags & BDRV_O_NOCACHE), errp);
+                         options, !(flags & BDRV_O_NOCACHE), keypairs, secretid,
+                         errp);
     if (r < 0) {
-        return r;
+        goto out;
     }
 
     /* rbd_open is always r/w */
@@ -708,13 +697,17 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    return 0;
+    r = 0;
+    goto out;
 
 failed_open:
     rados_ioctx_destroy(s->io_ctx);
     g_free(s->snap);
     g_free(s->image_name);
     rados_shutdown(s->cluster);
+out:
+    g_free(keypairs);
+    g_free(secretid);
     return r;
 }
 
Signature · 1 line
2.13.6
#44 lore raw

Re: [Qemu-devel] [PATCH v2 20/36] rbd: Remove non-schema options from runtime_opts

Quoted 15 lines · Kevin Wolf

Instead of the QemuOpts in qemu_rbd_connect(), we want to use QAPI objects. As a preparation, fetch those options directly from the QDict that .bdrv_open() supports in the rbd driver and that are not in the schema.

---
 block/rbd.c | 55 ++++++++++++++++++++++++-------------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)
block/rbd.c +0 -0
index 4bbcce4eca..2e79c2d1fd 100644

block/rbd.c static QemuOptsList runtime_opts = { · line 333

Quoted 8 lines
block/rbd.c +0 -5
@@ -326,28 +326,6 @@ static QemuOptsList runtime_opts = {
         /*
          * server.* extracted manually, see qemu_rbd_mon_host()
          */
-        {
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of secret providing the password",
-        },

Err, did we just forget this in the QAPI schema or is it really not supposed to be there?

Max

Reviewed-by
#45 lore raw

Re: [Qemu-devel] [PATCH v2 20/36] rbd: Remove non-schema options from runtime_opts

Quoted 11 lines · Max Reitz
Quoted 10 lines · Kevin Wolf

Instead of the QemuOpts in qemu_rbd_connect(), we want to use QAPI objects. As a preparation, fetch those options directly from the QDict that .bdrv_open() supports in the rbd driver and that are not in the schema.

---
 block/rbd.c | 55 ++++++++++++++++++++++++-------------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)
Quoted 16 lines

block/rbd.c static QemuOptsList runtime_opts = { · line 333

Quoted 13 lines
block/rbd.c +0 -5
index 4bbcce4eca..2e79c2d1fd 100644
@@ -326,28 +326,6 @@ static QemuOptsList runtime_opts = {
         /*
          * server.* extracted manually, see qemu_rbd_mon_host()
          */
-        {
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of secret providing the password",
-        },

Err, did we just forget this in the QAPI schema or is it really not supposed to be there?

Ah, we intentionally removed it after it was added. OK, then.

author
#46 lore raw

PATCH v2 21/36 rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()

In reply to Kevin Wolf

With the conversion to a QAPI options object, the function is now prepared to be used in a .bdrv_co_create implementation.

---
 block/rbd.c | 102 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 52 insertions(+), 50 deletions(-)
block/rbd.c +52 -50
index 2e79c2d1fd..26641e53e0 100644
@@ -24,6 +24,8 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi-visit.h"
 
 /*
  * When specifying the image filename use:
@@ -482,24 +484,27 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
     qemu_aio_unref(acb);
 }
 
-static char *qemu_rbd_mon_host(QDict *options, Error **errp)
+static char *qemu_rbd_mon_host(BlockdevOptionsRbd *opts, Error **errp)
 {
-    const char **vals = g_new(const char *, qdict_size(options) + 1);
-    char keybuf[32];
+    const char **vals;
     const char *host, *port;
     char *rados_str;
-    int i;
-
-    for (i = 0;; i++) {
-        sprintf(keybuf, "server.%d.host", i);
-        host = qdict_get_try_str(options, keybuf);
-        qdict_del(options, keybuf);
-        sprintf(keybuf, "server.%d.port", i);
-        port = qdict_get_try_str(options, keybuf);
-        qdict_del(options, keybuf);
-        if (!host && !port) {
-            break;
-        }
+    InetSocketAddressBaseList *p;
+    int i, cnt;
+
+    if (!opts->has_server) {
+        return NULL;
+    }
+
+    for (cnt = 0, p = opts->server; p; p = p->next) {
+        cnt++;
+    }
+
+    vals = g_new(const char *, cnt + 1);
+
+    for (i = 0, p = opts->server; p; p = p->next, i++) {
+        host = p->value->host;
+        port = p->value->port;
         if (!host) {
             error_setg(errp, "Parameter server.%d.host is missing", i);
             rados_str = NULL;
@@ -524,56 +529,34 @@ out:
 
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
                             char **s_snap, char **s_image_name,
-                            QDict *options, bool cache,
+                            BlockdevOptionsRbd *opts, bool cache,
                             const char *keypairs, const char *secretid,
                             Error **errp)
 {
-    QemuOpts *opts;
     char *mon_host = NULL;
-    const char *pool, *snap, *conf, *user, *image_name;
     Error *local_err = NULL;
     int r;
 
-    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(opts, options, &local_err);
+    mon_host = qemu_rbd_mon_host(opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         r = -EINVAL;
         goto failed_opts;
     }
 
-    mon_host = qemu_rbd_mon_host(options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        r = -EINVAL;
-        goto failed_opts;
-    }
-
-    pool           = qemu_opt_get(opts, "pool");
-    conf           = qemu_opt_get(opts, "conf");
-    snap           = qemu_opt_get(opts, "snapshot");
-    user           = qemu_opt_get(opts, "user");
-    image_name     = qemu_opt_get(opts, "image");
-
-    if (!pool || !image_name) {
-        error_setg(errp, "Parameters 'pool' and 'image' are required");
-        r = -EINVAL;
-        goto failed_opts;
-    }
-
-    r = rados_create(cluster, user);
+    r = rados_create(cluster, opts->user);
     if (r < 0) {
         error_setg_errno(errp, -r, "error initializing");
         goto failed_opts;
     }
 
-    *s_snap = g_strdup(snap);
-    *s_image_name = g_strdup(image_name);
+    *s_snap = g_strdup(opts->snapshot);
+    *s_image_name = g_strdup(opts->image);
 
     /* try default location when conf=NULL, but ignore failure */
-    r = rados_conf_read_file(*cluster, conf);
-    if (conf && r < 0) {
-        error_setg_errno(errp, -r, "error reading conf file %s", conf);
+    r = rados_conf_read_file(*cluster, opts->conf);
+    if (opts->has_conf && r < 0) {
+        error_setg_errno(errp, -r, "error reading conf file %s", opts->conf);
         goto failed_shutdown;
     }
 
@@ -613,13 +596,12 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
         goto failed_shutdown;
     }
 
-    r = rados_ioctx_create(*cluster, pool, io_ctx);
+    r = rados_ioctx_create(*cluster, opts->pool, io_ctx);
     if (r < 0) {
-        error_setg_errno(errp, -r, "error opening pool %s", pool);
+        error_setg_errno(errp, -r, "error opening pool %s", opts->pool);
         goto failed_shutdown;
     }
 
-    qemu_opts_del(opts);
     return 0;
 
 failed_shutdown:
@@ -627,7 +609,6 @@ failed_shutdown:
     g_free(*s_snap);
     g_free(*s_image_name);
 failed_opts:
-    qemu_opts_del(opts);
     g_free(mon_host);
     return r;
 }
@@ -636,6 +617,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
+    BlockdevOptionsRbd *opts = NULL;
+    Visitor *v;
+    QObject *crumpled = NULL;
     Error *local_err = NULL;
     const char *filename;
     char *keypairs, *secretid;
@@ -666,8 +650,25 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         qdict_del(options, "password-secret");
     }
 
+    /* Convert the remaining options into a QAPI object */
+    crumpled = qdict_crumple(options, errp);
+    if (crumpled == NULL) {
+        return -EINVAL;
+    }
+
+    v = qobject_input_visitor_new_keyval(crumpled);
+    visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
+    visit_free(v);
+    qobject_decref(crumpled);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        r = -EINVAL;
+        goto out;
+    }
+
     r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name,
-                         options, !(flags & BDRV_O_NOCACHE), keypairs, secretid,
+                         opts, !(flags & BDRV_O_NOCACHE), keypairs, secretid,
                          errp);
     if (r < 0) {
         goto out;
@@ -706,6 +707,7 @@ failed_open:
     g_free(s->image_name);
     rados_shutdown(s->cluster);
 out:
+    qapi_free_BlockdevOptionsRbd(opts);
     g_free(keypairs);
     g_free(secretid);
     return r;
Signature · 1 line
2.13.6
#47 lore raw

Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()

Quoted 12 lines · Kevin Wolf

With the conversion to a QAPI options object, the function is now prepared to be used in a .bdrv_co_create implementation.

---
 block/rbd.c | 102 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 52 insertions(+), 50 deletions(-)
block/rbd.c +0 -0
index 2e79c2d1fd..26641e53e0 100644

[...]

Quoted 33 lines
block/rbd.c +11 -15
@@ -482,24 +484,27 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
     qemu_aio_unref(acb);
 }
 
-static char *qemu_rbd_mon_host(QDict *options, Error **errp)
+static char *qemu_rbd_mon_host(BlockdevOptionsRbd *opts, Error **errp)
 {
-    const char **vals = g_new(const char *, qdict_size(options) + 1);
-    char keybuf[32];
+    const char **vals;
     const char *host, *port;
     char *rados_str;
-    int i;
-
-    for (i = 0;; i++) {
-        sprintf(keybuf, "server.%d.host", i);
-        host = qdict_get_try_str(options, keybuf);
-        qdict_del(options, keybuf);
-        sprintf(keybuf, "server.%d.port", i);
-        port = qdict_get_try_str(options, keybuf);
-        qdict_del(options, keybuf);
-        if (!host && !port) {
-            break;
-        }
+    InetSocketAddressBaseList *p;
+    int i, cnt;
+
+    if (!opts->has_server) {
+        return NULL;
+    }
+
+    for (cnt = 0, p = opts->server; p; p = p->next) {
+        cnt++;

block/rbd.c static void qemu_rbd_complete_aio(RADOSCB *rcb) · line 510

Quoted 10 lines
block/rbd.c +7 -0
+    }
+
+    vals = g_new(const char *, cnt + 1);
+
+    for (i = 0, p = opts->server; p; p = p->next, i++) {
+        host = p->value->host;
+        port = p->value->port;
         if (!host) {
             error_setg(errp, "Parameter server.%d.host is missing", i);
             rados_str = NULL;

host and port are mandatory in the schema, so we can omit this now.

Quoted 51 lines
block/rbd.c +3 -26
@@ -524,56 +529,34 @@ out:
 
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
                             char **s_snap, char **s_image_name,
-                            QDict *options, bool cache,
+                            BlockdevOptionsRbd *opts, bool cache,
                             const char *keypairs, const char *secretid,
                             Error **errp)
 {
-    QemuOpts *opts;
     char *mon_host = NULL;
-    const char *pool, *snap, *conf, *user, *image_name;
     Error *local_err = NULL;
     int r;
 
-    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(opts, options, &local_err);
+    mon_host = qemu_rbd_mon_host(opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         r = -EINVAL;
         goto failed_opts;
     }
 
-    mon_host = qemu_rbd_mon_host(options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        r = -EINVAL;
-        goto failed_opts;
-    }
-
-    pool           = qemu_opt_get(opts, "pool");
-    conf           = qemu_opt_get(opts, "conf");
-    snap           = qemu_opt_get(opts, "snapshot");
-    user           = qemu_opt_get(opts, "user");
-    image_name     = qemu_opt_get(opts, "image");
-
-    if (!pool || !image_name) {
-        error_setg(errp, "Parameters 'pool' and 'image' are required");
-        r = -EINVAL;
-        goto failed_opts;
-    }
-
-    r = rados_create(cluster, user);
+    r = rados_create(cluster, opts->user);
     if (r < 0) {
         error_setg_errno(errp, -r, "error initializing");
         goto failed_opts;
     }
 
-    *s_snap = g_strdup(snap);

block/rbd.c out: · line 558

Quoted 10 lines
block/rbd.c +4 -4
-    *s_image_name = g_strdup(image_name);
+    *s_snap = g_strdup(opts->snapshot);
+    *s_image_name = g_strdup(opts->image);
 
     /* try default location when conf=NULL, but ignore failure */
-    r = rados_conf_read_file(*cluster, conf);
-    if (conf && r < 0) {
-        error_setg_errno(errp, -r, "error reading conf file %s", conf);
+    r = rados_conf_read_file(*cluster, opts->conf);
+    if (opts->has_conf && r < 0) {

Reading opts->conf without knowing whether opts->has_conf is true is a bit weird. Would you mind "s->has_conf ? opts->conf : NULL" for the rados_conf_read() call?

On that thought, opts->snapshot and opts->user are optional, too. Are they guaranteed to be NULL if they haven't been specified? Should we guard those accesses with opts->has_* queries, too?

Quoted 4 lines
block/rbd.c +1 -0
+        error_setg_errno(errp, -r, "error reading conf file %s", opts->conf);
         goto failed_shutdown;
     }
 

[..]

block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, · line 656

Quoted 8 lines
block/rbd.c +4 -0
@@ -666,8 +650,25 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         qdict_del(options, "password-secret");
     }
 
+    /* Convert the remaining options into a QAPI object */
+    crumpled = qdict_crumple(options, errp);
+    if (crumpled == NULL) {
+        return -EINVAL;

"goto out", or this will leak @keypairs and @secretid.

Max

Quoted 28 lines
block/rbd.c +15 -1
+    }
+
+    v = qobject_input_visitor_new_keyval(crumpled);
+    visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
+    visit_free(v);
+    qobject_decref(crumpled);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        r = -EINVAL;
+        goto out;
+    }
+
     r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name,
-                         options, !(flags & BDRV_O_NOCACHE), keypairs, secretid,
+                         opts, !(flags & BDRV_O_NOCACHE), keypairs, secretid,
                          errp);
     if (r < 0) {
         goto out;
@@ -706,6 +707,7 @@ failed_open:
     g_free(s->image_name);
     rados_shutdown(s->cluster);
 out:
+    qapi_free_BlockdevOptionsRbd(opts);
     g_free(keypairs);
     g_free(secretid);
     return r;
author
#48 lore raw

Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()

Am 23.02.2018 um 00:25 hat Max Reitz geschrieben:

Quoted 18 lines · Kevin Wolf
Quoted 6 lines

With the conversion to a QAPI options object, the function is now prepared to be used in a .bdrv_co_create implementation.

block/rbd.c +0 -1
-    *s_snap = g_strdup(snap);

block/rbd.c out: · line 558

Quoted 10 lines
block/rbd.c +4 -4
-    *s_image_name = g_strdup(image_name);
+    *s_snap = g_strdup(opts->snapshot);
+    *s_image_name = g_strdup(opts->image);
 
     /* try default location when conf=NULL, but ignore failure */
-    r = rados_conf_read_file(*cluster, conf);
-    if (conf && r < 0) {
-        error_setg_errno(errp, -r, "error reading conf file %s", conf);
+    r = rados_conf_read_file(*cluster, opts->conf);
+    if (opts->has_conf && r < 0) {
Quoted 7 lines

Reading opts->conf without knowing whether opts->has_conf is true is a bit weird. Would you mind "s->has_conf ? opts->conf : NULL" for the rados_conf_read() call?

On that thought, opts->snapshot and opts->user are optional, too. Are they guaranteed to be NULL if they haven't been specified? Should we guard those accesses with opts->has_* queries, too?

These days, both the QMP marshalling code (for the outermost struct when called from x-blockdev-create) and the input visitor (for nested structs and non-QMP callers) initialise the objects with {0} and g_malloc0().

I think Markus once told me that I shouldn't do pointless has_* checks any more in QMP commands, so I intentionally did the same here.

Kevin

#49 lore raw

Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()

Quoted 27 lines · Kevin Wolf

Am 23.02.2018 um 00:25 hat Max Reitz geschrieben:

Quoted 18 lines · Kevin Wolf
Quoted 6 lines

With the conversion to a QAPI options object, the function is now prepared to be used in a .bdrv_co_create implementation.

block/rbd.c +0 -1
-    *s_snap = g_strdup(snap);

block/rbd.c out: · line 558

Quoted 10 lines
block/rbd.c +4 -4
-    *s_image_name = g_strdup(image_name);
+    *s_snap = g_strdup(opts->snapshot);
+    *s_image_name = g_strdup(opts->image);
 
     /* try default location when conf=NULL, but ignore failure */
-    r = rados_conf_read_file(*cluster, conf);
-    if (conf && r < 0) {
-        error_setg_errno(errp, -r, "error reading conf file %s", conf);
+    r = rados_conf_read_file(*cluster, opts->conf);
+    if (opts->has_conf && r < 0) {
Quoted 7 lines

Reading opts->conf without knowing whether opts->has_conf is true is a bit weird. Would you mind "s->has_conf ? opts->conf : NULL" for the rados_conf_read() call?

On that thought, opts->snapshot and opts->user are optional, too. Are they guaranteed to be NULL if they haven't been specified? Should we guard those accesses with opts->has_* queries, too?

Quoted 6 lines

These days, both the QMP marshalling code (for the outermost struct when called from x-blockdev-create) and the input visitor (for nested structs and non-QMP callers) initialise the objects with {0} and g_malloc0().

I think Markus once told me that I shouldn't do pointless has_* checks any more in QMP commands, so I intentionally did the same here.

I'm a bit cautious because of non-zero defaults (like sslverify in the ssh driver), but as long as you're aware...

Max

author
#50 lore raw

Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()

Am 23.02.2018 um 17:43 hat Max Reitz geschrieben:

Quoted 32 lines · Kevin Wolf
Quoted 31 lines

Am 23.02.2018 um 00:25 hat Max Reitz geschrieben:

Quoted 18 lines · Kevin Wolf
Quoted 6 lines

With the conversion to a QAPI options object, the function is now prepared to be used in a .bdrv_co_create implementation.

block/rbd.c +0 -1
-    *s_snap = g_strdup(snap);

block/rbd.c out: · line 558

Quoted 10 lines
block/rbd.c +4 -4
-    *s_image_name = g_strdup(image_name);
+    *s_snap = g_strdup(opts->snapshot);
+    *s_image_name = g_strdup(opts->image);
 
     /* try default location when conf=NULL, but ignore failure */
-    r = rados_conf_read_file(*cluster, conf);
-    if (conf && r < 0) {
-        error_setg_errno(errp, -r, "error reading conf file %s", conf);
+    r = rados_conf_read_file(*cluster, opts->conf);
+    if (opts->has_conf && r < 0) {
Quoted 7 lines

Reading opts->conf without knowing whether opts->has_conf is true is a bit weird. Would you mind "s->has_conf ? opts->conf : NULL" for the rados_conf_read() call?

On that thought, opts->snapshot and opts->user are optional, too. Are they guaranteed to be NULL if they haven't been specified? Should we guard those accesses with opts->has_* queries, too?

These days, both the QMP marshalling code (for the outermost struct when called from x-blockdev-create) and the input visitor (for nested structs and non-QMP callers) initialise the objects with {0} and g_malloc0().

Quoted 5 lines
Quoted 2 lines

I think Markus once told me that I shouldn't do pointless has_* checks any more in QMP commands, so I intentionally did the same here.

I'm a bit cautious because of non-zero defaults (like sslverify in the ssh driver), but as long as you're aware...

I still hope that QAPI will allow specifying default values in the schema sometime. But yes, for the time being, not checking has_* obviously only works when the default is 0/false/NULL.

Kevin

author
#51 lore raw

PATCH v2 22/36 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 |  19 ++++++-
 block/rbd.c          | 146 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 116 insertions(+), 49 deletions(-)
qapi/block-core.json +18 -1
index 74021c51d7..6c0c16ebe3 100644
@@ -3436,6 +3436,23 @@
             '*refcount-bits':   'int' } }
 
 ##
+# @BlockdevCreateOptionsRbd:
+#
+# Driver specific image creation options for rbd/Ceph.
+#
+# @location         Where to store the new image file. This location cannot
+#                   point to a snapshot.
+# @size             Size of the virtual disk in bytes
+# @cluster-size     RBD object size
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsRbd',
+  'data': { 'location':         'BlockdevOptionsRbd',
+            'size':             'size',
+            '*cluster-size' :   'size' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.
@@ -3484,7 +3501,7 @@
       'qed':            'BlockdevCreateNotSupported',
       'quorum':         'BlockdevCreateNotSupported',
       'raw':            'BlockdevCreateNotSupported',
-      'rbd':            'BlockdevCreateNotSupported',
+      'rbd':            'BlockdevCreateOptionsRbd',
       'replication':    'BlockdevCreateNotSupported',
       'sheepdog':       'BlockdevCreateNotSupported',
       'ssh':            'BlockdevCreateNotSupported',
block/rbd.c +98 -48

204 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v2 22/36
#52 lore raw

Re: [Qemu-devel] [PATCH v2 22/36] 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 |  19 ++++++-
 block/rbd.c          | 146 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 116 insertions(+), 49 deletions(-)
author
#53 lore raw

PATCH v2 23/36 rbd: Assing s->snap/image_name in qemu_rbd_open()

In reply to Kevin Wolf

Now that the options are already available in qemu_rbd_open() and not only parsed in qemu_rbd_connect(), we can assign s->snap and s->image_name there instead of passing the fields by reference to qemu_rbd_connect().

---
 block/rbd.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
block/rbd.c +5 -9
index 82f03505a9..a34bf0be46 100644
@@ -577,7 +577,6 @@ out:
 }
 
 static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
-                            char **s_snap, char **s_image_name,
                             BlockdevOptionsRbd *opts, bool cache,
                             const char *keypairs, const char *secretid,
                             Error **errp)
@@ -599,9 +598,6 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
         goto failed_opts;
     }
 
-    *s_snap = g_strdup(opts->snapshot);
-    *s_image_name = g_strdup(opts->image);
-
     /* try default location when conf=NULL, but ignore failure */
     r = rados_conf_read_file(*cluster, opts->conf);
     if (opts->has_conf && r < 0) {
@@ -655,8 +651,6 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
 
 failed_shutdown:
     rados_shutdown(*cluster);
-    g_free(*s_snap);
-    g_free(*s_image_name);
 failed_opts:
     g_free(mon_host);
     return r;
@@ -716,13 +710,15 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
-    r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name,
-                         opts, !(flags & BDRV_O_NOCACHE), keypairs, secretid,
-                         errp);
+    r = qemu_rbd_connect(&s->cluster, &s->io_ctx, opts,
+                         !(flags & BDRV_O_NOCACHE), keypairs, secretid, errp);
     if (r < 0) {
         goto out;
     }
 
+    s->snap = g_strdup(opts->snapshot);
+    s->image_name = g_strdup(opts->image);
+
     /* rbd_open is always r/w */
     r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
     if (r < 0) {
Signature · 1 line
2.13.6
#54 lore raw

Re: [Qemu-devel] [PATCH v2 23/36] rbd: ***ing s->snap/image_name in qemu_rbd_open()

I suppose the first word after the colon in your subject is supposed to be "assign" and not what it currently is (which is something I am not going to repeat!). :-)

Quoted 14 lines · Kevin Wolf

Now that the options are already available in qemu_rbd_open() and not only parsed in qemu_rbd_connect(), we can assign s->snap and s->image_name there instead of passing the fields by reference to qemu_rbd_connect().

---
 block/rbd.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
block/rbd.c +0 -0
index 82f03505a9..a34bf0be46 100644

[...]

Quoted 6 lines
block/rbd.c +0 -2
@@ -716,13 +710,15 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
-    r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name,
-                         opts, !(flags & BDRV_O_NOCACHE), keypairs, secretid,

block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, · line 721

Quoted 10 lines
block/rbd.c +5 -1
-                         errp);
+    r = qemu_rbd_connect(&s->cluster, &s->io_ctx, opts,
+                         !(flags & BDRV_O_NOCACHE), keypairs, secretid, errp);
     if (r < 0) {
         goto out;
     }
 
+    s->snap = g_strdup(opts->snapshot);
+    s->image_name = g_strdup(opts->image);
+

Same question as in patch 21: Should we guard these by checking opts->has_* first?

Max

Quoted 4 lines
block/rbd.c +0 -0
     /* rbd_open is always r/w */
     r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
     if (r < 0) {
author
#55 lore raw

PATCH v2 24/36 rbd: Use qemu_rbd_connect() in qemu_rbd_do_create()

In reply to Kevin Wolf

This is almost exactly the same code. The differences are that qemu_rbd_connect() supports BlockdevOptionsRbd.server and that the cache mode is set explicitly.

Supporting 'server' is a welcome new feature for image creation. Caching is disabled by default, so leave it that way.

---
 block/rbd.c | 54 ++++++++++--------------------------------------------
 1 file changed, 10 insertions(+), 44 deletions(-)
block/rbd.c +10 -44
index a34bf0be46..af8e186106 100644
@@ -103,6 +103,11 @@ typedef struct BDRVRBDState {
     char *snap;
 } BDRVRBDState;
 
+static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
+                            BlockdevOptionsRbd *opts, bool cache,
+                            const char *keypairs, const char *secretid,
+                            Error **errp);
+
 static char *qemu_rbd_next_tok(char *src, char delim, char **p)
 {
     char *end;
@@ -351,12 +356,6 @@ static int qemu_rbd_do_create(BlockdevCreateOptions *options,
         return -EINVAL;
     }
 
-    /* TODO Remove the limitation */
-    if (opts->location->has_server) {
-        error_setg(errp, "Can't specify server for image creation");
-        return -EINVAL;
-    }
-
     if (opts->has_cluster_size) {
         int64_t objsize = opts->cluster_size;
         if ((objsize - 1) & objsize) {    /* not a power of 2? */
@@ -370,54 +369,21 @@ static int qemu_rbd_do_create(BlockdevCreateOptions *options,
         obj_order = ctz32(objsize);
     }
 
-    ret = rados_create(&cluster, opts->location->user);
+    ret = qemu_rbd_connect(&cluster, &io_ctx, opts->location, false, keypairs,
+                           NULL, errp);
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "error initializing");
         return ret;
     }
 
-    /* try default location when conf=NULL, but ignore failure */
-    ret = rados_conf_read_file(cluster, opts->location->conf);
-    if (opts->location->conf && ret < 0) {
-        error_setg_errno(errp, -ret, "error reading conf file %s",
-                         opts->location->conf);
-        ret = -EIO;
-        goto shutdown;
-    }
-
-    ret = qemu_rbd_set_keypairs(cluster, keypairs, errp);
-    if (ret < 0) {
-        ret = -EIO;
-        goto shutdown;
-    }
-
-    if (qemu_rbd_set_auth(cluster, password_secret, errp) < 0) {
-        ret = -EIO;
-        goto shutdown;
-    }
-
-    ret = rados_connect(cluster);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret, "error connecting");
-        goto shutdown;
-    }
-
-    ret = rados_ioctx_create(cluster, opts->location->pool, &io_ctx);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret, "error opening pool %s",
-                         opts->location->pool);
-        goto shutdown;
-    }
-
     ret = rbd_create(io_ctx, opts->location->image, opts->size, &obj_order);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error rbd create");
+        goto out;
     }
 
-    rados_ioctx_destroy(io_ctx);
-
     ret = 0;
-shutdown:
+out:
+    rados_ioctx_destroy(io_ctx);
     rados_shutdown(cluster);
     return ret;
 }
Signature · 1 line
2.13.6
#56 lore raw

Re: [Qemu-devel] [PATCH v2 24/36] rbd: Use qemu_rbd_connect() in qemu_rbd_do_create()

Quoted 16 lines · Kevin Wolf

This is almost exactly the same code. The differences are that qemu_rbd_connect() supports BlockdevOptionsRbd.server and that the cache mode is set explicitly.

Supporting 'server' is a welcome new feature for image creation. Caching is disabled by default, so leave it that way.

---
 block/rbd.c | 54 ++++++++++--------------------------------------------
 1 file changed, 10 insertions(+), 44 deletions(-)
block/rbd.c +0 -0
index a34bf0be46..af8e186106 100644

[...]

block/rbd.c static int qemu_rbd_do_create(BlockdevCreateOptions *options, · line 373

Quoted 7 lines
block/rbd.c +2 -1
@@ -370,54 +369,21 @@ static int qemu_rbd_do_create(BlockdevCreateOptions *options,
         obj_order = ctz32(objsize);
     }
 
-    ret = rados_create(&cluster, opts->location->user);
+    ret = qemu_rbd_connect(&cluster, &io_ctx, opts->location, false, keypairs,
+                           NULL, errp);

s/NULL/password_secret/?

Max

author
#57 lore raw

PATCH v2 25/36 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 6576a73d6e..9283bfbaae 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 = NULL;
+    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
author
#58 lore raw

PATCH v2 26/36 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 6c0c16ebe3..085b791303 100644
@@ -3393,6 +3393,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)
@@ -3491,7 +3505,7 @@
       'iscsi':          'BlockdevCreateNotSupported',
       'luks':           'BlockdevCreateNotSupported',
       'nbd':            'BlockdevCreateNotSupported',
-      'nfs':            'BlockdevCreateNotSupported',
+      'nfs':            'BlockdevCreateOptionsNfs',
       'null-aio':       'BlockdevCreateNotSupported',
       'null-co':        'BlockdevCreateNotSupported',
       'nvme':           'BlockdevCreateNotSupported',
block/nfs.c +59 -15

130 lines not on this page · raw message

Signature · 1 line
2.13.6
author
#59 lore raw

PATCH v2 27/36 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     | 94 +++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 112 insertions(+), 27 deletions(-)
qapi/block-core.json +45 -0
index 085b791303..2b249c9e3d 100644
@@ -3467,6 +3467,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 +67 -27

127 lines not on this page · raw message

Signature · 1 line
2.13.6
Reviewed-by on v2 27/36
#60 lore raw

Re: [Qemu-devel] [PATCH v2 27/36] 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     | 94 +++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 112 insertions(+), 27 deletions(-)

Ooooh, thanks!

author
#61 lore raw

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

In reply to Kevin Wolf

Am 21.02.2018 um 14:53 hat Kevin Wolf geschrieben:

Quoted 8 lines

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.

s/redundacy/redundancy/

Both in the subject line and the commit message. Autocompletion is great, it helps to apply typos consistently.

Kevin

author
#62 lore raw

PATCH v2 28/36 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     | 240 +++++++++++++++++++++++++++++++++++----------------
 2 files changed, 189 insertions(+), 75 deletions(-)
qapi/block-core.json +23 -1
index 2b249c9e3d..f7679fce53 100644
@@ -3512,6 +3512,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.
@@ -3562,7 +3584,7 @@
       'raw':            'BlockdevCreateNotSupported',
       'rbd':            'BlockdevCreateOptionsRbd',
       'replication':    'BlockdevCreateNotSupported',
-      'sheepdog':       'BlockdevCreateNotSupported',
+      'sheepdog':       'BlockdevCreateOptionsSheepdog',
       'ssh':            'BlockdevCreateNotSupported',
       'throttle':       'BlockdevCreateNotSupported',
       'vdi':            'BlockdevCreateNotSupported',
block/sheepdog.c +166 -74

376 lines not on this page · raw message

Signature · 1 line
2.13.6
#63 lore raw

Re: [Qemu-devel] [PATCH v2 28/36] sheepdog: Support .bdrv_co_create

Quoted 8 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     | 240 +++++++++++++++++++++++++++++++++++----------------
 2 files changed, 189 insertions(+), 75 deletions(-)

[...]

Quoted 4 lines
block/sheepdog.c +0 -0
index 22df2ba9d0..d45cf68ff2 100644

[...]

Quoted 18 lines
block/sheepdog.c +14 -0
@@ -1882,6 +1866,42 @@ out_with_err_set:
     return ret;
 }
 
+static int sd_create_prealloc(BlockdevOptionsSheepdog *location, int64_t size,
+                              Error **errp)
+{
+    BlockDriverState *bs;
+    Visitor *v;
+    QObject *obj = NULL;
+    QDict *qdict;
+    Error *local_err = NULL;
+    int ret;
+
+    v = qobject_output_visitor_new(&obj);
+    visit_type_BlockdevOptionsSheepdog(v, NULL, &location, &local_err);
+    visit_free(v);
+

block/sheepdog.c out_with_err_set: · line 1892

Quoted 10 lines
block/sheepdog.c +10 -0
+    if (local_err) {
+        error_propagate(errp, local_err);
+        qobject_decref(obj);
+        return -EINVAL;
+    }
+
+    qdict = qobject_to_qdict(obj);
+    qdict_flatten(qdict);
+
+    bs = bdrv_open(NULL, NULL, qdict, BDRV_O_PROTOCOL | BDRV_O_RDWR, errp);

Hu, nice, we can even get rid of the filename again.

But I am wondering where "driver": "sheepdog" comes into qdict...

Max

Quoted 15 lines
block/sheepdog.c +12 -0
+    if (bs == NULL) {
+        ret = -EIO;
+        goto fail;
+    }
+
+    ret = sd_prealloc(bs, 0, size, errp);
+fail:
+    bdrv_unref(bs);
+    QDECREF(qdict);
+    return ret;
+}
+
 static int parse_redundancy(BDRVSheepdogState *s, SheepdogRedundancy *opt)
 {
     struct SheepdogInode *inode = &s->inode;
author
#64 lore raw

PATCH v2 29/36 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 b63addcf94..9a89b7f350 100644
@@ -543,21 +543,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",
-        },
         { /* end of list */ }
     },
 };
@@ -582,23 +567,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;
     }
 
     /*
@@ -609,51 +602,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");
@@ -662,17 +654,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");
@@ -729,15 +713,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) {
@@ -764,7 +750,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
author
#65 lore raw

PATCH v2 30/36 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 f7679fce53..431d4a4fb2 100644
@@ -2553,6 +2553,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
@@ -2562,14 +2619,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

140 lines not on this page · raw message

Signature · 1 line
2.13.6
author
#66 lore raw

PATCH v2 31/36 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 dcf766c213..77bc20041f 100644
@@ -655,19 +655,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 {
@@ -747,8 +741,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");
@@ -774,8 +766,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     }
     s->session = NULL;
 
-    qapi_free_BlockdevOptionsSsh(opts);
-
     return ret;
 }
 
@@ -783,6 +773,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;
 
@@ -793,8 +784,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;
     }
@@ -802,6 +798,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:
@@ -810,6 +808,8 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
     }
     s->sock = -1;
 
+    qapi_free_BlockdevOptionsSsh(opts);
+
     return ret;
 }
 
@@ -831,6 +831,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' };
@@ -849,7 +850,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);
@@ -876,6 +883,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
author
#67 lore raw

PATCH v2 32/36 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 431d4a4fb2..2f7fab46eb 100644
@@ -3593,6 +3593,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.
@@ -3644,7 +3658,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
#68 lore raw

Re: [Qemu-devel] [PATCH v2 32/36] ssh: Support .bdrv_co_create

Quoted 9 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(-)

This needs a rebase on my ssh truncation patches (notably "block/ssh:
Pull ssh_grow_file() from ssh_create()") -- good thing for me you need to rebase, because without those patches you cannot create qcow2 files over ssh. O:-)

Max

#69 lore raw

Re: [Qemu-devel] [PATCH v2 32/36] ssh: Support .bdrv_co_create

Quoted 11 lines · Max Reitz
Quoted 9 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(-)
Quoted 4 lines

This needs a rebase on my ssh truncation patches (notably "block/ssh:
Pull ssh_grow_file() from ssh_create()") -- good thing for me you need to rebase, because without those patches you cannot create qcow2 files over ssh. O:-)

Oops, meant to reply to v3...

Max

author
#70 lore raw

PATCH v2 33/36 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 ba14ed9459..6aed5bca0b 100644
@@ -1686,11 +1686,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 v2 33/36
#71 lore raw

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

Quoted 4 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.

Quoted 9 lines

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(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#72 lore raw

PATCH v2 34/36 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 4a7e448226..5c874aefa1 100644
@@ -3684,6 +3684,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 v2 34/36
#73 lore raw

Re: [Qemu-devel] [PATCH v2 34/36] block: Fail bdrv_truncate() with negative size

Quoted 10 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(+)

Are there any callers that can be simplified (such as now assert()ing rather than checking and returning an error) due to better guarantees about a non-negative argument?

Quoted 8 lines
block.c +0 -0
index 4a7e448226..5c874aefa1 100644
@@ -3684,6 +3684,11 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,
          error_setg(errp, "No medium inserted");

block.c int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc, · line 3694

Quoted 10 lines
block.c +5 -0
          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 · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#74 lore raw

PATCH v2 35/36 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
author
#75 lore raw

PATCH v2 36/36 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
#76 lore raw

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

Quoted 9 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

Minor note: If this test tried to create a qcow2 image over ssh, it would have seen "Image format driver does not support resize" without the ssh truncation patches.

But I'm not sure whether such a test case should be added here, because technically this test then becomes a qcow2+ssh test, and nobody is ever going to run the iotests with the qcow2+ssh combination.

(We could cheat and still mark this test as raw and then just create a qcow2 image nonetheless...)

Max