Changeset
MAINTAINERS               |   2 +
block.c                   |   1 +
block/crypto.c            |   6 +-
block/gluster.c           |   1 +
block/iscsi.c             |  24 +-
block/nbd.c               |  16 +-
block/nfs.c               |   8 +-
block/parallels.c         |  11 +-
block/qcow.c              |  11 +-
block/qcow2.c             |  11 +-
block/qed.c               |  11 +-
block/quorum.c            |   1 +
block/rbd.c               |  85 +++--
block/sheepdog.c          |  23 +-
block/snapshot.c          |   1 +
block/ssh.c               |  16 +-
block/vdi.c               |   4 +-
block/vhdx.c              |  11 +-
block/vpc.c               |  11 +-
block/vvfat.c             |   1 +
block/vxhs.c              |   1 +
blockdev.c                |   1 +
include/block/qdict.h     |  34 ++
include/qapi/qmp/qdict.h  |  17 -
qapi/block-core.json      |  19 +
qobject/Makefile.objs     |   1 +
qobject/block-qdict.c     | 722 ++++++++++++++++++++++++++++++++++++++
qobject/qdict.c           | 628 ---------------------------------
tests/Makefile.include    |   4 +
tests/check-block-qdict.c | 690 ++++++++++++++++++++++++++++++++++++
tests/check-qdict.c       | 641 ---------------------------------
tests/check-qobject.c     |   1 +
tests/test-replication.c  |   1 +
util/qemu-config.c        |   1 +
34 files changed, 1577 insertions(+), 1439 deletions(-)
create mode 100644 include/block/qdict.h
create mode 100644 qobject/block-qdict.c
create mode 100644 tests/check-block-qdict.c
Git apply log
Switched to a new branch '20180612125821.4229-1-armbru@redhat.com'
Applying: rbd: Drop deprecated -drive parameter "filename"
Applying: iscsi: Drop deprecated -drive parameter "filename"
Applying: block: Add block-specific QDict header
Applying: qobject: Move block-specific qdict code to block-qdict.c
Applying: block: Fix -blockdev for certain non-string scalars
Applying: block: Fix -drive for certain non-string scalars
Applying: block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts()
Applying: block: Factor out qobject_input_visitor_new_flat_confused()
Applying: block: Make remaining uses of qobject input visitor more robust
Applying: block-qdict: Simplify qdict_flatten_qdict()
Applying: block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist()
Applying: block-qdict: Clean up qdict_crumple() a bit
Applying: block-qdict: Simplify qdict_is_list() some
Applying: check-block-qdict: Rename qdict_flatten()'s variables for clarity
Applying: check-block-qdict: Cover flattening of empty lists and dictionaries
Applying: block: Fix -blockdev / blockdev-add for empty objects and arrays
Applying: rbd: New parameter auth-client-required
Applying: rbd: New parameter key-secret
To https://github.com/patchew-project/qemu
 + fe95caf...a6fee0e patchew/20180612125821.4229-1-armbru@redhat.com -> patchew/20180612125821.4229-1-armbru@redhat.com (forced update)
Test passed: checkpatch

loading

Test passed: docker-mingw@fedora

loading

Test passed: docker-quick@centos7

loading

Test passed: s390x

loading

[Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication
Posted by Markus Armbruster, 1 week ago
PATCH 01-17 are configuration fixes and cleanup, in particular
-blockdev driver=nfs,... and -drive driver=(nbd|sheepdog|ssh),... with
non-string scalars.

PATCH 18-19 provide support for configuring rbd authentication.

I'm happy to split the series if that helps.

Since the RFC post, I amended the documentation of the rbd
authentication parameters, and squashed PATCH 04 into 03.

Jeff Cody tested the RFC on his Ceph rig.  All results match
expectations.

The series clashes with Max's "[PATCH 00/13] block: Try to create well
typed json:{} filenames", but Max has rebased his work on mine.

Markus Armbruster (17):
  rbd: Drop deprecated -drive parameter "filename"
  iscsi: Drop deprecated -drive parameter "filename"
  qobject: Move block-specific qdict code to block-qdict.c
  block: Fix -blockdev for certain non-string scalars
  block: Fix -drive for certain non-string scalars
  block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts()
  block: Factor out qobject_input_visitor_new_flat_confused()
  block: Make remaining uses of qobject input visitor more robust
  block-qdict: Simplify qdict_flatten_qdict()
  block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist()
  block-qdict: Clean up qdict_crumple() a bit
  block-qdict: Simplify qdict_is_list() some
  check-block-qdict: Rename qdict_flatten()'s variables for clarity
  check-block-qdict: Cover flattening of empty lists and dictionaries
  block: Fix -blockdev / blockdev-add for empty objects and arrays
  rbd: New parameter auth-client-required
  rbd: New parameter key-secret

Max Reitz (1):
  block: Add block-specific QDict header

 MAINTAINERS               |   2 +
 block.c                   |   1 +
 block/crypto.c            |   6 +-
 block/gluster.c           |   1 +
 block/iscsi.c             |  24 +-
 block/nbd.c               |  16 +-
 block/nfs.c               |   8 +-
 block/parallels.c         |  11 +-
 block/qcow.c              |  11 +-
 block/qcow2.c             |  11 +-
 block/qed.c               |  11 +-
 block/quorum.c            |   1 +
 block/rbd.c               |  85 +++--
 block/sheepdog.c          |  23 +-
 block/snapshot.c          |   1 +
 block/ssh.c               |  16 +-
 block/vdi.c               |   4 +-
 block/vhdx.c              |  11 +-
 block/vpc.c               |  11 +-
 block/vvfat.c             |   1 +
 block/vxhs.c              |   1 +
 blockdev.c                |   1 +
 include/block/qdict.h     |  34 ++
 include/qapi/qmp/qdict.h  |  17 -
 qapi/block-core.json      |  19 +
 qobject/Makefile.objs     |   1 +
 qobject/block-qdict.c     | 722 ++++++++++++++++++++++++++++++++++++++
 qobject/qdict.c           | 628 ---------------------------------
 tests/Makefile.include    |   4 +
 tests/check-block-qdict.c | 690 ++++++++++++++++++++++++++++++++++++
 tests/check-qdict.c       | 641 ---------------------------------
 tests/check-qobject.c     |   1 +
 tests/test-replication.c  |   1 +
 util/qemu-config.c        |   1 +
 34 files changed, 1577 insertions(+), 1439 deletions(-)
 create mode 100644 include/block/qdict.h
 create mode 100644 qobject/block-qdict.c
 create mode 100644 tests/check-block-qdict.c

-- 
2.17.1


Re: [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication
Posted by Kevin Wolf, 1 week ago
Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> PATCH 01-17 are configuration fixes and cleanup, in particular
> -blockdev driver=nfs,... and -drive driver=(nbd|sheepdog|ssh),... with
> non-string scalars.
> 
> PATCH 18-19 provide support for configuring rbd authentication.
> 
> I'm happy to split the series if that helps.
> 
> Since the RFC post, I amended the documentation of the rbd
> authentication parameters, and squashed PATCH 04 into 03.

I was still reviewing the RFC while you sent this, so I posted two
comments that are still relevant for this version in the RFC thread.
Please read them there, I'm not going to duplicate them here.

Will continue with the review on this one now.

Kevin

Re: [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication
Posted by Kevin Wolf, 1 week ago
Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> PATCH 01-17 are configuration fixes and cleanup, in particular
> -blockdev driver=nfs,... and -drive driver=(nbd|sheepdog|ssh),... with
> non-string scalars.
> 
> PATCH 18-19 provide support for configuring rbd authentication.
> 
> I'm happy to split the series if that helps.
> 
> Since the RFC post, I amended the documentation of the rbd
> authentication parameters, and squashed PATCH 04 into 03.
> 
> Jeff Cody tested the RFC on his Ceph rig.  All results match
> expectations.
> 
> The series clashes with Max's "[PATCH 00/13] block: Try to create well
> typed json:{} filenames", but Max has rebased his work on mine.

I had minor, but still non-trivial comments for patches 9 and 12. The
rest is:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename"
Posted by Markus Armbruster, 1 week ago
Parameter "filename" is deprecated since commit 91589d9e5ca, v2.10.0.
Time to get rid of it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index a16431e267..40c6e4185f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -632,25 +632,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     QObject *crumpled = NULL;
     const QDictEntry *e;
     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
-     * 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);
-        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");
-- 
2.17.1


[Qemu-devel] [PATCH 02/18] iscsi: Drop deprecated -drive parameter "filename"
Posted by Markus Armbruster, 1 week ago
Parameter "filename" is deprecated since commit 5c3ad1a6a8f, v2.10.0.
Time to get rid of it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/iscsi.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index c2fbd8a8aa..7e3ea72bd2 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1713,10 +1713,6 @@ static QemuOptsList runtime_opts = {
             .name = "timeout",
             .type = QEMU_OPT_NUMBER,
         },
-        {
-            .name = "filename",
-            .type = QEMU_OPT_STRING,
-        },
         { /* end of list */ }
     },
 };
@@ -1756,27 +1752,12 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     char *initiator_name = NULL;
     QemuOpts *opts;
     Error *local_err = NULL;
-    const char *transport_name, *portal, *target, *filename;
+    const char *transport_name, *portal, *target;
 #if LIBISCSI_API_VERSION >= (20160603)
     enum iscsi_transport_type transport;
 #endif
     int i, ret = 0, timeout = 0, lun;
 
-    /* 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");
-        iscsi_parse_filename(filename, options, &local_err);
-        if (local_err) {
-            ret = -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) {
@@ -2006,7 +1987,7 @@ out:
         }
         memset(iscsilun, 0, sizeof(IscsiLun));
     }
-exit:
+
     return ret;
 }
 
-- 
2.17.1


[Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header
Posted by Markus Armbruster, 1 week ago
From: Max Reitz <mreitz@redhat.com>

There are numerous QDict functions that have been introduced for and are
used only by the block layer.  Move their declarations into an own
header file to reflect that.

While qdict_extract_subqdict() is in fact used outside of the block
layer (in util/qemu-config.c), it is still a function related very
closely to how the block layer works with nested QDicts, namely by
sometimes flattening them.  Therefore, its declaration is put into this
header as well and util/qemu-config.c includes it with a comment stating
exactly which function it needs.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20180509165530.29561-7-mreitz@redhat.com>
[Copright note tweaked, superfluous includes dropped]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                  |  1 +
 block/gluster.c          |  1 +
 block/iscsi.c            |  1 +
 block/nbd.c              |  1 +
 block/nfs.c              |  1 +
 block/parallels.c        |  1 +
 block/qcow.c             |  1 +
 block/qcow2.c            |  1 +
 block/qed.c              |  1 +
 block/quorum.c           |  1 +
 block/rbd.c              |  1 +
 block/sheepdog.c         |  1 +
 block/snapshot.c         |  1 +
 block/ssh.c              |  1 +
 block/vhdx.c             |  1 +
 block/vpc.c              |  1 +
 block/vvfat.c            |  1 +
 block/vxhs.c             |  1 +
 blockdev.c               |  1 +
 include/block/qdict.h    | 32 ++++++++++++++++++++++++++++++++
 include/qapi/qmp/qdict.h | 17 -----------------
 qobject/qdict.c          |  1 +
 tests/check-qdict.c      |  1 +
 tests/check-qobject.c    |  1 +
 tests/test-replication.c |  1 +
 util/qemu-config.c       |  1 +
 26 files changed, 56 insertions(+), 17 deletions(-)
 create mode 100644 include/block/qdict.h

diff --git a/block.c b/block.c
index 50887087f3..afe30caac3 100644
--- a/block.c
+++ b/block.c
@@ -27,6 +27,7 @@
 #include "block/block_int.h"
 #include "block/blockjob.h"
 #include "block/nbd.h"
+#include "block/qdict.h"
 #include "qemu/error-report.h"
 #include "module_block.h"
 #include "qemu/module.h"
diff --git a/block/gluster.c b/block/gluster.c
index 9900b6420c..b5fe7f3e87 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -11,6 +11,7 @@
 #include "qemu/osdep.h"
 #include <glusterfs/api/glfs.h>
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
diff --git a/block/iscsi.c b/block/iscsi.c
index 7e3ea72bd2..9f00fb47a5 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -33,6 +33,7 @@
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "scsi/constants.h"
 #include "qemu/iov.h"
 #include "qemu/option.h"
diff --git a/block/nbd.c b/block/nbd.c
index ff8333e3c1..d6c4c4ddbc 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -28,6 +28,7 @@
 
 #include "qemu/osdep.h"
 #include "nbd-client.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qemu/uri.h"
 #include "block/block_int.h"
diff --git a/block/nfs.c b/block/nfs.c
index 3349b67a76..3170b059b3 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -29,6 +29,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "trace.h"
 #include "qemu/iov.h"
 #include "qemu/option.h"
diff --git a/block/parallels.c b/block/parallels.c
index 6e9c37f44e..c1d9643498 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -31,6 +31,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
diff --git a/block/qcow.c b/block/qcow.c
index 1f866af0d3..8c08908fd8 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
diff --git a/block/qcow2.c b/block/qcow2.c
index 6fa5e1d71a..d2d955f984 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include <zlib.h>
diff --git a/block/qed.c b/block/qed.c
index 65cfe92393..324a953cbc 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qemu/timer.h"
 #include "qemu/bswap.h"
diff --git a/block/quorum.c b/block/quorum.c
index b6476c405a..9152da8c58 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -17,6 +17,7 @@
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-block.h"
 #include "qapi/qmp/qdict.h"
diff --git a/block/rbd.c b/block/rbd.c
index 40c6e4185f..9659c7361f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -18,6 +18,7 @@
 #include "qemu/error-report.h"
 #include "qemu/option.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "crypto/secret.h"
 #include "qemu/cutils.h"
 #include "qapi/qmp/qstring.h"
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 7b98725af7..2e1f0e6eca 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -24,6 +24,7 @@
 #include "qemu/option.h"
 #include "qemu/sockets.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/bitops.h"
 #include "qemu/cutils.h"
diff --git a/block/snapshot.c b/block/snapshot.c
index 2953d96c06..f9903bc94e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "block/snapshot.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
diff --git a/block/ssh.c b/block/ssh.c
index 4c4fa3ccfc..eec37dd27c 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -28,6 +28,7 @@
 #include <libssh2_sftp.h>
 
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/option.h"
diff --git a/block/vhdx.c b/block/vhdx.c
index 0831c5c5f4..2e32e24514 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -18,6 +18,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
diff --git a/block/vpc.c b/block/vpc.c
index 0ebfcd3cc8..41c8c980f1 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
diff --git a/block/vvfat.c b/block/vvfat.c
index 662dca0114..4595f335b8 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -27,6 +27,7 @@
 #include <dirent.h>
 #include "qapi/error.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/bswap.h"
diff --git a/block/vxhs.c b/block/vxhs.c
index 339e23218d..0cb0a007e9 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -12,6 +12,7 @@
 #include <qnio/qnio_api.h>
 #include <sys/param.h>
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
diff --git a/blockdev.c b/blockdev.c
index 8de95be8f4..721dc9a4fc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -35,6 +35,7 @@
 #include "sysemu/blockdev.h"
 #include "hw/block/block.h"
 #include "block/blockjob.h"
+#include "block/qdict.h"
 #include "block/throttle-groups.h"
 #include "monitor/monitor.h"
 #include "qemu/error-report.h"
diff --git a/include/block/qdict.h b/include/block/qdict.h
new file mode 100644
index 0000000000..71c037afba
--- /dev/null
+++ b/include/block/qdict.h
@@ -0,0 +1,32 @@
+/*
+ * Special QDict functions used by the block layer
+ *
+ * Copyright (c) 2013-2018 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef BLOCK_QDICT_H
+#define BLOCK_QDICT_H
+
+#include "qapi/qmp/qdict.h"
+
+void qdict_copy_default(QDict *dst, QDict *src, const char *key);
+void qdict_set_default_str(QDict *dst, const char *key, const char *val);
+
+void qdict_join(QDict *dest, QDict *src, bool overwrite);
+
+void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
+void qdict_array_split(QDict *src, QList **dst);
+int qdict_array_entries(QDict *src, const char *subqdict);
+QObject *qdict_crumple(const QDict *src, Error **errp);
+void qdict_flatten(QDict *qdict);
+
+typedef struct QDictRenames {
+    const char *from;
+    const char *to;
+} QDictRenames;
+bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp);
+
+#endif
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 921a28d2d3..7f3ec10a10 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -67,23 +67,6 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
 bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
 const char *qdict_get_try_str(const QDict *qdict, const char *key);
 
-void qdict_copy_default(QDict *dst, QDict *src, const char *key);
-void qdict_set_default_str(QDict *dst, const char *key, const char *val);
-
 QDict *qdict_clone_shallow(const QDict *src);
-void qdict_flatten(QDict *qdict);
-
-void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
-void qdict_array_split(QDict *src, QList **dst);
-int qdict_array_entries(QDict *src, const char *subqdict);
-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 */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 22800eeceb..0554c64553 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "block/qdict.h"
 #include "qapi/qmp/qnum.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qbool.h"
diff --git a/tests/check-qdict.c b/tests/check-qdict.c
index eba5d3528e..93e2112b6d 100644
--- a/tests/check-qdict.c
+++ b/tests/check-qdict.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "block/qdict.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qnum.h"
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
index 5cb08fcb63..16ccbde82c 100644
--- a/tests/check-qobject.c
+++ b/tests/check-qobject.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "block/qdict.h"
 #include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
diff --git a/tests/test-replication.c b/tests/test-replication.c
index 68c0d04f2a..c8165ae954 100644
--- a/tests/test-replication.c
+++ b/tests/test-replication.c
@@ -15,6 +15,7 @@
 #include "qemu/option.h"
 #include "replication.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 
 #define IMG_SIZE (64 * 1024 * 1024)
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 14d84022dc..9d2e278e29 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "block/qdict.h" /* for qdict_extract_subqdict() */
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qmp/qdict.h"
-- 
2.17.1


Re: [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header
Posted by Kevin Wolf, 1 week ago
Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> From: Max Reitz <mreitz@redhat.com>
> 
> There are numerous QDict functions that have been introduced for and are
> used only by the block layer.  Move their declarations into an own
> header file to reflect that.
> 
> While qdict_extract_subqdict() is in fact used outside of the block
> layer (in util/qemu-config.c), it is still a function related very
> closely to how the block layer works with nested QDicts, namely by
> sometimes flattening them.  Therefore, its declaration is put into this
> header as well and util/qemu-config.c includes it with a comment stating
> exactly which function it needs.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Message-Id: <20180509165530.29561-7-mreitz@redhat.com>
> [Copright note tweaked, superfluous includes dropped]

s/Copright/Copyright/

Kevin

Re: [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header
Posted by Markus Armbruster, 1 week ago
Kevin Wolf <kwolf@redhat.com> writes:

> Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
>> From: Max Reitz <mreitz@redhat.com>
>> 
>> There are numerous QDict functions that have been introduced for and are
>> used only by the block layer.  Move their declarations into an own
>> header file to reflect that.
>> 
>> While qdict_extract_subqdict() is in fact used outside of the block
>> layer (in util/qemu-config.c), it is still a function related very
>> closely to how the block layer works with nested QDicts, namely by
>> sometimes flattening them.  Therefore, its declaration is put into this
>> header as well and util/qemu-config.c includes it with a comment stating
>> exactly which function it needs.
>> 
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Message-Id: <20180509165530.29561-7-mreitz@redhat.com>
>> [Copright note tweaked, superfluous includes dropped]
>
> s/Copright/Copyright/

Fixing, thanks!

[Qemu-devel] [PATCH 04/18] qobject: Move block-specific qdict code to block-qdict.c
Posted by Markus Armbruster, 1 week ago
Pure code motion, except for two brace placements and a comment
tweaked to appease checkpatch.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 MAINTAINERS               |   2 +
 qobject/Makefile.objs     |   1 +
 qobject/block-qdict.c     | 640 +++++++++++++++++++++++++++++++++++++
 qobject/qdict.c           | 629 ------------------------------------
 tests/Makefile.include    |   4 +
 tests/check-block-qdict.c | 655 ++++++++++++++++++++++++++++++++++++++
 tests/check-qdict.c       | 642 -------------------------------------
 7 files changed, 1302 insertions(+), 1271 deletions(-)
 create mode 100644 qobject/block-qdict.c
 create mode 100644 tests/check-block-qdict.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a40f558694..f44ef871d0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1366,6 +1366,8 @@ F: qemu-img*
 F: qemu-io*
 F: tests/qemu-iotests/
 F: util/qemu-progress.c
+F: qobject/block-qdict.c
+F: test/check-block-qdict.c
 T: git git://repo.or.cz/qemu/kevin.git block
 
 Block I/O path
diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
index 002d25873a..7b12c9cacf 100644
--- a/qobject/Makefile.objs
+++ b/qobject/Makefile.objs
@@ -1,2 +1,3 @@
 util-obj-y = qnull.o qnum.o qstring.o qdict.o qlist.o qbool.o qlit.o
 util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o
+util-obj-y += block-qdict.o
diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
new file mode 100644
index 0000000000..fb92010dc5
--- /dev/null
+++ b/qobject/block-qdict.c
@@ -0,0 +1,640 @@
+/*
+ * Special QDict functions used by the block layer
+ *
+ * Copyright (c) 2013-2018 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "block/qdict.h"
+#include "qapi/qmp/qlist.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+
+/**
+ * qdict_copy_default(): If no entry mapped by 'key' exists in 'dst' yet, the
+ * value of 'key' in 'src' is copied there (and the refcount increased
+ * accordingly).
+ */
+void qdict_copy_default(QDict *dst, QDict *src, const char *key)
+{
+    QObject *val;
+
+    if (qdict_haskey(dst, key)) {
+        return;
+    }
+
+    val = qdict_get(src, key);
+    if (val) {
+        qdict_put_obj(dst, key, qobject_ref(val));
+    }
+}
+
+/**
+ * qdict_set_default_str(): If no entry mapped by 'key' exists in 'dst' yet, a
+ * new QString initialised by 'val' is put there.
+ */
+void qdict_set_default_str(QDict *dst, const char *key, const char *val)
+{
+    if (qdict_haskey(dst, key)) {
+        return;
+    }
+
+    qdict_put_str(dst, key, val);
+}
+
+static void qdict_flatten_qdict(QDict *qdict, QDict *target,
+                                const char *prefix);
+
+static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
+{
+    QObject *value;
+    const QListEntry *entry;
+    char *new_key;
+    int i;
+
+    /* This function is never called with prefix == NULL, i.e., it is always
+     * called from within qdict_flatten_q(list|dict)(). Therefore, it does not
+     * need to remove list entries during the iteration (the whole list will be
+     * deleted eventually anyway from qdict_flatten_qdict()). */
+    assert(prefix);
+
+    entry = qlist_first(qlist);
+
+    for (i = 0; entry; entry = qlist_next(entry), i++) {
+        value = qlist_entry_obj(entry);
+        new_key = g_strdup_printf("%s.%i", prefix, i);
+
+        if (qobject_type(value) == QTYPE_QDICT) {
+            qdict_flatten_qdict(qobject_to(QDict, value), target, new_key);
+        } else if (qobject_type(value) == QTYPE_QLIST) {
+            qdict_flatten_qlist(qobject_to(QList, value), target, new_key);
+        } else {
+            /* All other types are moved to the target unchanged. */
+            qdict_put_obj(target, new_key, qobject_ref(value));
+        }
+
+        g_free(new_key);
+    }
+}
+
+static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
+{
+    QObject *value;
+    const QDictEntry *entry, *next;
+    char *new_key;
+    bool delete;
+
+    entry = qdict_first(qdict);
+
+    while (entry != NULL) {
+
+        next = qdict_next(qdict, entry);
+        value = qdict_entry_value(entry);
+        new_key = NULL;
+        delete = false;
+
+        if (prefix) {
+            new_key = g_strdup_printf("%s.%s", prefix, entry->key);
+        }
+
+        if (qobject_type(value) == QTYPE_QDICT) {
+            /* Entries of QDicts are processed recursively, the QDict object
+             * itself disappears. */
+            qdict_flatten_qdict(qobject_to(QDict, value), target,
+                                new_key ? new_key : entry->key);
+            delete = true;
+        } else if (qobject_type(value) == QTYPE_QLIST) {
+            qdict_flatten_qlist(qobject_to(QList, value), target,
+                                new_key ? new_key : entry->key);
+            delete = true;
+        } else if (prefix) {
+            /* All other objects are moved to the target unchanged. */
+            qdict_put_obj(target, new_key, qobject_ref(value));
+            delete = true;
+        }
+
+        g_free(new_key);
+
+        if (delete) {
+            qdict_del(qdict, entry->key);
+
+            /* Restart loop after modifying the iterated QDict */
+            entry = qdict_first(qdict);
+            continue;
+        }
+
+        entry = next;
+    }
+}
+
+/**
+ * qdict_flatten(): For each nested QDict with key x, all fields with key y
+ * are moved to this QDict and their key is renamed to "x.y". For each nested
+ * QList with key x, the field at index y is moved to this QDict with the key
+ * "x.y" (i.e., the reverse of what qdict_array_split() does).
+ * This operation is applied recursively for nested QDicts and QLists.
+ */
+void qdict_flatten(QDict *qdict)
+{
+    qdict_flatten_qdict(qdict, qdict, NULL);
+}
+
+/* extract all the src QDict entries starting by start into dst */
+void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start)
+
+{
+    const QDictEntry *entry, *next;
+    const char *p;
+
+    *dst = qdict_new();
+    entry = qdict_first(src);
+
+    while (entry != NULL) {
+        next = qdict_next(src, entry);
+        if (strstart(entry->key, start, &p)) {
+            qdict_put_obj(*dst, p, qobject_ref(entry->value));
+            qdict_del(src, entry->key);
+        }
+        entry = next;
+    }
+}
+
+static int qdict_count_prefixed_entries(const QDict *src, const char *start)
+{
+    const QDictEntry *entry;
+    int count = 0;
+
+    for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) {
+        if (strstart(entry->key, start, NULL)) {
+            if (count == INT_MAX) {
+                return -ERANGE;
+            }
+            count++;
+        }
+    }
+
+    return count;
+}
+
+/**
+ * qdict_array_split(): This function moves array-like elements of a QDict into
+ * a new QList. Every entry in the original QDict with a key "%u" or one
+ * prefixed "%u.", where %u designates an unsigned integer starting at 0 and
+ * incrementally counting up, will be moved to a new QDict at index %u in the
+ * output QList with the key prefix removed, if that prefix is "%u.". If the
+ * whole key is just "%u", the whole QObject will be moved unchanged without
+ * creating a new QDict. The function terminates when there is no entry in the
+ * QDict with a prefix directly (incrementally) following the last one; it also
+ * returns if there are both entries with "%u" and "%u." for the same index %u.
+ * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "4.y": 1, "o.o": 7, "2": 66}
+ *      (or {"1.x": 0, "4.y": 1, "0.a": 42, "o.o": 7, "0.b": 23, "2": 66})
+ *       => [{"a": 42, "b": 23}, {"x": 0}, 66]
+ *      and {"4.y": 1, "o.o": 7} (remainder of the old QDict)
+ */
+void qdict_array_split(QDict *src, QList **dst)
+{
+    unsigned i;
+
+    *dst = qlist_new();
+
+    for (i = 0; i < UINT_MAX; i++) {
+        QObject *subqobj;
+        bool is_subqdict;
+        QDict *subqdict;
+        char indexstr[32], prefix[32];
+        size_t snprintf_ret;
+
+        snprintf_ret = snprintf(indexstr, 32, "%u", i);
+        assert(snprintf_ret < 32);
+
+        subqobj = qdict_get(src, indexstr);
+
+        snprintf_ret = snprintf(prefix, 32, "%u.", i);
+        assert(snprintf_ret < 32);
+
+        /* Overflow is the same as positive non-zero results */
+        is_subqdict = qdict_count_prefixed_entries(src, prefix);
+
+        /*
+         * There may be either a single subordinate object (named
+         * "%u") or multiple objects (each with a key prefixed "%u."),
+         * but not both.
+         */
+        if (!subqobj == !is_subqdict) {
+            break;
+        }
+
+        if (is_subqdict) {
+            qdict_extract_subqdict(src, &subqdict, prefix);
+            assert(qdict_size(subqdict) > 0);
+        } else {
+            qobject_ref(subqobj);
+            qdict_del(src, indexstr);
+        }
+
+        qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
+    }
+}
+
+/**
+ * qdict_split_flat_key:
+ * @key: the key string to split
+ * @prefix: non-NULL pointer to hold extracted prefix
+ * @suffix: non-NULL pointer to remaining suffix
+ *
+ * Given a flattened key such as 'foo.0.bar', split it into two parts
+ * at the first '.' separator. Allows double dot ('..') to escape the
+ * normal separator.
+ *
+ * e.g.
+ *    'foo.0.bar' -> prefix='foo' and suffix='0.bar'
+ *    'foo..0.bar' -> prefix='foo.0' and suffix='bar'
+ *
+ * The '..' sequence will be unescaped in the returned 'prefix'
+ * string. The 'suffix' string will be left in escaped format, so it
+ * can be fed back into the qdict_split_flat_key() key as the input
+ * later.
+ *
+ * The caller is responsible for freeing the string returned in @prefix
+ * using g_free().
+ */
+static void qdict_split_flat_key(const char *key, char **prefix,
+                                 const char **suffix)
+{
+    const char *separator;
+    size_t i, j;
+
+    /* Find first '.' separator, but if there is a pair '..'
+     * that acts as an escape, so skip over '..' */
+    separator = NULL;
+    do {
+        if (separator) {
+            separator += 2;
+        } else {
+            separator = key;
+        }
+        separator = strchr(separator, '.');
+    } while (separator && separator[1] == '.');
+
+    if (separator) {
+        *prefix = g_strndup(key, separator - key);
+        *suffix = separator + 1;
+    } else {
+        *prefix = g_strdup(key);
+        *suffix = NULL;
+    }
+
+    /* Unescape the '..' sequence into '.' */
+    for (i = 0, j = 0; (*prefix)[i] != '\0'; i++, j++) {
+        if ((*prefix)[i] == '.') {
+            assert((*prefix)[i + 1] == '.');
+            i++;
+        }
+        (*prefix)[j] = (*prefix)[i];
+    }
+    (*prefix)[j] = '\0';
+}
+
+/**
+ * qdict_is_list:
+ * @maybe_list: dict to check if keys represent list elements.
+ *
+ * Determine whether all keys in @maybe_list are valid list elements.
+ * If @maybe_list is non-zero in length and all the keys look like
+ * valid list indexes, this will return 1. If @maybe_list is zero
+ * length or all keys are non-numeric then it will return 0 to indicate
+ * it is a normal qdict. If there is a mix of numeric and non-numeric
+ * keys, or the list indexes are non-contiguous, an error is reported.
+ *
+ * Returns: 1 if a valid list, 0 if a dict, -1 on error
+ */
+static int qdict_is_list(QDict *maybe_list, Error **errp)
+{
+    const QDictEntry *ent;
+    ssize_t len = 0;
+    ssize_t max = -1;
+    int is_list = -1;
+    int64_t val;
+
+    for (ent = qdict_first(maybe_list); ent != NULL;
+         ent = qdict_next(maybe_list, ent)) {
+
+        if (qemu_strtoi64(ent->key, NULL, 10, &val) == 0) {
+            if (is_list == -1) {
+                is_list = 1;
+            } else if (!is_list) {
+                error_setg(errp,
+                           "Cannot mix list and non-list keys");
+                return -1;
+            }
+            len++;
+            if (val > max) {
+                max = val;
+            }
+        } else {
+            if (is_list == -1) {
+                is_list = 0;
+            } else if (is_list) {
+                error_setg(errp,
+                           "Cannot mix list and non-list keys");
+                return -1;
+            }
+        }
+    }
+
+    if (is_list == -1) {
+        assert(!qdict_size(maybe_list));
+        is_list = 0;
+    }
+
+    /* NB this isn't a perfect check - e.g. it won't catch
+     * a list containing '1', '+1', '01', '3', but that
+     * does not matter - we've still proved that the
+     * input is a list. It is up the caller to do a
+     * stricter check if desired */
+    if (len != (max + 1)) {
+        error_setg(errp, "List indices are not contiguous, "
+                   "saw %zd elements but %zd largest index",
+                   len, max);
+        return -1;
+    }
+
+    return is_list;
+}
+
+/**
+ * qdict_crumple:
+ * @src: the original flat dictionary (only scalar values) to crumple
+ *
+ * Takes a flat dictionary whose keys use '.' separator to indicate
+ * nesting, and values are scalars, and crumples it into a nested
+ * structure.
+ *
+ * To include a literal '.' in a key name, it must be escaped as '..'
+ *
+ * For example, an input of:
+ *
+ * { 'foo.0.bar': 'one', 'foo.0.wizz': '1',
+ *   'foo.1.bar': 'two', 'foo.1.wizz': '2' }
+ *
+ * will result in an output of:
+ *
+ * {
+ *   'foo': [
+ *      { 'bar': 'one', 'wizz': '1' },
+ *      { 'bar': 'two', 'wizz': '2' }
+ *   ],
+ * }
+ *
+ * The following scenarios in the input dict will result in an
+ * error being returned:
+ *
+ *  - Any values in @src are non-scalar types
+ *  - If keys in @src imply that a particular level is both a
+ *    list and a dict. e.g., "foo.0.bar" and "foo.eek.bar".
+ *  - If keys in @src imply that a particular level is a list,
+ *    but the indices are non-contiguous. e.g. "foo.0.bar" and
+ *    "foo.2.bar" without any "foo.1.bar" present.
+ *  - If keys in @src represent list indexes, but are not in
+ *    the "%zu" format. e.g. "foo.+0.bar"
+ *
+ * Returns: either a QDict or QList for the nested data structure, or NULL
+ * on error
+ */
+QObject *qdict_crumple(const QDict *src, Error **errp)
+{
+    const QDictEntry *ent;
+    QDict *two_level, *multi_level = NULL;
+    QObject *dst = NULL, *child;
+    size_t i;
+    char *prefix = NULL;
+    const char *suffix = NULL;
+    int is_list;
+
+    two_level = qdict_new();
+
+    /* Step 1: split our totally flat dict into a two level dict */
+    for (ent = qdict_first(src); ent != NULL; ent = qdict_next(src, ent)) {
+        if (qobject_type(ent->value) == QTYPE_QDICT ||
+            qobject_type(ent->value) == QTYPE_QLIST) {
+            error_setg(errp, "Value %s is not a scalar",
+                       ent->key);
+            goto error;
+        }
+
+        qdict_split_flat_key(ent->key, &prefix, &suffix);
+
+        child = qdict_get(two_level, prefix);
+        if (suffix) {
+            QDict *child_dict = qobject_to(QDict, child);
+            if (!child_dict) {
+                if (child) {
+                    error_setg(errp, "Key %s prefix is already set as a scalar",
+                               prefix);
+                    goto error;
+                }
+
+                child_dict = qdict_new();
+                qdict_put_obj(two_level, prefix, QOBJECT(child_dict));
+            }
+
+            qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
+        } else {
+            if (child) {
+                error_setg(errp, "Key %s prefix is already set as a dict",
+                           prefix);
+                goto error;
+            }
+            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
+        }
+
+        g_free(prefix);
+        prefix = NULL;
+    }
+
+    /* Step 2: optionally process the two level dict recursively
+     * into a multi-level dict */
+    multi_level = qdict_new();
+    for (ent = qdict_first(two_level); ent != NULL;
+         ent = qdict_next(two_level, ent)) {
+        QDict *dict = qobject_to(QDict, ent->value);
+        if (dict) {
+            child = qdict_crumple(dict, errp);
+            if (!child) {
+                goto error;
+            }
+
+            qdict_put_obj(multi_level, ent->key, child);
+        } else {
+            qdict_put_obj(multi_level, ent->key, qobject_ref(ent->value));
+        }
+    }
+    qobject_unref(two_level);
+    two_level = NULL;
+
+    /* Step 3: detect if we need to turn our dict into list */
+    is_list = qdict_is_list(multi_level, errp);
+    if (is_list < 0) {
+        goto error;
+    }
+
+    if (is_list) {
+        dst = QOBJECT(qlist_new());
+
+        for (i = 0; i < qdict_size(multi_level); i++) {
+            char *key = g_strdup_printf("%zu", i);
+
+            child = qdict_get(multi_level, key);
+            g_free(key);
+
+            if (!child) {
+                error_setg(errp, "Missing list index %zu", i);
+                goto error;
+            }
+
+            qlist_append_obj(qobject_to(QList, dst), qobject_ref(child));
+        }
+        qobject_unref(multi_level);
+        multi_level = NULL;
+    } else {
+        dst = QOBJECT(multi_level);
+    }
+
+    return dst;
+
+ error:
+    g_free(prefix);
+    qobject_unref(multi_level);
+    qobject_unref(two_level);
+    qobject_unref(dst);
+    return NULL;
+}
+
+/**
+ * qdict_array_entries(): Returns the number of direct array entries if the
+ * sub-QDict of src specified by the prefix in subqdict (or src itself for
+ * prefix == "") is valid as an array, i.e. the length of the created list if
+ * the sub-QDict would become empty after calling qdict_array_split() on it. If
+ * the array is not valid, -EINVAL is returned.
+ */
+int qdict_array_entries(QDict *src, const char *subqdict)
+{
+    const QDictEntry *entry;
+    unsigned i;
+    unsigned entries = 0;
+    size_t subqdict_len = strlen(subqdict);
+
+    assert(!subqdict_len || subqdict[subqdict_len - 1] == '.');
+
+    /* qdict_array_split() loops until UINT_MAX, but as we want to return
+     * negative errors, we only have a signed return value here. Any additional
+     * entries will lead to -EINVAL. */
+    for (i = 0; i < INT_MAX; i++) {
+        QObject *subqobj;
+        int subqdict_entries;
+        char *prefix = g_strdup_printf("%s%u.", subqdict, i);
+
+        subqdict_entries = qdict_count_prefixed_entries(src, prefix);
+
+        /* Remove ending "." */
+        prefix[strlen(prefix) - 1] = 0;
+        subqobj = qdict_get(src, prefix);
+
+        g_free(prefix);
+
+        if (subqdict_entries < 0) {
+            return subqdict_entries;
+        }
+
+        /* There may be either a single subordinate object (named "%u") or
+         * multiple objects (each with a key prefixed "%u."), but not both. */
+        if (subqobj && subqdict_entries) {
+            return -EINVAL;
+        } else if (!subqobj && !subqdict_entries) {
+            break;
+        }
+
+        entries += subqdict_entries ? subqdict_entries : 1;
+    }
+
+    /* Consider everything handled that isn't part of the given sub-QDict */
+    for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) {
+        if (!strstart(qdict_entry_key(entry), subqdict, NULL)) {
+            entries++;
+        }
+    }
+
+    /* Anything left in the sub-QDict that wasn't handled? */
+    if (qdict_size(src) != entries) {
+        return -EINVAL;
+    }
+
+    return i;
+}
+
+/**
+ * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all
+ * elements from src to dest.
+ *
+ * If an element from src has a key already present in dest, it will not be
+ * moved unless overwrite is true.
+ *
+ * If overwrite is true, the conflicting values in dest will be discarded and
+ * replaced by the corresponding values from src.
+ *
+ * Therefore, with overwrite being true, the src QDict will always be empty when
+ * this function returns. If overwrite is false, the src QDict will be empty
+ * iff there were no conflicts.
+ */
+void qdict_join(QDict *dest, QDict *src, bool overwrite)
+{
+    const QDictEntry *entry, *next;
+
+    entry = qdict_first(src);
+    while (entry) {
+        next = qdict_next(src, entry);
+
+        if (overwrite || !qdict_haskey(dest, entry->key)) {
+            qdict_put_obj(dest, entry->key, qobject_ref(entry->value));
+            qdict_del(src, entry->key);
+        }
+
+        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);
+            qdict_put_obj(qdict, renames->to, qobject_ref(qobj));
+            qdict_del(qdict, renames->from);
+        }
+
+        renames++;
+    }
+    return true;
+}
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 0554c64553..3d8c2f7bbc 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -11,17 +11,11 @@
  */
 
 #include "qemu/osdep.h"
-#include "block/qdict.h"
 #include "qapi/qmp/qnum.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qbool.h"
-#include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qnull.h"
 #include "qapi/qmp/qstring.h"
-#include "qapi/error.h"
-#include "qemu/queue.h"
-#include "qemu-common.h"
-#include "qemu/cutils.h"
 
 /**
  * qdict_new(): Create a new QDict
@@ -464,626 +458,3 @@ void qdict_destroy_obj(QObject *obj)
 
     g_free(qdict);
 }
-
-/**
- * qdict_copy_default(): If no entry mapped by 'key' exists in 'dst' yet, the
- * value of 'key' in 'src' is copied there (and the refcount increased
- * accordingly).
- */
-void qdict_copy_default(QDict *dst, QDict *src, const char *key)
-{
-    QObject *val;
-
-    if (qdict_haskey(dst, key)) {
-        return;
-    }
-
-    val = qdict_get(src, key);
-    if (val) {
-        qdict_put_obj(dst, key, qobject_ref(val));
-    }
-}
-
-/**
- * qdict_set_default_str(): If no entry mapped by 'key' exists in 'dst' yet, a
- * new QString initialised by 'val' is put there.
- */
-void qdict_set_default_str(QDict *dst, const char *key, const char *val)
-{
-    if (qdict_haskey(dst, key)) {
-        return;
-    }
-
-    qdict_put_str(dst, key, val);
-}
-
-static void qdict_flatten_qdict(QDict *qdict, QDict *target,
-                                const char *prefix);
-
-static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
-{
-    QObject *value;
-    const QListEntry *entry;
-    char *new_key;
-    int i;
-
-    /* This function is never called with prefix == NULL, i.e., it is always
-     * called from within qdict_flatten_q(list|dict)(). Therefore, it does not
-     * need to remove list entries during the iteration (the whole list will be
-     * deleted eventually anyway from qdict_flatten_qdict()). */
-    assert(prefix);
-
-    entry = qlist_first(qlist);
-
-    for (i = 0; entry; entry = qlist_next(entry), i++) {
-        value = qlist_entry_obj(entry);
-        new_key = g_strdup_printf("%s.%i", prefix, i);
-
-        if (qobject_type(value) == QTYPE_QDICT) {
-            qdict_flatten_qdict(qobject_to(QDict, value), target, new_key);
-        } else if (qobject_type(value) == QTYPE_QLIST) {
-            qdict_flatten_qlist(qobject_to(QList, value), target, new_key);
-        } else {
-            /* All other types are moved to the target unchanged. */
-            qdict_put_obj(target, new_key, qobject_ref(value));
-        }
-
-        g_free(new_key);
-    }
-}
-
-static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
-{
-    QObject *value;
-    const QDictEntry *entry, *next;
-    char *new_key;
-    bool delete;
-
-    entry = qdict_first(qdict);
-
-    while (entry != NULL) {
-
-        next = qdict_next(qdict, entry);
-        value = qdict_entry_value(entry);
-        new_key = NULL;
-        delete = false;
-
-        if (prefix) {
-            new_key = g_strdup_printf("%s.%s", prefix, entry->key);
-        }
-
-        if (qobject_type(value) == QTYPE_QDICT) {
-            /* Entries of QDicts are processed recursively, the QDict object
-             * itself disappears. */
-            qdict_flatten_qdict(qobject_to(QDict, value), target,
-                                new_key ? new_key : entry->key);
-            delete = true;
-        } else if (qobject_type(value) == QTYPE_QLIST) {
-            qdict_flatten_qlist(qobject_to(QList, value), target,
-                                new_key ? new_key : entry->key);
-            delete = true;
-        } else if (prefix) {
-            /* All other objects are moved to the target unchanged. */
-            qdict_put_obj(target, new_key, qobject_ref(value));
-            delete = true;
-        }
-
-        g_free(new_key);
-
-        if (delete) {
-            qdict_del(qdict, entry->key);
-
-            /* Restart loop after modifying the iterated QDict */
-            entry = qdict_first(qdict);
-            continue;
-        }
-
-        entry = next;
-    }
-}
-
-/**
- * qdict_flatten(): For each nested QDict with key x, all fields with key y
- * are moved to this QDict and their key is renamed to "x.y". For each nested
- * QList with key x, the field at index y is moved to this QDict with the key
- * "x.y" (i.e., the reverse of what qdict_array_split() does).
- * This operation is applied recursively for nested QDicts and QLists.
- */
-void qdict_flatten(QDict *qdict)
-{
-    qdict_flatten_qdict(qdict, qdict, NULL);
-}
-
-/* extract all the src QDict entries starting by start into dst */
-void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start)
-
-{
-    const QDictEntry *entry, *next;
-    const char *p;
-
-    *dst = qdict_new();
-    entry = qdict_first(src);
-
-    while (entry != NULL) {
-        next = qdict_next(src, entry);
-        if (strstart(entry->key, start, &p)) {
-            qdict_put_obj(*dst, p, qobject_ref(entry->value));
-            qdict_del(src, entry->key);
-        }
-        entry = next;
-    }
-}
-
-static int qdict_count_prefixed_entries(const QDict *src, const char *start)
-{
-    const QDictEntry *entry;
-    int count = 0;
-
-    for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) {
-        if (strstart(entry->key, start, NULL)) {
-            if (count == INT_MAX) {
-                return -ERANGE;
-            }
-            count++;
-        }
-    }
-
-    return count;
-}
-
-/**
- * qdict_array_split(): This function moves array-like elements of a QDict into
- * a new QList. Every entry in the original QDict with a key "%u" or one
- * prefixed "%u.", where %u designates an unsigned integer starting at 0 and
- * incrementally counting up, will be moved to a new QDict at index %u in the
- * output QList with the key prefix removed, if that prefix is "%u.". If the
- * whole key is just "%u", the whole QObject will be moved unchanged without
- * creating a new QDict. The function terminates when there is no entry in the
- * QDict with a prefix directly (incrementally) following the last one; it also
- * returns if there are both entries with "%u" and "%u." for the same index %u.
- * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "4.y": 1, "o.o": 7, "2": 66}
- *      (or {"1.x": 0, "4.y": 1, "0.a": 42, "o.o": 7, "0.b": 23, "2": 66})
- *       => [{"a": 42, "b": 23}, {"x": 0}, 66]
- *      and {"4.y": 1, "o.o": 7} (remainder of the old QDict)
- */
-void qdict_array_split(QDict *src, QList **dst)
-{
-    unsigned i;
-
-    *dst = qlist_new();
-
-    for (i = 0; i < UINT_MAX; i++) {
-        QObject *subqobj;
-        bool is_subqdict;
-        QDict *subqdict;
-        char indexstr[32], prefix[32];
-        size_t snprintf_ret;
-
-        snprintf_ret = snprintf(indexstr, 32, "%u", i);
-        assert(snprintf_ret < 32);
-
-        subqobj = qdict_get(src, indexstr);
-
-        snprintf_ret = snprintf(prefix, 32, "%u.", i);
-        assert(snprintf_ret < 32);
-
-        /* Overflow is the same as positive non-zero results */
-        is_subqdict = qdict_count_prefixed_entries(src, prefix);
-
-        // There may be either a single subordinate object (named "%u") or
-        // multiple objects (each with a key prefixed "%u."), but not both.
-        if (!subqobj == !is_subqdict) {
-            break;
-        }
-
-        if (is_subqdict) {
-            qdict_extract_subqdict(src, &subqdict, prefix);
-            assert(qdict_size(subqdict) > 0);
-        } else {
-            qobject_ref(subqobj);
-            qdict_del(src, indexstr);
-        }
-
-        qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
-    }
-}
-
-/**
- * qdict_split_flat_key:
- * @key: the key string to split
- * @prefix: non-NULL pointer to hold extracted prefix
- * @suffix: non-NULL pointer to remaining suffix
- *
- * Given a flattened key such as 'foo.0.bar', split it into two parts
- * at the first '.' separator. Allows double dot ('..') to escape the
- * normal separator.
- *
- * e.g.
- *    'foo.0.bar' -> prefix='foo' and suffix='0.bar'
- *    'foo..0.bar' -> prefix='foo.0' and suffix='bar'
- *
- * The '..' sequence will be unescaped in the returned 'prefix'
- * string. The 'suffix' string will be left in escaped format, so it
- * can be fed back into the qdict_split_flat_key() key as the input
- * later.
- *
- * The caller is responsible for freeing the string returned in @prefix
- * using g_free().
- */
-static void qdict_split_flat_key(const char *key, char **prefix,
-                                 const char **suffix)
-{
-    const char *separator;
-    size_t i, j;
-
-    /* Find first '.' separator, but if there is a pair '..'
-     * that acts as an escape, so skip over '..' */
-    separator = NULL;
-    do {
-        if (separator) {
-            separator += 2;
-        } else {
-            separator = key;
-        }
-        separator = strchr(separator, '.');
-    } while (separator && separator[1] == '.');
-
-    if (separator) {
-        *prefix = g_strndup(key, separator - key);
-        *suffix = separator + 1;
-    } else {
-        *prefix = g_strdup(key);
-        *suffix = NULL;
-    }
-
-    /* Unescape the '..' sequence into '.' */
-    for (i = 0, j = 0; (*prefix)[i] != '\0'; i++, j++) {
-        if ((*prefix)[i] == '.') {
-            assert((*prefix)[i + 1] == '.');
-            i++;
-        }
-        (*prefix)[j] = (*prefix)[i];
-    }
-    (*prefix)[j] = '\0';
-}
-
-/**
- * qdict_is_list:
- * @maybe_list: dict to check if keys represent list elements.
- *
- * Determine whether all keys in @maybe_list are valid list elements.
- * If @maybe_list is non-zero in length and all the keys look like
- * valid list indexes, this will return 1. If @maybe_list is zero
- * length or all keys are non-numeric then it will return 0 to indicate
- * it is a normal qdict. If there is a mix of numeric and non-numeric
- * keys, or the list indexes are non-contiguous, an error is reported.
- *
- * Returns: 1 if a valid list, 0 if a dict, -1 on error
- */
-static int qdict_is_list(QDict *maybe_list, Error **errp)
-{
-    const QDictEntry *ent;
-    ssize_t len = 0;
-    ssize_t max = -1;
-    int is_list = -1;
-    int64_t val;
-
-    for (ent = qdict_first(maybe_list); ent != NULL;
-         ent = qdict_next(maybe_list, ent)) {
-
-        if (qemu_strtoi64(ent->key, NULL, 10, &val) == 0) {
-            if (is_list == -1) {
-                is_list = 1;
-            } else if (!is_list) {
-                error_setg(errp,
-                           "Cannot mix list and non-list keys");
-                return -1;
-            }
-            len++;
-            if (val > max) {
-                max = val;
-            }
-        } else {
-            if (is_list == -1) {
-                is_list = 0;
-            } else if (is_list) {
-                error_setg(errp,
-                           "Cannot mix list and non-list keys");
-                return -1;
-            }
-        }
-    }
-
-    if (is_list == -1) {
-        assert(!qdict_size(maybe_list));
-        is_list = 0;
-    }
-
-    /* NB this isn't a perfect check - e.g. it won't catch
-     * a list containing '1', '+1', '01', '3', but that
-     * does not matter - we've still proved that the
-     * input is a list. It is up the caller to do a
-     * stricter check if desired */
-    if (len != (max + 1)) {
-        error_setg(errp, "List indices are not contiguous, "
-                   "saw %zd elements but %zd largest index",
-                   len, max);
-        return -1;
-    }
-
-    return is_list;
-}
-
-/**
- * qdict_crumple:
- * @src: the original flat dictionary (only scalar values) to crumple
- *
- * Takes a flat dictionary whose keys use '.' separator to indicate
- * nesting, and values are scalars, and crumples it into a nested
- * structure.
- *
- * To include a literal '.' in a key name, it must be escaped as '..'
- *
- * For example, an input of:
- *
- * { 'foo.0.bar': 'one', 'foo.0.wizz': '1',
- *   'foo.1.bar': 'two', 'foo.1.wizz': '2' }
- *
- * will result in an output of:
- *
- * {
- *   'foo': [
- *      { 'bar': 'one', 'wizz': '1' },
- *      { 'bar': 'two', 'wizz': '2' }
- *   ],
- * }
- *
- * The following scenarios in the input dict will result in an
- * error being returned:
- *
- *  - Any values in @src are non-scalar types
- *  - If keys in @src imply that a particular level is both a
- *    list and a dict. e.g., "foo.0.bar" and "foo.eek.bar".
- *  - If keys in @src imply that a particular level is a list,
- *    but the indices are non-contiguous. e.g. "foo.0.bar" and
- *    "foo.2.bar" without any "foo.1.bar" present.
- *  - If keys in @src represent list indexes, but are not in
- *    the "%zu" format. e.g. "foo.+0.bar"
- *
- * Returns: either a QDict or QList for the nested data structure, or NULL
- * on error
- */
-QObject *qdict_crumple(const QDict *src, Error **errp)
-{
-    const QDictEntry *ent;
-    QDict *two_level, *multi_level = NULL;
-    QObject *dst = NULL, *child;
-    size_t i;
-    char *prefix = NULL;
-    const char *suffix = NULL;
-    int is_list;
-
-    two_level = qdict_new();
-
-    /* Step 1: split our totally flat dict into a two level dict */
-    for (ent = qdict_first(src); ent != NULL; ent = qdict_next(src, ent)) {
-        if (qobject_type(ent->value) == QTYPE_QDICT ||
-            qobject_type(ent->value) == QTYPE_QLIST) {
-            error_setg(errp, "Value %s is not a scalar",
-                       ent->key);
-            goto error;
-        }
-
-        qdict_split_flat_key(ent->key, &prefix, &suffix);
-
-        child = qdict_get(two_level, prefix);
-        if (suffix) {
-            QDict *child_dict = qobject_to(QDict, child);
-            if (!child_dict) {
-                if (child) {
-                    error_setg(errp, "Key %s prefix is already set as a scalar",
-                               prefix);
-                    goto error;
-                }
-
-                child_dict = qdict_new();
-                qdict_put_obj(two_level, prefix, QOBJECT(child_dict));
-            }
-
-            qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
-        } else {
-            if (child) {
-                error_setg(errp, "Key %s prefix is already set as a dict",
-                           prefix);
-                goto error;
-            }
-            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
-        }
-
-        g_free(prefix);
-        prefix = NULL;
-    }
-
-    /* Step 2: optionally process the two level dict recursively
-     * into a multi-level dict */
-    multi_level = qdict_new();
-    for (ent = qdict_first(two_level); ent != NULL;
-         ent = qdict_next(two_level, ent)) {
-        QDict *dict = qobject_to(QDict, ent->value);
-        if (dict) {
-            child = qdict_crumple(dict, errp);
-            if (!child) {
-                goto error;
-            }
-
-            qdict_put_obj(multi_level, ent->key, child);
-        } else {
-            qdict_put_obj(multi_level, ent->key, qobject_ref(ent->value));
-        }
-    }
-    qobject_unref(two_level);
-    two_level = NULL;
-
-    /* Step 3: detect if we need to turn our dict into list */
-    is_list = qdict_is_list(multi_level, errp);
-    if (is_list < 0) {
-        goto error;
-    }
-
-    if (is_list) {
-        dst = QOBJECT(qlist_new());
-
-        for (i = 0; i < qdict_size(multi_level); i++) {
-            char *key = g_strdup_printf("%zu", i);
-
-            child = qdict_get(multi_level, key);
-            g_free(key);
-
-            if (!child) {
-                error_setg(errp, "Missing list index %zu", i);
-                goto error;
-            }
-
-            qlist_append_obj(qobject_to(QList, dst), qobject_ref(child));
-        }
-        qobject_unref(multi_level);
-        multi_level = NULL;
-    } else {
-        dst = QOBJECT(multi_level);
-    }
-
-    return dst;
-
- error:
-    g_free(prefix);
-    qobject_unref(multi_level);
-    qobject_unref(two_level);
-    qobject_unref(dst);
-    return NULL;
-}
-
-/**
- * qdict_array_entries(): Returns the number of direct array entries if the
- * sub-QDict of src specified by the prefix in subqdict (or src itself for
- * prefix == "") is valid as an array, i.e. the length of the created list if
- * the sub-QDict would become empty after calling qdict_array_split() on it. If
- * the array is not valid, -EINVAL is returned.
- */
-int qdict_array_entries(QDict *src, const char *subqdict)
-{
-    const QDictEntry *entry;
-    unsigned i;
-    unsigned entries = 0;
-    size_t subqdict_len = strlen(subqdict);
-
-    assert(!subqdict_len || subqdict[subqdict_len - 1] == '.');
-
-    /* qdict_array_split() loops until UINT_MAX, but as we want to return
-     * negative errors, we only have a signed return value here. Any additional
-     * entries will lead to -EINVAL. */
-    for (i = 0; i < INT_MAX; i++) {
-        QObject *subqobj;
-        int subqdict_entries;
-        char *prefix = g_strdup_printf("%s%u.", subqdict, i);
-
-        subqdict_entries = qdict_count_prefixed_entries(src, prefix);
-
-        /* Remove ending "." */
-        prefix[strlen(prefix) - 1] = 0;
-        subqobj = qdict_get(src, prefix);
-
-        g_free(prefix);
-
-        if (subqdict_entries < 0) {
-            return subqdict_entries;
-        }
-
-        /* There may be either a single subordinate object (named "%u") or
-         * multiple objects (each with a key prefixed "%u."), but not both. */
-        if (subqobj && subqdict_entries) {
-            return -EINVAL;
-        } else if (!subqobj && !subqdict_entries) {
-            break;
-        }
-
-        entries += subqdict_entries ? subqdict_entries : 1;
-    }
-
-    /* Consider everything handled that isn't part of the given sub-QDict */
-    for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) {
-        if (!strstart(qdict_entry_key(entry), subqdict, NULL)) {
-            entries++;
-        }
-    }
-
-    /* Anything left in the sub-QDict that wasn't handled? */
-    if (qdict_size(src) != entries) {
-        return -EINVAL;
-    }
-
-    return i;
-}
-
-/**
- * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all
- * elements from src to dest.
- *
- * If an element from src has a key already present in dest, it will not be
- * moved unless overwrite is true.
- *
- * If overwrite is true, the conflicting values in dest will be discarded and
- * replaced by the corresponding values from src.
- *
- * Therefore, with overwrite being true, the src QDict will always be empty when
- * this function returns. If overwrite is false, the src QDict will be empty
- * iff there were no conflicts.
- */
-void qdict_join(QDict *dest, QDict *src, bool overwrite)
-{
-    const QDictEntry *entry, *next;
-
-    entry = qdict_first(src);
-    while (entry) {
-        next = qdict_next(src, entry);
-
-        if (overwrite || !qdict_haskey(dest, entry->key)) {
-            qdict_put_obj(dest, entry->key, qobject_ref(entry->value));
-            qdict_del(src, entry->key);
-        }
-
-        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);
-            qdict_put_obj(qdict, renames->to, qobject_ref(qobj));
-            qdict_del(qdict, renames->from);
-        }
-
-        renames++;
-    }
-    return true;
-}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index d098a104bb..2023e257fd 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -40,6 +40,8 @@ SYSEMU_TARGET_LIST := $(subst -softmmu.mak,,$(notdir \
 
 check-unit-y = tests/check-qdict$(EXESUF)
 gcov-files-check-qdict-y = qobject/qdict.c
+check-unit-y = tests/check-block-qdict$(EXESUF)
+gcov-files-check-block-qdict-y = qobject/block-qdict.c
 check-unit-y += tests/test-char$(EXESUF)
 gcov-files-check-qdict-y = chardev/char.c
 check-unit-y += tests/check-qnum$(EXESUF)
@@ -585,6 +587,7 @@ GENERATED_FILES += tests/test-qapi-types.h tests/test-qapi-visit.h \
 test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
 	tests/check-qlist.o tests/check-qnull.o tests/check-qobject.o \
 	tests/check-qjson.o tests/check-qlit.o \
+	tests/check-block-qtest.o \
 	tests/test-coroutine.o tests/test-string-output-visitor.o \
 	tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
 	tests/test-clone-visitor.o \
@@ -615,6 +618,7 @@ test-block-obj-y = $(block-obj-y) $(test-io-obj-y) tests/iothread.o
 tests/check-qnum$(EXESUF): tests/check-qnum.o $(test-util-obj-y)
 tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y)
 tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
+tests/check-block-qdict$(EXESUF): tests/check-block-qdict.o $(test-util-obj-y)
 tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
 tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
 tests/check-qobject$(EXESUF): tests/check-qobject.o $(test-util-obj-y)
diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
new file mode 100644
index 0000000000..5b9f4d506e
--- /dev/null
+++ b/tests/check-block-qdict.c
@@ -0,0 +1,655 @@
+/*
+ * Unit-tests for Block layer QDict extras
+ *
+ * Copyright (c) 2013-2018 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "block/qdict.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnum.h"
+#include "qapi/error.h"
+
+static void qdict_defaults_test(void)
+{
+    QDict *dict, *copy;
+
+    dict = qdict_new();
+    copy = qdict_new();
+
+    qdict_set_default_str(dict, "foo", "abc");
+    qdict_set_default_str(dict, "foo", "def");
+    g_assert_cmpstr(qdict_get_str(dict, "foo"), ==, "abc");
+    qdict_set_default_str(dict, "bar", "ghi");
+
+    qdict_copy_default(copy, dict, "foo");
+    g_assert_cmpstr(qdict_get_str(copy, "foo"), ==, "abc");
+    qdict_set_default_str(copy, "bar", "xyz");
+    qdict_copy_default(copy, dict, "bar");
+    g_assert_cmpstr(qdict_get_str(copy, "bar"), ==, "xyz");
+
+    qobject_unref(copy);
+    qobject_unref(dict);
+}
+
+static void qdict_flatten_test(void)
+{
+    QList *list1 = qlist_new();
+    QList *list2 = qlist_new();
+    QDict *dict1 = qdict_new();
+    QDict *dict2 = qdict_new();
+    QDict *dict3 = qdict_new();
+
+    /*
+     * Test the flattening of
+     *
+     * {
+     *     "e": [
+     *         42,
+     *         [
+     *             23,
+     *             66,
+     *             {
+     *                 "a": 0,
+     *                 "b": 1
+     *             }
+     *         ]
+     *     ],
+     *     "f": {
+     *         "c": 2,
+     *         "d": 3,
+     *     },
+     *     "g": 4
+     * }
+     *
+     * to
+     *
+     * {
+     *     "e.0": 42,
+     *     "e.1.0": 23,
+     *     "e.1.1": 66,
+     *     "e.1.2.a": 0,
+     *     "e.1.2.b": 1,
+     *     "f.c": 2,
+     *     "f.d": 3,
+     *     "g": 4
+     * }
+     */
+
+    qdict_put_int(dict1, "a", 0);
+    qdict_put_int(dict1, "b", 1);
+
+    qlist_append_int(list1, 23);
+    qlist_append_int(list1, 66);
+    qlist_append(list1, dict1);
+    qlist_append_int(list2, 42);
+    qlist_append(list2, list1);
+
+    qdict_put_int(dict2, "c", 2);
+    qdict_put_int(dict2, "d", 3);
+    qdict_put(dict3, "e", list2);
+    qdict_put(dict3, "f", dict2);
+    qdict_put_int(dict3, "g", 4);
+
+    qdict_flatten(dict3);
+
+    g_assert(qdict_get_int(dict3, "e.0") == 42);
+    g_assert(qdict_get_int(dict3, "e.1.0") == 23);
+    g_assert(qdict_get_int(dict3, "e.1.1") == 66);
+    g_assert(qdict_get_int(dict3, "e.1.2.a") == 0);
+    g_assert(qdict_get_int(dict3, "e.1.2.b") == 1);
+    g_assert(qdict_get_int(dict3, "f.c") == 2);
+    g_assert(qdict_get_int(dict3, "f.d") == 3);
+    g_assert(qdict_get_int(dict3, "g") == 4);
+
+    g_assert(qdict_size(dict3) == 8);
+
+    qobject_unref(dict3);
+}
+
+static void qdict_array_split_test(void)
+{
+    QDict *test_dict = qdict_new();
+    QDict *dict1, *dict2;
+    QNum *int1;
+    QList *test_list;
+
+    /*
+     * Test the split of
+     *
+     * {
+     *     "1.x": 0,
+     *     "4.y": 1,
+     *     "0.a": 42,
+     *     "o.o": 7,
+     *     "0.b": 23,
+     *     "2": 66
+     * }
+     *
+     * to
+     *
+     * [
+     *     {
+     *         "a": 42,
+     *         "b": 23
+     *     },
+     *     {
+     *         "x": 0
+     *     },
+     *     66
+     * ]
+     *
+     * and
+     *
+     * {
+     *     "4.y": 1,
+     *     "o.o": 7
+     * }
+     *
+     * (remaining in the old QDict)
+     *
+     * This example is given in the comment of qdict_array_split().
+     */
+
+    qdict_put_int(test_dict, "1.x", 0);
+    qdict_put_int(test_dict, "4.y", 1);
+    qdict_put_int(test_dict, "0.a", 42);
+    qdict_put_int(test_dict, "o.o", 7);
+    qdict_put_int(test_dict, "0.b", 23);
+    qdict_put_int(test_dict, "2", 66);
+
+    qdict_array_split(test_dict, &test_list);
+
+    dict1 = qobject_to(QDict, qlist_pop(test_list));
+    dict2 = qobject_to(QDict, qlist_pop(test_list));
+    int1 = qobject_to(QNum, qlist_pop(test_list));
+
+    g_assert(dict1);
+    g_assert(dict2);
+    g_assert(int1);
+    g_assert(qlist_empty(test_list));
+
+    qobject_unref(test_list);
+
+    g_assert(qdict_get_int(dict1, "a") == 42);
+    g_assert(qdict_get_int(dict1, "b") == 23);
+
+    g_assert(qdict_size(dict1) == 2);
+
+    qobject_unref(dict1);
+
+    g_assert(qdict_get_int(dict2, "x") == 0);
+
+    g_assert(qdict_size(dict2) == 1);
+
+    qobject_unref(dict2);
+
+    g_assert_cmpint(qnum_get_int(int1), ==, 66);
+
+    qobject_unref(int1);
+
+    g_assert(qdict_get_int(test_dict, "4.y") == 1);
+    g_assert(qdict_get_int(test_dict, "o.o") == 7);
+
+    g_assert(qdict_size(test_dict) == 2);
+
+    qobject_unref(test_dict);
+
+    /*
+     * Test the split of
+     *
+     * {
+     *     "0": 42,
+     *     "1": 23,
+     *     "1.x": 84
+     * }
+     *
+     * to
+     *
+     * [
+     *     42
+     * ]
+     *
+     * and
+     *
+     * {
+     *     "1": 23,
+     *     "1.x": 84
+     * }
+     *
+     * That is, test whether splitting stops if there is both an entry with key
+     * of "%u" and other entries with keys prefixed "%u." for the same index.
+     */
+
+    test_dict = qdict_new();
+
+    qdict_put_int(test_dict, "0", 42);
+    qdict_put_int(test_dict, "1", 23);
+    qdict_put_int(test_dict, "1.x", 84);
+
+    qdict_array_split(test_dict, &test_list);
+
+    int1 = qobject_to(QNum, qlist_pop(test_list));
+
+    g_assert(int1);
+    g_assert(qlist_empty(test_list));
+
+    qobject_unref(test_list);
+
+    g_assert_cmpint(qnum_get_int(int1), ==, 42);
+
+    qobject_unref(int1);
+
+    g_assert(qdict_get_int(test_dict, "1") == 23);
+    g_assert(qdict_get_int(test_dict, "1.x") == 84);
+
+    g_assert(qdict_size(test_dict) == 2);
+
+    qobject_unref(test_dict);
+}
+
+static void qdict_array_entries_test(void)
+{
+    QDict *dict = qdict_new();
+
+    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 0);
+
+    qdict_put_int(dict, "bar", 0);
+    qdict_put_int(dict, "baz.0", 0);
+    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 0);
+
+    qdict_put_int(dict, "foo.1", 0);
+    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, -EINVAL);
+    qdict_put_int(dict, "foo.0", 0);
+    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 2);
+    qdict_put_int(dict, "foo.bar", 0);
+    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, -EINVAL);
+    qdict_del(dict, "foo.bar");
+
+    qdict_put_int(dict, "foo.2.a", 0);
+    qdict_put_int(dict, "foo.2.b", 0);
+    qdict_put_int(dict, "foo.2.c", 0);
+    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 3);
+    g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL);
+
+    qobject_unref(dict);
+
+    dict = qdict_new();
+    qdict_put_int(dict, "1", 0);
+    g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL);
+    qdict_put_int(dict, "0", 0);
+    g_assert_cmpint(qdict_array_entries(dict, ""), ==, 2);
+    qdict_put_int(dict, "bar", 0);
+    g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL);
+    qdict_del(dict, "bar");
+
+    qdict_put_int(dict, "2.a", 0);
+    qdict_put_int(dict, "2.b", 0);
+    qdict_put_int(dict, "2.c", 0);
+    g_assert_cmpint(qdict_array_entries(dict, ""), ==, 3);
+
+    qobject_unref(dict);
+}
+
+static void qdict_join_test(void)
+{
+    QDict *dict1, *dict2;
+    bool overwrite = false;
+    int i;
+
+    dict1 = qdict_new();
+    dict2 = qdict_new();
+
+    /* Test everything once without overwrite and once with */
+    do {
+        /* Test empty dicts */
+        qdict_join(dict1, dict2, overwrite);
+
+        g_assert(qdict_size(dict1) == 0);
+        g_assert(qdict_size(dict2) == 0);
+
+        /* First iteration: Test movement */
+        /* Second iteration: Test empty source and non-empty destination */
+        qdict_put_int(dict2, "foo", 42);
+
+        for (i = 0; i < 2; i++) {
+            qdict_join(dict1, dict2, overwrite);
+
+            g_assert(qdict_size(dict1) == 1);
+            g_assert(qdict_size(dict2) == 0);
+
+            g_assert(qdict_get_int(dict1, "foo") == 42);
+        }
+
+        /* Test non-empty source and destination without conflict */
+        qdict_put_int(dict2, "bar", 23);
+
+        qdict_join(dict1, dict2, overwrite);
+
+        g_assert(qdict_size(dict1) == 2);
+        g_assert(qdict_size(dict2) == 0);
+
+        g_assert(qdict_get_int(dict1, "foo") == 42);
+        g_assert(qdict_get_int(dict1, "bar") == 23);
+
+        /* Test conflict */
+        qdict_put_int(dict2, "foo", 84);
+
+        qdict_join(dict1, dict2, overwrite);
+
+        g_assert(qdict_size(dict1) == 2);
+        g_assert(qdict_size(dict2) == !overwrite);
+
+        g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42));
+        g_assert(qdict_get_int(dict1, "bar") == 23);
+
+        if (!overwrite) {
+            g_assert(qdict_get_int(dict2, "foo") == 84);
+        }
+
+        /* Check the references */
+        g_assert(qdict_get(dict1, "foo")->base.refcnt == 1);
+        g_assert(qdict_get(dict1, "bar")->base.refcnt == 1);
+
+        if (!overwrite) {
+            g_assert(qdict_get(dict2, "foo")->base.refcnt == 1);
+        }
+
+        /* Clean up */
+        qdict_del(dict1, "foo");
+        qdict_del(dict1, "bar");
+
+        if (!overwrite) {
+            qdict_del(dict2, "foo");
+        }
+    } while (overwrite ^= true);
+
+    qobject_unref(dict1);
+    qobject_unref(dict2);
+}
+
+static void qdict_crumple_test_recursive(void)
+{
+    QDict *src, *dst, *rule, *vnc, *acl, *listen;
+    QList *rules;
+
+    src = qdict_new();
+    qdict_put_str(src, "vnc.listen.addr", "127.0.0.1");
+    qdict_put_str(src, "vnc.listen.port", "5901");
+    qdict_put_str(src, "vnc.acl.rules.0.match", "fred");
+    qdict_put_str(src, "vnc.acl.rules.0.policy", "allow");
+    qdict_put_str(src, "vnc.acl.rules.1.match", "bob");
+    qdict_put_str(src, "vnc.acl.rules.1.policy", "deny");
+    qdict_put_str(src, "vnc.acl.default", "deny");
+    qdict_put_str(src, "vnc.acl..name", "acl0");
+    qdict_put_str(src, "vnc.acl.rule..name", "acl0");
+
+    dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
+    g_assert(dst);
+    g_assert_cmpint(qdict_size(dst), ==, 1);
+
+    vnc = qdict_get_qdict(dst, "vnc");
+    g_assert(vnc);
+    g_assert_cmpint(qdict_size(vnc), ==, 3);
+
+    listen = qdict_get_qdict(vnc, "listen");
+    g_assert(listen);
+    g_assert_cmpint(qdict_size(listen), ==, 2);
+    g_assert_cmpstr("127.0.0.1", ==, qdict_get_str(listen, "addr"));
+    g_assert_cmpstr("5901", ==, qdict_get_str(listen, "port"));
+
+    acl = qdict_get_qdict(vnc, "acl");
+    g_assert(acl);
+    g_assert_cmpint(qdict_size(acl), ==, 3);
+
+    rules = qdict_get_qlist(acl, "rules");
+    g_assert(rules);
+    g_assert_cmpint(qlist_size(rules), ==, 2);
+
+    rule = qobject_to(QDict, qlist_pop(rules));
+    g_assert(rule);
+    g_assert_cmpint(qdict_size(rule), ==, 2);
+    g_assert_cmpstr("fred", ==, qdict_get_str(rule, "match"));
+    g_assert_cmpstr("allow", ==, qdict_get_str(rule, "policy"));
+    qobject_unref(rule);
+
+    rule = qobject_to(QDict, qlist_pop(rules));
+    g_assert(rule);
+    g_assert_cmpint(qdict_size(rule), ==, 2);
+    g_assert_cmpstr("bob", ==, qdict_get_str(rule, "match"));
+    g_assert_cmpstr("deny", ==, qdict_get_str(rule, "policy"));
+    qobject_unref(rule);
+
+    /* With recursive crumpling, we should see all names unescaped */
+    g_assert_cmpstr("acl0", ==, qdict_get_str(vnc, "acl.name"));
+    g_assert_cmpstr("acl0", ==, qdict_get_str(acl, "rule.name"));
+
+    qobject_unref(src);
+    qobject_unref(dst);
+}
+
+static void qdict_crumple_test_empty(void)
+{
+    QDict *src, *dst;
+
+    src = qdict_new();
+
+    dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
+
+    g_assert_cmpint(qdict_size(dst), ==, 0);
+
+    qobject_unref(src);
+    qobject_unref(dst);
+}
+
+static int qdict_count_entries(QDict *dict)
+{
+    const QDictEntry *e;
+    int count = 0;
+
+    for (e = qdict_first(dict); e; e = qdict_next(dict, e)) {
+        count++;
+    }
+
+    return count;
+}
+
+static void qdict_rename_keys_test(void)
+{
+    QDict *dict = qdict_new();
+    QDict *copy;
+    QDictRenames *renames;
+    Error *local_err = NULL;
+
+    qdict_put_str(dict, "abc", "foo");
+    qdict_put_str(dict, "abcdef", "bar");
+    qdict_put_int(dict, "number", 42);
+    qdict_put_bool(dict, "flag", true);
+    qdict_put_null(dict, "nothing");
+
+    /* Empty rename list */
+    renames = (QDictRenames[]) {
+        { NULL, "this can be anything" }
+    };
+    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);
+    g_assert_cmpint(qdict_count_entries(copy), ==, 5);
+
+    qobject_unref(copy);
+
+    /* Simple rename of all entries */
+    renames = (QDictRenames[]) {
+        { "abc",        "str1" },
+        { "abcdef",     "str2" },
+        { "number",     "int" },
+        { "flag",       "bool" },
+        { "nothing",    "null" },
+        { 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"));
+
+    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);
+    g_assert_cmpint(qdict_count_entries(copy), ==, 5);
+
+    qobject_unref(copy);
+
+    /* Renames are processed top to bottom */
+    renames = (QDictRenames[]) {
+        { "abc",        "tmp" },
+        { "abcdef",     "abc" },
+        { "number",     "abcdef" },
+        { "flag",       "number" },
+        { "nothing",    "flag" },
+        { "tmp",        "nothing" },
+        { NULL , NULL }
+    };
+    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"));
+    g_assert_cmpint(qdict_count_entries(copy), ==, 5);
+
+    qobject_unref(copy);
+
+    /* Conflicting rename */
+    renames = (QDictRenames[]) {
+        { "abcdef",     "abc" },
+        { NULL , NULL }
+    };
+    copy = qdict_clone_shallow(dict);
+    qdict_rename_keys(copy, renames, &local_err);
+
+    g_assert(local_err != NULL);
+    error_free(local_err);
+    local_err = NULL;
+
+    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);
+    g_assert_cmpint(qdict_count_entries(copy), ==, 5);
+
+    qobject_unref(copy);
+
+    /* Renames in an empty dict */
+    renames = (QDictRenames[]) {
+        { "abcdef",     "abc" },
+        { NULL , NULL }
+    };
+
+    qobject_unref(dict);
+    dict = qdict_new();
+
+    qdict_rename_keys(dict, renames, &error_abort);
+    g_assert(qdict_first(dict) == NULL);
+
+    qobject_unref(dict);
+}
+
+static void qdict_crumple_test_bad_inputs(void)
+{
+    QDict *src;
+    Error *error = NULL;
+
+    src = qdict_new();
+    /* rule.0 can't be both a string and a dict */
+    qdict_put_str(src, "rule.0", "fred");
+    qdict_put_str(src, "rule.0.policy", "allow");
+
+    g_assert(qdict_crumple(src, &error) == NULL);
+    g_assert(error != NULL);
+    error_free(error);
+    error = NULL;
+    qobject_unref(src);
+
+    src = qdict_new();
+    /* rule can't be both a list and a dict */
+    qdict_put_str(src, "rule.0", "fred");
+    qdict_put_str(src, "rule.a", "allow");
+
+    g_assert(qdict_crumple(src, &error) == NULL);
+    g_assert(error != NULL);
+    error_free(error);
+    error = NULL;
+    qobject_unref(src);
+
+    src = qdict_new();
+    /* The input should be flat, ie no dicts or lists */
+    qdict_put(src, "rule.a", qdict_new());
+    qdict_put_str(src, "rule.b", "allow");
+
+    g_assert(qdict_crumple(src, &error) == NULL);
+    g_assert(error != NULL);
+    error_free(error);
+    error = NULL;
+    qobject_unref(src);
+
+    src = qdict_new();
+    /* List indexes must not have gaps */
+    qdict_put_str(src, "rule.0", "deny");
+    qdict_put_str(src, "rule.3", "allow");
+
+    g_assert(qdict_crumple(src, &error) == NULL);
+    g_assert(error != NULL);
+    error_free(error);
+    error = NULL;
+    qobject_unref(src);
+
+    src = qdict_new();
+    /* List indexes must be in %zu format */
+    qdict_put_str(src, "rule.0", "deny");
+    qdict_put_str(src, "rule.+1", "allow");
+
+    g_assert(qdict_crumple(src, &error) == NULL);
+    g_assert(error != NULL);
+    error_free(error);
+    error = NULL;
+    qobject_unref(src);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/public/defaults", qdict_defaults_test);
+    g_test_add_func("/public/flatten", qdict_flatten_test);
+    g_test_add_func("/public/array_split", qdict_array_split_test);
+    g_test_add_func("/public/array_entries", qdict_array_entries_test);
+    g_test_add_func("/public/join", qdict_join_test);
+    g_test_add_func("/public/crumple/recursive",
+                    qdict_crumple_test_recursive);
+    g_test_add_func("/public/crumple/empty",
+                    qdict_crumple_test_empty);
+    g_test_add_func("/public/crumple/bad_inputs",
+                    qdict_crumple_test_bad_inputs);
+
+    g_test_add_func("/public/rename_keys", qdict_rename_keys_test);
+
+    return g_test_run();
+}
diff --git a/tests/check-qdict.c b/tests/check-qdict.c
index 93e2112b6d..86e9fe7dc4 100644
--- a/tests/check-qdict.c
+++ b/tests/check-qdict.c
@@ -11,13 +11,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "block/qdict.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qlist.h"
-#include "qapi/qmp/qnum.h"
-#include "qapi/qmp/qstring.h"
-#include "qapi/error.h"
-#include "qemu-common.h"
 
 /*
  * Public Interface test-cases
@@ -157,28 +151,6 @@ static void qdict_get_try_str_test(void)
     qobject_unref(tests_dict);
 }
 
-static void qdict_defaults_test(void)
-{
-    QDict *dict, *copy;
-
-    dict = qdict_new();
-    copy = qdict_new();
-
-    qdict_set_default_str(dict, "foo", "abc");
-    qdict_set_default_str(dict, "foo", "def");
-    g_assert_cmpstr(qdict_get_str(dict, "foo"), ==, "abc");
-    qdict_set_default_str(dict, "bar", "ghi");
-
-    qdict_copy_default(copy, dict, "foo");
-    g_assert_cmpstr(qdict_get_str(copy, "foo"), ==, "abc");
-    qdict_set_default_str(copy, "bar", "xyz");
-    qdict_copy_default(copy, dict, "bar");
-    g_assert_cmpstr(qdict_get_str(copy, "bar"), ==, "xyz");
-
-    qobject_unref(copy);
-    qobject_unref(dict);
-}
-
 static void qdict_haskey_not_test(void)
 {
     QDict *tests_dict = qdict_new();
@@ -254,606 +226,6 @@ static void qdict_iterapi_test(void)
     qobject_unref(tests_dict);
 }
 
-static void qdict_flatten_test(void)
-{
-    QList *list1 = qlist_new();
-    QList *list2 = qlist_new();
-    QDict *dict1 = qdict_new();
-    QDict *dict2 = qdict_new();
-    QDict *dict3 = qdict_new();
-
-    /*
-     * Test the flattening of
-     *
-     * {
-     *     "e": [
-     *         42,
-     *         [
-     *             23,
-     *             66,
-     *             {
-     *                 "a": 0,
-     *                 "b": 1
-     *             }
-     *         ]
-     *     ],
-     *     "f": {
-     *         "c": 2,
-     *         "d": 3,
-     *     },
-     *     "g": 4
-     * }
-     *
-     * to
-     *
-     * {
-     *     "e.0": 42,
-     *     "e.1.0": 23,
-     *     "e.1.1": 66,
-     *     "e.1.2.a": 0,
-     *     "e.1.2.b": 1,
-     *     "f.c": 2,
-     *     "f.d": 3,
-     *     "g": 4
-     * }
-     */
-
-    qdict_put_int(dict1, "a", 0);
-    qdict_put_int(dict1, "b", 1);
-
-    qlist_append_int(list1, 23);
-    qlist_append_int(list1, 66);
-    qlist_append(list1, dict1);
-    qlist_append_int(list2, 42);
-    qlist_append(list2, list1);
-
-    qdict_put_int(dict2, "c", 2);
-    qdict_put_int(dict2, "d", 3);
-    qdict_put(dict3, "e", list2);
-    qdict_put(dict3, "f", dict2);
-    qdict_put_int(dict3, "g", 4);
-
-    qdict_flatten(dict3);
-
-    g_assert(qdict_get_int(dict3, "e.0") == 42);
-    g_assert(qdict_get_int(dict3, "e.1.0") == 23);
-    g_assert(qdict_get_int(dict3, "e.1.1") == 66);
-    g_assert(qdict_get_int(dict3, "e.1.2.a") == 0);
-    g_assert(qdict_get_int(dict3, "e.1.2.b") == 1);
-    g_assert(qdict_get_int(dict3, "f.c") == 2);
-    g_assert(qdict_get_int(dict3, "f.d") == 3);
-    g_assert(qdict_get_int(dict3, "g") == 4);
-
-    g_assert(qdict_size(dict3) == 8);
-
-    qobject_unref(dict3);
-}
-
-static void qdict_array_split_test(void)
-{
-    QDict *test_dict = qdict_new();
-    QDict *dict1, *dict2;
-    QNum *int1;
-    QList *test_list;
-
-    /*
-     * Test the split of
-     *
-     * {
-     *     "1.x": 0,
-     *     "4.y": 1,
-     *     "0.a": 42,
-     *     "o.o": 7,
-     *     "0.b": 23,
-     *     "2": 66
-     * }
-     *
-     * to
-     *
-     * [
-     *     {
-     *         "a": 42,
-     *         "b": 23
-     *     },
-     *     {
-     *         "x": 0
-     *     },
-     *     66
-     * ]
-     *
-     * and
-     *
-     * {
-     *     "4.y": 1,
-     *     "o.o": 7
-     * }
-     *
-     * (remaining in the old QDict)
-     *
-     * This example is given in the comment of qdict_array_split().
-     */
-
-    qdict_put_int(test_dict, "1.x", 0);
-    qdict_put_int(test_dict, "4.y", 1);
-    qdict_put_int(test_dict, "0.a", 42);
-    qdict_put_int(test_dict, "o.o", 7);
-    qdict_put_int(test_dict, "0.b", 23);
-    qdict_put_int(test_dict, "2", 66);
-
-    qdict_array_split(test_dict, &test_list);
-
-    dict1 = qobject_to(QDict, qlist_pop(test_list));
-    dict2 = qobject_to(QDict, qlist_pop(test_list));
-    int1 = qobject_to(QNum, qlist_pop(test_list));
-
-    g_assert(dict1);
-    g_assert(dict2);
-    g_assert(int1);
-    g_assert(qlist_empty(test_list));
-
-    qobject_unref(test_list);
-
-    g_assert(qdict_get_int(dict1, "a") == 42);
-    g_assert(qdict_get_int(dict1, "b") == 23);
-
-    g_assert(qdict_size(dict1) == 2);
-
-    qobject_unref(dict1);
-
-    g_assert(qdict_get_int(dict2, "x") == 0);
-
-    g_assert(qdict_size(dict2) == 1);
-
-    qobject_unref(dict2);
-
-    g_assert_cmpint(qnum_get_int(int1), ==, 66);
-
-    qobject_unref(int1);
-
-    g_assert(qdict_get_int(test_dict, "4.y") == 1);
-    g_assert(qdict_get_int(test_dict, "o.o") == 7);
-
-    g_assert(qdict_size(test_dict) == 2);
-
-    qobject_unref(test_dict);
-
-    /*
-     * Test the split of
-     *
-     * {
-     *     "0": 42,
-     *     "1": 23,
-     *     "1.x": 84
-     * }
-     *
-     * to
-     *
-     * [
-     *     42
-     * ]
-     *
-     * and
-     *
-     * {
-     *     "1": 23,
-     *     "1.x": 84
-     * }
-     *
-     * That is, test whether splitting stops if there is both an entry with key
-     * of "%u" and other entries with keys prefixed "%u." for the same index.
-     */
-
-    test_dict = qdict_new();
-
-    qdict_put_int(test_dict, "0", 42);
-    qdict_put_int(test_dict, "1", 23);
-    qdict_put_int(test_dict, "1.x", 84);
-
-    qdict_array_split(test_dict, &test_list);
-
-    int1 = qobject_to(QNum, qlist_pop(test_list));
-
-    g_assert(int1);
-    g_assert(qlist_empty(test_list));
-
-    qobject_unref(test_list);
-
-    g_assert_cmpint(qnum_get_int(int1), ==, 42);
-
-    qobject_unref(int1);
-
-    g_assert(qdict_get_int(test_dict, "1") == 23);
-    g_assert(qdict_get_int(test_dict, "1.x") == 84);
-
-    g_assert(qdict_size(test_dict) == 2);
-
-    qobject_unref(test_dict);
-}
-
-static void qdict_array_entries_test(void)
-{
-    QDict *dict = qdict_new();
-
-    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 0);
-
-    qdict_put_int(dict, "bar", 0);
-    qdict_put_int(dict, "baz.0", 0);
-    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 0);
-
-    qdict_put_int(dict, "foo.1", 0);
-    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, -EINVAL);
-    qdict_put_int(dict, "foo.0", 0);
-    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 2);
-    qdict_put_int(dict, "foo.bar", 0);
-    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, -EINVAL);
-    qdict_del(dict, "foo.bar");
-
-    qdict_put_int(dict, "foo.2.a", 0);
-    qdict_put_int(dict, "foo.2.b", 0);
-    qdict_put_int(dict, "foo.2.c", 0);
-    g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 3);
-    g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL);
-
-    qobject_unref(dict);
-
-    dict = qdict_new();
-    qdict_put_int(dict, "1", 0);
-    g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL);
-    qdict_put_int(dict, "0", 0);
-    g_assert_cmpint(qdict_array_entries(dict, ""), ==, 2);
-    qdict_put_int(dict, "bar", 0);
-    g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL);
-    qdict_del(dict, "bar");
-
-    qdict_put_int(dict, "2.a", 0);
-    qdict_put_int(dict, "2.b", 0);
-    qdict_put_int(dict, "2.c", 0);
-    g_assert_cmpint(qdict_array_entries(dict, ""), ==, 3);
-
-    qobject_unref(dict);
-}
-
-static void qdict_join_test(void)
-{
-    QDict *dict1, *dict2;
-    bool overwrite = false;
-    int i;
-
-    dict1 = qdict_new();
-    dict2 = qdict_new();
-
-    /* Test everything once without overwrite and once with */
-    do
-    {
-        /* Test empty dicts */
-        qdict_join(dict1, dict2, overwrite);
-
-        g_assert(qdict_size(dict1) == 0);
-        g_assert(qdict_size(dict2) == 0);
-
-        /* First iteration: Test movement */
-        /* Second iteration: Test empty source and non-empty destination */
-        qdict_put_int(dict2, "foo", 42);
-
-        for (i = 0; i < 2; i++) {
-            qdict_join(dict1, dict2, overwrite);
-
-            g_assert(qdict_size(dict1) == 1);
-            g_assert(qdict_size(dict2) == 0);
-
-            g_assert(qdict_get_int(dict1, "foo") == 42);
-        }
-
-        /* Test non-empty source and destination without conflict */
-        qdict_put_int(dict2, "bar", 23);
-
-        qdict_join(dict1, dict2, overwrite);
-
-        g_assert(qdict_size(dict1) == 2);
-        g_assert(qdict_size(dict2) == 0);
-
-        g_assert(qdict_get_int(dict1, "foo") == 42);
-        g_assert(qdict_get_int(dict1, "bar") == 23);
-
-        /* Test conflict */
-        qdict_put_int(dict2, "foo", 84);
-
-        qdict_join(dict1, dict2, overwrite);
-
-        g_assert(qdict_size(dict1) == 2);
-        g_assert(qdict_size(dict2) == !overwrite);
-
-        g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42));
-        g_assert(qdict_get_int(dict1, "bar") == 23);
-
-        if (!overwrite) {
-            g_assert(qdict_get_int(dict2, "foo") == 84);
-        }
-
-        /* Check the references */
-        g_assert(qdict_get(dict1, "foo")->base.refcnt == 1);
-        g_assert(qdict_get(dict1, "bar")->base.refcnt == 1);
-
-        if (!overwrite) {
-            g_assert(qdict_get(dict2, "foo")->base.refcnt == 1);
-        }
-
-        /* Clean up */
-        qdict_del(dict1, "foo");
-        qdict_del(dict1, "bar");
-
-        if (!overwrite) {
-            qdict_del(dict2, "foo");
-        }
-    }
-    while (overwrite ^= true);
-
-    qobject_unref(dict1);
-    qobject_unref(dict2);
-}
-
-static void qdict_crumple_test_recursive(void)
-{
-    QDict *src, *dst, *rule, *vnc, *acl, *listen;
-    QList *rules;
-
-    src = qdict_new();
-    qdict_put_str(src, "vnc.listen.addr", "127.0.0.1");
-    qdict_put_str(src, "vnc.listen.port", "5901");
-    qdict_put_str(src, "vnc.acl.rules.0.match", "fred");
-    qdict_put_str(src, "vnc.acl.rules.0.policy", "allow");
-    qdict_put_str(src, "vnc.acl.rules.1.match", "bob");
-    qdict_put_str(src, "vnc.acl.rules.1.policy", "deny");
-    qdict_put_str(src, "vnc.acl.default", "deny");
-    qdict_put_str(src, "vnc.acl..name", "acl0");
-    qdict_put_str(src, "vnc.acl.rule..name", "acl0");
-
-    dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
-    g_assert(dst);
-    g_assert_cmpint(qdict_size(dst), ==, 1);
-
-    vnc = qdict_get_qdict(dst, "vnc");
-    g_assert(vnc);
-    g_assert_cmpint(qdict_size(vnc), ==, 3);
-
-    listen = qdict_get_qdict(vnc, "listen");
-    g_assert(listen);
-    g_assert_cmpint(qdict_size(listen), ==, 2);
-    g_assert_cmpstr("127.0.0.1", ==, qdict_get_str(listen, "addr"));
-    g_assert_cmpstr("5901", ==, qdict_get_str(listen, "port"));
-
-    acl = qdict_get_qdict(vnc, "acl");
-    g_assert(acl);
-    g_assert_cmpint(qdict_size(acl), ==, 3);
-
-    rules = qdict_get_qlist(acl, "rules");
-    g_assert(rules);
-    g_assert_cmpint(qlist_size(rules), ==, 2);
-
-    rule = qobject_to(QDict, qlist_pop(rules));
-    g_assert(rule);
-    g_assert_cmpint(qdict_size(rule), ==, 2);
-    g_assert_cmpstr("fred", ==, qdict_get_str(rule, "match"));
-    g_assert_cmpstr("allow", ==, qdict_get_str(rule, "policy"));
-    qobject_unref(rule);
-
-    rule = qobject_to(QDict, qlist_pop(rules));
-    g_assert(rule);
-    g_assert_cmpint(qdict_size(rule), ==, 2);
-    g_assert_cmpstr("bob", ==, qdict_get_str(rule, "match"));
-    g_assert_cmpstr("deny", ==, qdict_get_str(rule, "policy"));
-    qobject_unref(rule);
-
-    /* With recursive crumpling, we should see all names unescaped */
-    g_assert_cmpstr("acl0", ==, qdict_get_str(vnc, "acl.name"));
-    g_assert_cmpstr("acl0", ==, qdict_get_str(acl, "rule.name"));
-
-    qobject_unref(src);
-    qobject_unref(dst);
-}
-
-static void qdict_crumple_test_empty(void)
-{
-    QDict *src, *dst;
-
-    src = qdict_new();
-
-    dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
-
-    g_assert_cmpint(qdict_size(dst), ==, 0);
-
-    qobject_unref(src);
-    qobject_unref(dst);
-}
-
-static int qdict_count_entries(QDict *dict)
-{
-    const QDictEntry *e;
-    int count = 0;
-
-    for (e = qdict_first(dict); e; e = qdict_next(dict, e)) {
-        count++;
-    }
-
-    return count;
-}
-
-static void qdict_rename_keys_test(void)
-{
-    QDict *dict = qdict_new();
-    QDict *copy;
-    QDictRenames *renames;
-    Error *local_err = NULL;
-
-    qdict_put_str(dict, "abc", "foo");
-    qdict_put_str(dict, "abcdef", "bar");
-    qdict_put_int(dict, "number", 42);
-    qdict_put_bool(dict, "flag", true);
-    qdict_put_null(dict, "nothing");
-
-    /* Empty rename list */
-    renames = (QDictRenames[]) {
-        { NULL, "this can be anything" }
-    };
-    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);
-    g_assert_cmpint(qdict_count_entries(copy), ==, 5);
-
-    qobject_unref(copy);
-
-    /* Simple rename of all entries */
-    renames = (QDictRenames[]) {
-        { "abc",        "str1" },
-        { "abcdef",     "str2" },
-        { "number",     "int" },
-        { "flag",       "bool" },
-        { "nothing",    "null" },
-        { 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"));
-
-    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);
-    g_assert_cmpint(qdict_count_entries(copy), ==, 5);
-
-    qobject_unref(copy);
-
-    /* Renames are processed top to bottom */
-    renames = (QDictRenames[]) {
-        { "abc",        "tmp" },
-        { "abcdef",     "abc" },
-        { "number",     "abcdef" },
-        { "flag",       "number" },
-        { "nothing",    "flag" },
-        { "tmp",        "nothing" },
-        { NULL , NULL }
-    };
-    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"));
-    g_assert_cmpint(qdict_count_entries(copy), ==, 5);
-
-    qobject_unref(copy);
-
-    /* Conflicting rename */
-    renames = (QDictRenames[]) {
-        { "abcdef",     "abc" },
-        { NULL , NULL }
-    };
-    copy = qdict_clone_shallow(dict);
-    qdict_rename_keys(copy, renames, &local_err);
-
-    g_assert(local_err != NULL);
-    error_free(local_err);
-    local_err = NULL;
-
-    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);
-    g_assert_cmpint(qdict_count_entries(copy), ==, 5);
-
-    qobject_unref(copy);
-
-    /* Renames in an empty dict */
-    renames = (QDictRenames[]) {
-        { "abcdef",     "abc" },
-        { NULL , NULL }
-    };
-
-    qobject_unref(dict);
-    dict = qdict_new();
-
-    qdict_rename_keys(dict, renames, &error_abort);
-    g_assert(qdict_first(dict) == NULL);
-
-    qobject_unref(dict);
-}
-
-static void qdict_crumple_test_bad_inputs(void)
-{
-    QDict *src;
-    Error *error = NULL;
-
-    src = qdict_new();
-    /* rule.0 can't be both a string and a dict */
-    qdict_put_str(src, "rule.0", "fred");
-    qdict_put_str(src, "rule.0.policy", "allow");
-
-    g_assert(qdict_crumple(src, &error) == NULL);
-    g_assert(error != NULL);
-    error_free(error);
-    error = NULL;
-    qobject_unref(src);
-
-    src = qdict_new();
-    /* rule can't be both a list and a dict */
-    qdict_put_str(src, "rule.0", "fred");
-    qdict_put_str(src, "rule.a", "allow");
-
-    g_assert(qdict_crumple(src, &error) == NULL);
-    g_assert(error != NULL);
-    error_free(error);
-    error = NULL;
-    qobject_unref(src);
-
-    src = qdict_new();
-    /* The input should be flat, ie no dicts or lists */
-    qdict_put(src, "rule.a", qdict_new());
-    qdict_put_str(src, "rule.b", "allow");
-
-    g_assert(qdict_crumple(src, &error) == NULL);
-    g_assert(error != NULL);
-    error_free(error);
-    error = NULL;
-    qobject_unref(src);
-
-    src = qdict_new();
-    /* List indexes must not have gaps */
-    qdict_put_str(src, "rule.0", "deny");
-    qdict_put_str(src, "rule.3", "allow");
-
-    g_assert(qdict_crumple(src, &error) == NULL);
-    g_assert(error != NULL);
-    error_free(error);
-    error = NULL;
-    qobject_unref(src);
-
-    src = qdict_new();
-    /* List indexes must be in %zu format */
-    qdict_put_str(src, "rule.0", "deny");
-    qdict_put_str(src, "rule.+1", "allow");
-
-    g_assert(qdict_crumple(src, &error) == NULL);
-    g_assert(error != NULL);
-    error_free(error);
-    error = NULL;
-    qobject_unref(src);
-}
-
 /*
  * Errors test-cases
  */
@@ -987,29 +359,15 @@ int main(int argc, char **argv)
     g_test_add_func("/public/get_try_int", qdict_get_try_int_test);
     g_test_add_func("/public/get_str", qdict_get_str_test);
     g_test_add_func("/public/get_try_str", qdict_get_try_str_test);
-    g_test_add_func("/public/defaults", qdict_defaults_test);
     g_test_add_func("/public/haskey_not", qdict_haskey_not_test);
     g_test_add_func("/public/haskey", qdict_haskey_test);
     g_test_add_func("/public/del", qdict_del_test);
     g_test_add_func("/public/to_qdict", qobject_to_qdict_test);
     g_test_add_func("/public/iterapi", qdict_iterapi_test);
-    g_test_add_func("/public/flatten", qdict_flatten_test);
-    g_test_add_func("/public/array_split", qdict_array_split_test);
-    g_test_add_func("/public/array_entries", qdict_array_entries_test);
-    g_test_add_func("/public/join", qdict_join_test);
 
     g_test_add_func("/errors/put_exists", qdict_put_exists_test);
     g_test_add_func("/errors/get_not_exists", qdict_get_not_exists_test);
 
-    g_test_add_func("/public/crumple/recursive",
-                    qdict_crumple_test_recursive);
-    g_test_add_func("/public/crumple/empty",
-                    qdict_crumple_test_empty);
-    g_test_add_func("/public/crumple/bad_inputs",
-                    qdict_crumple_test_bad_inputs);
-
-    g_test_add_func("/public/rename_keys", qdict_rename_keys_test);
-
     /* The Big one */
     if (g_test_slow()) {
         g_test_add_func("/stress/test", qdict_stress_test);
-- 
2.17.1


[Qemu-devel] [PATCH 05/18] block: Fix -blockdev for certain non-string scalars
Posted by Markus Armbruster, 1 week ago
Configuration flows through the block subsystem in a rather peculiar
way.  Configuration made with -drive enters it as QemuOpts.
Configuration made with -blockdev / blockdev-add enters it as QAPI
type BlockdevOptions.  The block subsystem uses QDict, QemuOpts and
QAPI types internally.  The precise flow is next to impossible to
explain (I tried for this commit message, but gave up after wasting
several hours).  What I can explain is a flaw in the BlockDriver
interface that leads to this bug:

    $ qemu-system-x86 -blockdev node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234: Internal error: parameter user invalid
    qemu-system-x86: -blockdev node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234: Internal error: parameter user invalid

QMP blockdev-add is broken the same way.

Here's what happens.  The block layer passes configuration represented
as flat QDict (with dotted keys) to BlockDriver methods
.bdrv_file_open().  The QDict's members are typed according to the
QAPI schema.

nfs_file_open() converts it to QAPI type BlockdevOptionsNfs, with
qdict_crumple() and a qobject input visitor.

This visitor comes in two flavors.  The plain flavor requires scalars
to be typed according to the QAPI schema.  That's the case here.  The
keyval flavor requires string scalars.  That's not the case here.
nfs_file_open() uses the latter, and promptly falls apart for members
@user, @group, @tcp-syn-count, @readahead-size, @page-cache-size,
@debug.

Switching to the plain flavor would fix -blockdev, but break -drive,
because there the scalars arrive in nfs_file_open() as strings.

The proper fix would be to replace the QDict by QAPI type
BlockdevOptions in the BlockDriver interface.  Sadly, that's beyond my
reach right now.

Next best would be to fix the block layer to always pass correctly
typed QDicts to the BlockDriver methods.  Also beyond my reach.

What I can do is throw another hack onto the pile: have
nfs_file_open() convert all members to string, so use of the keyval
flavor actually works, by replacing qdict_crumple() by new function
qdict_crumple_for_keyval_qiv().

The pattern "pass result of qdict_crumple() to
qobject_input_visitor_new_keyval()" occurs several times more:

* qemu_rbd_open()

  Same issue as nfs_file_open(), but since BlockdevOptionsRbd has only
  string members, its only a latent bug.  Fix it anyway.

* parallels_co_create_opts(), qcow_co_create_opts(),
  qcow2_co_create_opts(), bdrv_qed_co_create_opts(),
  sd_co_create_opts(), vhdx_co_create_opts(), vpc_co_create_opts()

  These work, because they create the QDict with
  qemu_opts_to_qdict_filtered(), which creates only string scalars.
  The function sports a TODO comment asking for better typing; that's
  going to be fun.  Use qdict_crumple_for_keyval_qiv() to be safe.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/nfs.c           |  2 +-
 block/parallels.c     |  2 +-
 block/qcow.c          |  2 +-
 block/qcow2.c         |  2 +-
 block/qed.c           |  2 +-
 block/rbd.c           |  2 +-
 block/sheepdog.c      |  2 +-
 block/vhdx.c          |  2 +-
 block/vpc.c           |  2 +-
 include/block/qdict.h |  1 +
 qobject/block-qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 3170b059b3..6935b611cc 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -561,7 +561,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
     const QDictEntry *e;
     Error *local_err = NULL;
 
-    crumpled = qdict_crumple(options, errp);
+    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
     if (crumpled == NULL) {
         return NULL;
     }
diff --git a/block/parallels.c b/block/parallels.c
index c1d9643498..695899fc4b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -653,7 +653,7 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "parallels");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {
diff --git a/block/qcow.c b/block/qcow.c
index 8c08908fd8..860b058240 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -997,7 +997,7 @@ static int coroutine_fn qcow_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "qcow");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {
diff --git a/block/qcow2.c b/block/qcow2.c
index d2d955f984..0a27afa2ef 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3152,7 +3152,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
     qdict_put_str(qdict, "file", bs->node_name);
 
     /* Now get the QAPI type BlockdevCreateOptions */
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {
diff --git a/block/qed.c b/block/qed.c
index 324a953cbc..88fa36d409 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -763,7 +763,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "qed");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {
diff --git a/block/rbd.c b/block/rbd.c
index 9659c7361f..09720e97c0 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -647,7 +647,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Convert the remaining options into a QAPI object */
-    crumpled = qdict_crumple(options, errp);
+    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
     if (crumpled == NULL) {
         r = -EINVAL;
         goto out;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 2e1f0e6eca..a93f93d360 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2217,7 +2217,7 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts,
     }
 
     /* Get the QAPI object */
-    crumpled = qdict_crumple(qdict, errp);
+    crumpled = qdict_crumple_for_keyval_qiv(qdict, errp);
     if (crumpled == NULL) {
         ret = -EINVAL;
         goto fail;
diff --git a/block/vhdx.c b/block/vhdx.c
index 2e32e24514..78b29d9fc7 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2005,7 +2005,7 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "vhdx");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {
diff --git a/block/vpc.c b/block/vpc.c
index 41c8c980f1..16178e5a90 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1119,7 +1119,7 @@ static int coroutine_fn vpc_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "vpc");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {
diff --git a/include/block/qdict.h b/include/block/qdict.h
index 71c037afba..47d9638c37 100644
--- a/include/block/qdict.h
+++ b/include/block/qdict.h
@@ -21,6 +21,7 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
 void qdict_array_split(QDict *src, QList **dst);
 int qdict_array_entries(QDict *src, const char *subqdict);
 QObject *qdict_crumple(const QDict *src, Error **errp);
+QObject *qdict_crumple_for_keyval_qiv(QDict *qdict, Error **errp);
 void qdict_flatten(QDict *qdict);
 
 typedef struct QDictRenames {
diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index fb92010dc5..aba372c2eb 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -9,7 +9,10 @@
 
 #include "qemu/osdep.h"
 #include "block/qdict.h"
+#include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qstring.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
 
@@ -513,6 +516,60 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
     return NULL;
 }
 
+/**
+ * qdict_crumple_for_keyval_qiv:
+ * @src: the flat dictionary (only scalar values) to crumple
+ * @errp: location to store error
+ *
+ * Like qdict_crumple(), but additionally transforms scalar values so
+ * the result can be passed to qobject_input_visitor_new_keyval().
+ *
+ * The block subsystem uses this function to prepare its flat QDict
+ * with possibly confused scalar types for a visit.  It should not be
+ * used for anything else, and it should go away once the block
+ * subsystem has been cleaned up.
+ */
+QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp)
+{
+    QDict *tmp = NULL;
+    char *buf;
+    const char *s;
+    const QDictEntry *ent;
+    QObject *dst;
+
+    for (ent = qdict_first(src); ent; ent = qdict_next(src, ent)) {
+        buf = NULL;
+        switch (qobject_type(ent->value)) {
+        case QTYPE_QNULL:
+        case QTYPE_QSTRING:
+            continue;
+        case QTYPE_QNUM:
+            s = buf = qnum_to_string(qobject_to(QNum, ent->value));
+            break;
+        case QTYPE_QDICT:
+        case QTYPE_QLIST:
+            /* @src isn't flat; qdict_crumple() will fail */
+            continue;
+        case QTYPE_QBOOL:
+            s = qbool_get_bool(qobject_to(QBool, ent->value))
+                ? "on" : "off";
+            break;
+        default:
+            abort();
+        }
+
+        if (!tmp) {
+            tmp = qdict_clone_shallow(src);
+        }
+        qdict_put(tmp, ent->key, qstring_from_str(s));
+        g_free(buf);
+    }
+
+    dst = qdict_crumple(tmp ?: src, errp);
+    qobject_unref(tmp);
+    return dst;
+}
+
 /**
  * qdict_array_entries(): Returns the number of direct array entries if the
  * sub-QDict of src specified by the prefix in subqdict (or src itself for
-- 
2.17.1


[Qemu-devel] [PATCH 06/18] block: Fix -drive for certain non-string scalars
Posted by Markus Armbruster, 1 week ago
The previous commit fixed -blockdev breakage due to misuse of the
qobject input visitor's keyval flavor in bdrv_file_open().  The commit
message explain why using the plain flavor would be just as wrong; it
would break -drive.  Turns out we break it in three places:
nbd_open(), sd_open() and ssh_file_open().  They are even marked
FIXME.  Example breakage:

    $ qemu-system-x86 -drive node-name=n1,driver=nbd,server.type=inet,server.host=localhost,server.port=1234,server.numeric=off
    qemu-system-x86: -drive node-name=n1,driver=nbd,server.type=inet,server.host=localhost,server.port=1234,server.numeric=off: Invalid parameter type for 'numeric', expected: boolean

Fix it the same way: replace qdict_crumple() by
qdict_crumple_for_keyval_qiv(), and switch from plain to the keyval
flavor.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/nbd.c      | 12 ++----------
 block/sheepdog.c | 12 ++----------
 block/ssh.c      | 12 ++----------
 3 files changed, 6 insertions(+), 30 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index d6c4c4ddbc..614dd9fec0 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -273,20 +273,12 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options,
         goto done;
     }
 
-    crumpled_addr = qdict_crumple(addr, errp);
+    crumpled_addr = qdict_crumple_for_keyval_qiv(addr, errp);
     if (!crumpled_addr) {
         goto done;
     }
 
-    /*
-     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
-     * server.type=inet.  .to doesn't matter, it's ignored anyway.
-     * That's because when @options come from -blockdev or
-     * blockdev_add, members are typed according to the QAPI schema,
-     * but when they come from -drive, they're all QString.  The
-     * visitor expects the former.
-     */
-    iv = qobject_input_visitor_new(crumpled_addr);
+    iv = qobject_input_visitor_new_keyval(crumpled_addr);
     visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index a93f93d360..29e3e1eaaa 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -546,20 +546,12 @@ static SocketAddress *sd_server_config(QDict *options, Error **errp)
 
     qdict_extract_subqdict(options, &server, "server.");
 
-    crumpled_server = qdict_crumple(server, errp);
+    crumpled_server = qdict_crumple_for_keyval_qiv(server, errp);
     if (!crumpled_server) {
         goto done;
     }
 
-    /*
-     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
-     * server.type=inet.  .to doesn't matter, it's ignored anyway.
-     * That's because when @options come from -blockdev or
-     * blockdev_add, members are typed according to the QAPI schema,
-     * but when they come from -drive, they're all QString.  The
-     * visitor expects the former.
-     */
-    iv = qobject_input_visitor_new(crumpled_server);
+    iv = qobject_input_visitor_new_keyval(crumpled_server);
     visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/block/ssh.c b/block/ssh.c
index eec37dd27c..bd85d989d5 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -623,20 +623,12 @@ static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp)
     }
 
     /* Create the QAPI object */
-    crumpled = qdict_crumple(options, errp);
+    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
     if (crumpled == NULL) {
         goto fail;
     }
 
-    /*
-     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive.
-     * .to doesn't matter, it's ignored anyway.
-     * That's because when @options come from -blockdev or
-     * blockdev_add, members are typed according to the QAPI schema,
-     * but when they come from -drive, they're all QString.  The
-     * visitor expects the former.
-     */
-    v = qobject_input_visitor_new(crumpled);
+    v = qobject_input_visitor_new_keyval(crumpled);
     visit_type_BlockdevOptionsSsh(v, NULL, &result, &local_err);
     visit_free(v);
     qobject_unref(crumpled);
-- 
2.17.1


[Qemu-devel] [PATCH 07/18] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts()
Posted by Markus Armbruster, 1 week ago
The following pattern occurs in the .bdrv_co_create_opts() methods of
parallels, qcow, qcow2, qed, vhdx and vpc:

    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
    qobject_unref(qdict);
    qdict = qobject_to(QDict, qobj);
    if (qdict == NULL) {
         ret = -EINVAL;
         goto done;
    }

    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
    [...]
    ret = 0;
done:
    qobject_unref(qdict);
    [...]
    return ret;

If qobject_to() fails, we return failure without setting errp.  That's
wrong.  As far as I can tell, it cannot fail here.  Clean it up
anyway, by removing the useless conversion.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/parallels.c | 9 ++++-----
 block/qcow.c      | 9 ++++-----
 block/qcow2.c     | 9 ++++-----
 block/qed.c       | 9 ++++-----
 block/vhdx.c      | 9 ++++-----
 block/vpc.c       | 9 ++++-----
 6 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 695899fc4b..ceb7a15d62 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -616,7 +616,7 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
     BlockdevCreateOptions *create_options = NULL;
     Error *local_err = NULL;
     BlockDriverState *bs = NULL;
-    QDict *qdict = NULL;
+    QDict *qdict;
     QObject *qobj;
     Visitor *v;
     int ret;
@@ -654,14 +654,13 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
     qdict_put_str(qdict, "file", bs->node_name);
 
     qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    qobject_unref(qdict);
-    qdict = qobject_to(QDict, qobj);
-    if (qdict == NULL) {
+    if (!qobj) {
         ret = -EINVAL;
         goto done;
     }
 
-    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    v = qobject_input_visitor_new_keyval(qobj);
+    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/qcow.c b/block/qcow.c
index 860b058240..2f81f081fd 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -946,7 +946,7 @@ static int coroutine_fn qcow_co_create_opts(const char *filename,
 {
     BlockdevCreateOptions *create_options = NULL;
     BlockDriverState *bs = NULL;
-    QDict *qdict = NULL;
+    QDict *qdict;
     QObject *qobj;
     Visitor *v;
     const char *val;
@@ -998,14 +998,13 @@ static int coroutine_fn qcow_co_create_opts(const char *filename,
     qdict_put_str(qdict, "file", bs->node_name);
 
     qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    qobject_unref(qdict);
-    qdict = qobject_to(QDict, qobj);
-    if (qdict == NULL) {
+    if (!qobj) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    v = qobject_input_visitor_new_keyval(qobj);
+    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 0a27afa2ef..8c338661db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3080,7 +3080,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
                                              Error **errp)
 {
     BlockdevCreateOptions *create_options = NULL;
-    QDict *qdict = NULL;
+    QDict *qdict;
     QObject *qobj;
     Visitor *v;
     BlockDriverState *bs = NULL;
@@ -3153,14 +3153,13 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
 
     /* Now get the QAPI type BlockdevCreateOptions */
     qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    qobject_unref(qdict);
-    qdict = qobject_to(QDict, qobj);
-    if (qdict == NULL) {
+    if (!qobj) {
         ret = -EINVAL;
         goto finish;
     }
 
-    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    v = qobject_input_visitor_new_keyval(qobj);
+    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/qed.c b/block/qed.c
index 88fa36d409..fcec760b26 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -722,7 +722,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
                                                 Error **errp)
 {
     BlockdevCreateOptions *create_options = NULL;
-    QDict *qdict = NULL;
+    QDict *qdict;
     QObject *qobj;
     Visitor *v;
     BlockDriverState *bs = NULL;
@@ -764,14 +764,13 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
     qdict_put_str(qdict, "file", bs->node_name);
 
     qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    qobject_unref(qdict);
-    qdict = qobject_to(QDict, qobj);
-    if (qdict == NULL) {
+    if (!qobj) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    v = qobject_input_visitor_new_keyval(qobj);
+    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/vhdx.c b/block/vhdx.c
index 78b29d9fc7..f2aec3d2cd 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1965,7 +1965,7 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename,
                                             Error **errp)
 {
     BlockdevCreateOptions *create_options = NULL;
-    QDict *qdict = NULL;
+    QDict *qdict;
     QObject *qobj;
     Visitor *v;
     BlockDriverState *bs = NULL;
@@ -2006,14 +2006,13 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename,
     qdict_put_str(qdict, "file", bs->node_name);
 
     qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    qobject_unref(qdict);
-    qdict = qobject_to(QDict, qobj);
-    if (qdict == NULL) {
+    if (!qobj) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    v = qobject_input_visitor_new_keyval(qobj);
+    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/vpc.c b/block/vpc.c
index 16178e5a90..a9bb04149d 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1081,7 +1081,7 @@ static int coroutine_fn vpc_co_create_opts(const char *filename,
                                            QemuOpts *opts, Error **errp)
 {
     BlockdevCreateOptions *create_options = NULL;
-    QDict *qdict = NULL;
+    QDict *qdict;
     QObject *qobj;
     Visitor *v;
     BlockDriverState *bs = NULL;
@@ -1120,14 +1120,13 @@ static int coroutine_fn vpc_co_create_opts(const char *filename,
     qdict_put_str(qdict, "file", bs->node_name);
 
     qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    qobject_unref(qdict);
-    qdict = qobject_to(QDict, qobj);
-    if (qdict == NULL) {
+    if (!qobj) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    v = qobject_input_visitor_new_keyval(qobj);
+    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
-- 
2.17.1


[Qemu-devel] [PATCH 08/18] block: Factor out qobject_input_visitor_new_flat_confused()
Posted by Markus Armbruster, 1 week ago
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/nbd.c           |  7 ++-----
 block/nfs.c           |  7 ++-----
 block/parallels.c     |  7 ++-----
 block/qcow.c          |  7 ++-----
 block/qcow2.c         |  7 ++-----
 block/qed.c           |  7 ++-----
 block/rbd.c           |  7 ++-----
 block/sheepdog.c      | 14 ++++----------
 block/ssh.c           |  7 ++-----
 block/vhdx.c          |  7 ++-----
 block/vpc.c           |  7 ++-----
 include/block/qdict.h |  3 ++-
 qobject/block-qdict.c | 28 +++++++++++++++++++++++++++-
 13 files changed, 53 insertions(+), 62 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 614dd9fec0..13db4030e6 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -263,7 +263,6 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options,
 {
     SocketAddress *saddr = NULL;
     QDict *addr = NULL;
-    QObject *crumpled_addr = NULL;
     Visitor *iv = NULL;
     Error *local_err = NULL;
 
@@ -273,12 +272,11 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options,
         goto done;
     }
 
-    crumpled_addr = qdict_crumple_for_keyval_qiv(addr, errp);
-    if (!crumpled_addr) {
+    iv = qobject_input_visitor_new_flat_confused(addr, errp);
+    if (!iv) {
         goto done;
     }
 
-    iv = qobject_input_visitor_new_keyval(crumpled_addr);
     visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -287,7 +285,6 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options,
 
 done:
     qobject_unref(addr);
-    qobject_unref(crumpled_addr);
     visit_free(iv);
     return saddr;
 }
diff --git a/block/nfs.c b/block/nfs.c
index 6935b611cc..743ca0450e 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -556,20 +556,17 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
                                                      Error **errp)
 {
     BlockdevOptionsNfs *opts = NULL;
-    QObject *crumpled = NULL;
     Visitor *v;
     const QDictEntry *e;
     Error *local_err = NULL;
 
-    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
-    if (crumpled == NULL) {
+    v = qobject_input_visitor_new_flat_confused(options, errp);
+    if (!v) {
         return NULL;
     }
 
-    v = qobject_input_visitor_new_keyval(crumpled);
     visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err);
     visit_free(v);
-    qobject_unref(crumpled);
 
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/block/parallels.c b/block/parallels.c
index ceb7a15d62..fd215e202a 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -617,7 +617,6 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
     Error *local_err = NULL;
     BlockDriverState *bs = NULL;
     QDict *qdict;
-    QObject *qobj;
     Visitor *v;
     int ret;
 
@@ -653,14 +652,12 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "parallels");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    if (!qobj) {
+    v = qobject_input_visitor_new_flat_confused(qdict, errp);
+    if (!v) {
         ret = -EINVAL;
         goto done;
     }
 
-    v = qobject_input_visitor_new_keyval(qobj);
-    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/qcow.c b/block/qcow.c
index 2f81f081fd..5532731b9f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -947,7 +947,6 @@ static int coroutine_fn qcow_co_create_opts(const char *filename,
     BlockdevCreateOptions *create_options = NULL;
     BlockDriverState *bs = NULL;
     QDict *qdict;
-    QObject *qobj;
     Visitor *v;
     const char *val;
     Error *local_err = NULL;
@@ -997,14 +996,12 @@ static int coroutine_fn qcow_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "qcow");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    if (!qobj) {
+    v = qobject_input_visitor_new_flat_confused(qdict, errp);
+    if (!v) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(qobj);
-    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 8c338661db..945132f692 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3081,7 +3081,6 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
 {
     BlockdevCreateOptions *create_options = NULL;
     QDict *qdict;
-    QObject *qobj;
     Visitor *v;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
@@ -3152,14 +3151,12 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
     qdict_put_str(qdict, "file", bs->node_name);
 
     /* Now get the QAPI type BlockdevCreateOptions */
-    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    if (!qobj) {
+    v = qobject_input_visitor_new_flat_confused(qdict, errp);
+    if (!v) {
         ret = -EINVAL;
         goto finish;
     }
 
-    v = qobject_input_visitor_new_keyval(qobj);
-    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/qed.c b/block/qed.c
index fcec760b26..2363814538 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -723,7 +723,6 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
 {
     BlockdevCreateOptions *create_options = NULL;
     QDict *qdict;
-    QObject *qobj;
     Visitor *v;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
@@ -763,14 +762,12 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "qed");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    if (!qobj) {
+    v = qobject_input_visitor_new_flat_confused(qdict, errp);
+    if (!v) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(qobj);
-    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/rbd.c b/block/rbd.c
index 09720e97c0..82346a2a5e 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -630,7 +630,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVRBDState *s = bs->opaque;
     BlockdevOptionsRbd *opts = NULL;
     Visitor *v;
-    QObject *crumpled = NULL;
     const QDictEntry *e;
     Error *local_err = NULL;
     char *keypairs, *secretid;
@@ -647,16 +646,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Convert the remaining options into a QAPI object */
-    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
-    if (crumpled == NULL) {
+    v = qobject_input_visitor_new_flat_confused(options, errp);
+    if (!v) {
         r = -EINVAL;
         goto out;
     }
 
-    v = qobject_input_visitor_new_keyval(crumpled);
     visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
     visit_free(v);
-    qobject_unref(crumpled);
 
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 29e3e1eaaa..665b1763eb 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -539,19 +539,17 @@ static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s,
 static SocketAddress *sd_server_config(QDict *options, Error **errp)
 {
     QDict *server = NULL;
-    QObject *crumpled_server = NULL;
     Visitor *iv = NULL;
     SocketAddress *saddr = NULL;
     Error *local_err = NULL;
 
     qdict_extract_subqdict(options, &server, "server.");
 
-    crumpled_server = qdict_crumple_for_keyval_qiv(server, errp);
-    if (!crumpled_server) {
+    iv = qobject_input_visitor_new_flat_confused(server, errp);
+    if (!iv) {
         goto done;
     }
 
-    iv = qobject_input_visitor_new_keyval(crumpled_server);
     visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -560,7 +558,6 @@ static SocketAddress *sd_server_config(QDict *options, Error **errp)
 
 done:
     visit_free(iv);
-    qobject_unref(crumpled_server);
     qobject_unref(server);
     return saddr;
 }
@@ -2173,7 +2170,6 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts,
 {
     BlockdevCreateOptions *create_options = NULL;
     QDict *qdict, *location_qdict;
-    QObject *crumpled;
     Visitor *v;
     char *redundancy;
     Error *local_err = NULL;
@@ -2209,16 +2205,14 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts,
     }
 
     /* Get the QAPI object */
-    crumpled = qdict_crumple_for_keyval_qiv(qdict, errp);
-    if (crumpled == NULL) {
+    v = qobject_input_visitor_new_flat_confused(qdict, errp);
+    if (!v) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(crumpled);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
-    qobject_unref(crumpled);
 
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/block/ssh.c b/block/ssh.c
index bd85d989d5..da7bbf73e2 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -606,7 +606,6 @@ static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp)
     BlockdevOptionsSsh *result = NULL;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
-    QObject *crumpled;
     const QDictEntry *e;
     Visitor *v;
 
@@ -623,15 +622,13 @@ static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp)
     }
 
     /* Create the QAPI object */
-    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
-    if (crumpled == NULL) {
+    v = qobject_input_visitor_new_flat_confused(options, errp);
+    if (!v) {
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(crumpled);
     visit_type_BlockdevOptionsSsh(v, NULL, &result, &local_err);
     visit_free(v);
-    qobject_unref(crumpled);
 
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/block/vhdx.c b/block/vhdx.c
index f2aec3d2cd..a677703a9e 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1966,7 +1966,6 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename,
 {
     BlockdevCreateOptions *create_options = NULL;
     QDict *qdict;
-    QObject *qobj;
     Visitor *v;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
@@ -2005,14 +2004,12 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "vhdx");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    if (!qobj) {
+    v = qobject_input_visitor_new_flat_confused(qdict, errp);
+    if (!v) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(qobj);
-    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/block/vpc.c b/block/vpc.c
index a9bb04149d..bf294abfa7 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1082,7 +1082,6 @@ static int coroutine_fn vpc_co_create_opts(const char *filename,
 {
     BlockdevCreateOptions *create_options = NULL;
     QDict *qdict;
-    QObject *qobj;
     Visitor *v;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
@@ -1119,14 +1118,12 @@ static int coroutine_fn vpc_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "vpc");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-    if (!qobj) {
+    v = qobject_input_visitor_new_flat_confused(qdict, errp);
+    if (!v) {
         ret = -EINVAL;
         goto fail;
     }
 
-    v = qobject_input_visitor_new_keyval(qobj);
-    qobject_unref(qobj);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
diff --git a/include/block/qdict.h b/include/block/qdict.h
index 47d9638c37..d8cb502d7d 100644
--- a/include/block/qdict.h
+++ b/include/block/qdict.h
@@ -21,7 +21,6 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
 void qdict_array_split(QDict *src, QList **dst);
 int qdict_array_entries(QDict *src, const char *subqdict);
 QObject *qdict_crumple(const QDict *src, Error **errp);
-QObject *qdict_crumple_for_keyval_qiv(QDict *qdict, Error **errp);
 void qdict_flatten(QDict *qdict);
 
 typedef struct QDictRenames {
@@ -30,4 +29,6 @@ typedef struct QDictRenames {
 } QDictRenames;
 bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp);
 
+Visitor *qobject_input_visitor_new_flat_confused(QDict *qdict,
+                                                 Error **errp);
 #endif
diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index aba372c2eb..41f39abc4a 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -13,6 +13,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qnum.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
 
@@ -529,7 +530,7 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
  * used for anything else, and it should go away once the block
  * subsystem has been cleaned up.
  */
-QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp)
+static QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp)
 {
     QDict *tmp = NULL;
     char *buf;
@@ -695,3 +696,28 @@ bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp)
     }
     return true;
 }
+
+/*
+ * Create a QObject input visitor for flat @qdict with possibly
+ * confused scalar types.
+ *
+ * The block subsystem uses this function to visit its flat QDict with
+ * possibly confused scalar types.  It should not be used for anything
+ * else, and it should go away once the block subsystem has been
+ * cleaned up.
+ */
+Visitor *qobject_input_visitor_new_flat_confused(QDict *qdict,
+                                                 Error **errp)
+{
+    QObject *crumpled;
+    Visitor *v;
+
+    crumpled = qdict_crumple_for_keyval_qiv(qdict, errp);
+    if (!crumpled) {
+        return NULL;
+    }
+
+    v = qobject_input_visitor_new_keyval(crumpled);
+    qobject_unref(crumpled);
+    return v;
+}
-- 
2.17.1


[Qemu-devel] [PATCH 09/18] block: Make remaining uses of qobject input visitor more robust
Posted by Markus Armbruster, 1 week ago
Remaining uses of qobject_input_visitor_new_keyval() in the block
subsystem:

* block_crypto_create_opts_init()
  Currently doesn't visit any non-string scalars, thus safe.  It's
  called from
  - block_crypto_open_luks()
    Creates the QDict with qemu_opts_to_qdict_filtered(), which
    creates only string scalars, but has a TODO asking for other types.
  - qcow_open()
  - qcow2_open(), qcow2_co_invalidate_cache(), qcow2_reopen_prepare()

* block_crypto_create_opts_init(), called from
  - block_crypto_co_create_opts_luks()
    Also creates the QDict with qemu_opts_to_qdict_filtered().

* vdi_co_create_opts()
  Also creates the QDict with qemu_opts_to_qdict_filtered().

Replace these uses by qobject_input_visitor_new_flat_confused() for
robustness.  This adds crumpling.  Right now, that's a no-op, but if
we ever extend these things in non-flat ways, crumpling will be
needed.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/crypto.c | 6 +++---
 block/vdi.c    | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index bc322b50f5..25d12e9d47 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -21,11 +21,11 @@
 #include "qemu/osdep.h"
 
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "crypto/block.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/qapi-visit-crypto.h"
-#include "qapi/qmp/qdict.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/error.h"
 #include "qemu/option.h"
@@ -159,7 +159,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
     ret = g_new0(QCryptoBlockOpenOptions, 1);
     ret->format = format;
 
-    v = qobject_input_visitor_new_keyval(QOBJECT(opts));
+    v = qobject_input_visitor_new_flat_confused(opts, &error_abort);
 
     visit_start_struct(v, NULL, NULL, 0, &local_err);
     if (local_err) {
@@ -210,7 +210,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
     ret = g_new0(QCryptoBlockCreateOptions, 1);
     ret->format = format;
 
-    v = qobject_input_visitor_new_keyval(QOBJECT(opts));
+    v = qobject_input_visitor_new_flat_confused(opts, &error_abort);
 
     visit_start_struct(v, NULL, NULL, 0, &local_err);
     if (local_err) {
diff --git a/block/vdi.c b/block/vdi.c
index 668af0a828..6b6eed126d 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -51,10 +51,10 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qdict.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
@@ -934,7 +934,7 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
     }
 
     /* Get the QAPI object */
-    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    v = qobject_input_visitor_new_flat_confused(qdict, &error_abort);
     visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
     visit_free(v);
 
-- 
2.17.1


[Qemu-devel] [PATCH 10/18] block-qdict: Simplify qdict_flatten_qdict()
Posted by Markus Armbruster, 1 week ago
There's no need to restart the loop.  We don't elsewhere, e.g. in
qdict_extract_subqdict(), qdict_join() and qemu_opts_absorb_qdict().
Simplify accordingly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/block-qdict.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index 41f39abc4a..f32df343e8 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -89,16 +89,13 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
     QObject *value;
     const QDictEntry *entry, *next;
     char *new_key;
-    bool delete;
 
     entry = qdict_first(qdict);
 
     while (entry != NULL) {
-
         next = qdict_next(qdict, entry);
         value = qdict_entry_value(entry);
         new_key = NULL;
-        delete = false;
 
         if (prefix) {
             new_key = g_strdup_printf("%s.%s", prefix, entry->key);
@@ -109,27 +106,18 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
              * itself disappears. */
             qdict_flatten_qdict(qobject_to(QDict, value), target,
                                 new_key ? new_key : entry->key);
-            delete = true;
+            qdict_del(qdict, entry->key);
         } else if (qobject_type(value) == QTYPE_QLIST) {
             qdict_flatten_qlist(qobject_to(QList, value), target,
                                 new_key ? new_key : entry->key);
-            delete = true;
+            qdict_del(qdict, entry->key);
         } else if (prefix) {
             /* All other objects are moved to the target unchanged. */
             qdict_put_obj(target, new_key, qobject_ref(value));
-            delete = true;
-        }
-
-        g_free(new_key);
-
-        if (delete) {
             qdict_del(qdict, entry->key);
-
-            /* Restart loop after modifying the iterated QDict */
-            entry = qdict_first(qdict);
-            continue;
         }
 
+        g_free(new_key);
         entry = next;
     }
 }
-- 
2.17.1


[Qemu-devel] [PATCH 11/18] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist()
Posted by Markus Armbruster, 1 week ago
qdict_flatten_qdict() skips copying scalars from @qdict to @target
when the two are the same.  Fair enough, but it uses a non-obvious
test for "same".  Replace it by the obvious one.  While there, improve
comments.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/block-qdict.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index f32df343e8..a4e1c8d08f 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -71,12 +71,15 @@ static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
         value = qlist_entry_obj(entry);
         new_key = g_strdup_printf("%s.%i", prefix, i);
 
+        /*
+         * Flatten non-empty QDict and QList recursively into @target,
+         * copy other objects to @target
+         */
         if (qobject_type(value) == QTYPE_QDICT) {
             qdict_flatten_qdict(qobject_to(QDict, value), target, new_key);
         } else if (qobject_type(value) == QTYPE_QLIST) {
             qdict_flatten_qlist(qobject_to(QList, value), target, new_key);
         } else {
-            /* All other types are moved to the target unchanged. */
             qdict_put_obj(target, new_key, qobject_ref(value));
         }
 
@@ -101,9 +104,11 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
             new_key = g_strdup_printf("%s.%s", prefix, entry->key);
         }
 
+        /*
+         * Flatten non-empty QDict and QList recursively into @target,
+         * copy other objects to @target
+         */
         if (qobject_type(value) == QTYPE_QDICT) {
-            /* Entries of QDicts are processed recursively, the QDict object
-             * itself disappears. */
             qdict_flatten_qdict(qobject_to(QDict, value), target,
                                 new_key ? new_key : entry->key);
             qdict_del(qdict, entry->key);
@@ -111,8 +116,7 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
             qdict_flatten_qlist(qobject_to(QList, value), target,
                                 new_key ? new_key : entry->key);
             qdict_del(qdict, entry->key);
-        } else if (prefix) {
-            /* All other objects are moved to the target unchanged. */
+        } else if (target != qdict) {
             qdict_put_obj(target, new_key, qobject_ref(value));
             qdict_del(qdict, entry->key);
         }
-- 
2.17.1


[Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit
Posted by Markus Armbruster, 1 week ago
When you mix scalar and non-scalar keys, whether you get an "already
set as scalar" or an "already set as dict" error depends on qdict
iteration order.  Neither message makes much sense.  Replace by
""Cannot mix scalar and non-scalar keys".  This is similar to the
message we get for mixing list and non-list keys.

I find qdict_crumple()'s first loop hard to understand.  Rearrange it
and add a comment.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/block-qdict.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index a4e1c8d08f..35e9052816 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
 QObject *qdict_crumple(const QDict *src, Error **errp)
 {
     const QDictEntry *ent;
-    QDict *two_level, *multi_level = NULL;
+    QDict *two_level, *multi_level = NULL, *child_dict;
     QObject *dst = NULL, *child;
     size_t i;
     char *prefix = NULL;
@@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
         }
 
         qdict_split_flat_key(ent->key, &prefix, &suffix);
-
         child = qdict_get(two_level, prefix);
+        child_dict = qobject_to(QDict, child);
+
+        if (child) {
+            /*
+             * An existing child must be a dict and @ent must be a
+             * dict member (i.e. suffix not null), or else @ent
+             * clashes.
+             */
+            if (!child_dict || !suffix) {
+                error_setg(errp,
+                           "Cannot mix scalar and non-scalar keys");
+                goto error;
+            }
+        } else if (suffix) {
+            child_dict = qdict_new();
+            qdict_put(two_level, prefix, child_dict);
+        } else {
+            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
+        }
+
         if (suffix) {
-            QDict *child_dict = qobject_to(QDict, child);
-            if (!child_dict) {
-                if (child) {
-                    error_setg(errp, "Key %s prefix is already set as a scalar",
-                               prefix);
-                    goto error;
-                }
-
-                child_dict = qdict_new();
-                qdict_put_obj(two_level, prefix, QOBJECT(child_dict));
-            }
-
             qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
-        } else {
-            if (child) {
-                error_setg(errp, "Key %s prefix is already set as a dict",
-                           prefix);
-                goto error;
-            }
-            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
         }
 
         g_free(prefix);
-- 
2.17.1


Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit
Posted by Kevin Wolf, 1 week ago
Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> When you mix scalar and non-scalar keys, whether you get an "already
> set as scalar" or an "already set as dict" error depends on qdict
> iteration order.  Neither message makes much sense.  Replace by
> ""Cannot mix scalar and non-scalar keys".  This is similar to the
> message we get for mixing list and non-list keys.
> 
> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
> and add a comment.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

To be honest, I found the old version of the loop more obvious.

>  qobject/block-qdict.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> index a4e1c8d08f..35e9052816 100644
> --- a/qobject/block-qdict.c
> +++ b/qobject/block-qdict.c
> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
>  QObject *qdict_crumple(const QDict *src, Error **errp)
>  {
>      const QDictEntry *ent;
> -    QDict *two_level, *multi_level = NULL;
> +    QDict *two_level, *multi_level = NULL, *child_dict;
>      QObject *dst = NULL, *child;
>      size_t i;
>      char *prefix = NULL;
> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
>          }
>  
>          qdict_split_flat_key(ent->key, &prefix, &suffix);
> -
>          child = qdict_get(two_level, prefix);
> +        child_dict = qobject_to(QDict, child);
> +
> +        if (child) {
> +            /*
> +             * An existing child must be a dict and @ent must be a
> +             * dict member (i.e. suffix not null), or else @ent
> +             * clashes.
> +             */
> +            if (!child_dict || !suffix) {
> +                error_setg(errp,
> +                           "Cannot mix scalar and non-scalar keys");
> +                goto error;
> +            }
> +        } else if (suffix) {
> +            child_dict = qdict_new();
> +            qdict_put(two_level, prefix, child_dict);
> +        } else {
> +            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> +        }

At least, can you please move the else branch to the if below so that
the addition of the new entry is symmetrical for both scalars and dicts?
As the code is, it mixes the conflict check, creation of the child dict
and addition of scalars (but not to the child dict) in a weird way in a
single if block.

Or maybe organise it like this:

if (child && !(child_dict && suffix)) {
    error
}

if (suffix) {
    if (!child_dict) {
        create it
        add it to two_level
    }
    add entry to child_dict
} else {
    add entry to two_level
}

Kevin

Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit
Posted by Markus Armbruster, 1 week ago
Kevin Wolf <kwolf@redhat.com> writes:

> Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
>> When you mix scalar and non-scalar keys, whether you get an "already
>> set as scalar" or an "already set as dict" error depends on qdict
>> iteration order.  Neither message makes much sense.  Replace by
>> ""Cannot mix scalar and non-scalar keys".  This is similar to the
>> message we get for mixing list and non-list keys.
>> 
>> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
>> and add a comment.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> To be honest, I found the old version of the loop more obvious.
>
>>  qobject/block-qdict.c | 42 +++++++++++++++++++++---------------------
>>  1 file changed, 21 insertions(+), 21 deletions(-)
>> 
>> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
>> index a4e1c8d08f..35e9052816 100644
>> --- a/qobject/block-qdict.c
>> +++ b/qobject/block-qdict.c
>> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
>>  QObject *qdict_crumple(const QDict *src, Error **errp)
>>  {
>>      const QDictEntry *ent;
>> -    QDict *two_level, *multi_level = NULL;
>> +    QDict *two_level, *multi_level = NULL, *child_dict;
>>      QObject *dst = NULL, *child;
>>      size_t i;
>>      char *prefix = NULL;
>> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
>>          }
>>  
>>          qdict_split_flat_key(ent->key, &prefix, &suffix);
>> -
>>          child = qdict_get(two_level, prefix);
>> +        child_dict = qobject_to(QDict, child);
>> +
>> +        if (child) {
>> +            /*
>> +             * An existing child must be a dict and @ent must be a
>> +             * dict member (i.e. suffix not null), or else @ent
>> +             * clashes.
>> +             */
>> +            if (!child_dict || !suffix) {
>> +                error_setg(errp,
>> +                           "Cannot mix scalar and non-scalar keys");
>> +                goto error;
>> +            }
>> +        } else if (suffix) {
>> +            child_dict = qdict_new();
>> +            qdict_put(two_level, prefix, child_dict);
>> +        } else {
>> +            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
>> +        }
>
> At least, can you please move the else branch to the if below so that
> the addition of the new entry is symmetrical for both scalars and dicts?
> As the code is, it mixes the conflict check, creation of the child dict
> and addition of scalars (but not to the child dict) in a weird way in a
> single if block.
>
> Or maybe organise it like this:
>
> if (child && !(child_dict && suffix)) {
>     error
> }
>
> if (suffix) {
>     if (!child_dict) {
>         create it
>         add it to two_level
>     }
>     add entry to child_dict
> } else {
>     add entry to two_level
> }

Fleshing out...

        if (child && !child_dict) {
            /*
             * @prefix already exists and it's a non-dictionary,
             * i.e. we've seen a scalar with key @prefix.  The same
             * key can't occur twice, therefore suffix must be
             * non-null.
             */
            assert(suffix);
            /*
             * @ent has key @prefix.@suffix, but we've already seen
             * key @prefix: clash.
             */
            error_setg(errp, "Cannot mix scalar and non-scalar keys");
            goto error;
        }

        if (suffix) {
            if (!child_dict) {
                child_dict = qdict_new();
                qdict_put(two_level, prefix, child_dict);
            }
            qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
        } else {
            assert(!child);
            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
        }

Okay?

Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit
Posted by Kevin Wolf, 1 week ago
Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> >> When you mix scalar and non-scalar keys, whether you get an "already
> >> set as scalar" or an "already set as dict" error depends on qdict
> >> iteration order.  Neither message makes much sense.  Replace by
> >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
> >> message we get for mixing list and non-list keys.
> >> 
> >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
> >> and add a comment.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > To be honest, I found the old version of the loop more obvious.
> >
> >>  qobject/block-qdict.c | 42 +++++++++++++++++++++---------------------
> >>  1 file changed, 21 insertions(+), 21 deletions(-)
> >> 
> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> >> index a4e1c8d08f..35e9052816 100644
> >> --- a/qobject/block-qdict.c
> >> +++ b/qobject/block-qdict.c
> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
> >>  QObject *qdict_crumple(const QDict *src, Error **errp)
> >>  {
> >>      const QDictEntry *ent;
> >> -    QDict *two_level, *multi_level = NULL;
> >> +    QDict *two_level, *multi_level = NULL, *child_dict;
> >>      QObject *dst = NULL, *child;
> >>      size_t i;
> >>      char *prefix = NULL;
> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
> >>          }
> >>  
> >>          qdict_split_flat_key(ent->key, &prefix, &suffix);
> >> -
> >>          child = qdict_get(two_level, prefix);
> >> +        child_dict = qobject_to(QDict, child);
> >> +
> >> +        if (child) {
> >> +            /*
> >> +             * An existing child must be a dict and @ent must be a
> >> +             * dict member (i.e. suffix not null), or else @ent
> >> +             * clashes.
> >> +             */
> >> +            if (!child_dict || !suffix) {
> >> +                error_setg(errp,
> >> +                           "Cannot mix scalar and non-scalar keys");
> >> +                goto error;
> >> +            }
> >> +        } else if (suffix) {
> >> +            child_dict = qdict_new();
> >> +            qdict_put(two_level, prefix, child_dict);
> >> +        } else {
> >> +            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> >> +        }
> >
> > At least, can you please move the else branch to the if below so that
> > the addition of the new entry is symmetrical for both scalars and dicts?
> > As the code is, it mixes the conflict check, creation of the child dict
> > and addition of scalars (but not to the child dict) in a weird way in a
> > single if block.
> >
> > Or maybe organise it like this:
> >
> > if (child && !(child_dict && suffix)) {
> >     error
> > }
> >
> > if (suffix) {
> >     if (!child_dict) {
> >         create it
> >         add it to two_level
> >     }
> >     add entry to child_dict
> > } else {
> >     add entry to two_level
> > }
> 
> Fleshing out...
> 
>         if (child && !child_dict) {
>             /*
>              * @prefix already exists and it's a non-dictionary,
>              * i.e. we've seen a scalar with key @prefix.  The same
>              * key can't occur twice, therefore suffix must be
>              * non-null.
>              */
>             assert(suffix);
>             /*
>              * @ent has key @prefix.@suffix, but we've already seen
>              * key @prefix: clash.
>              */
>             error_setg(errp, "Cannot mix scalar and non-scalar keys");
>             goto error;
>         }

This catches "foo.bar" after "foo", but not "foo" after "foo.bar".

>         if (suffix) {
>             if (!child_dict) {
>                 child_dict = qdict_new();
>                 qdict_put(two_level, prefix, child_dict);
>             }
>             qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
>         } else {
>             assert(!child);

So "foo" after "foo.bar" would fail this assertion.

>             qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
>         }
> 
> Okay?

Kevin

Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit
Posted by Daniel P. Berrangé, 1 week ago
On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote:
> Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
> > Kevin Wolf <kwolf@redhat.com> writes:
> > 
> > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> > >> When you mix scalar and non-scalar keys, whether you get an "already
> > >> set as scalar" or an "already set as dict" error depends on qdict
> > >> iteration order.  Neither message makes much sense.  Replace by
> > >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
> > >> message we get for mixing list and non-list keys.
> > >> 
> > >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
> > >> and add a comment.
> > >> 
> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > >
> > > To be honest, I found the old version of the loop more obvious.
> > >
> > >>  qobject/block-qdict.c | 42 +++++++++++++++++++++---------------------
> > >>  1 file changed, 21 insertions(+), 21 deletions(-)
> > >> 
> > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> > >> index a4e1c8d08f..35e9052816 100644
> > >> --- a/qobject/block-qdict.c
> > >> +++ b/qobject/block-qdict.c
> > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
> > >>  QObject *qdict_crumple(const QDict *src, Error **errp)
> > >>  {
> > >>      const QDictEntry *ent;
> > >> -    QDict *two_level, *multi_level = NULL;
> > >> +    QDict *two_level, *multi_level = NULL, *child_dict;
> > >>      QObject *dst = NULL, *child;
> > >>      size_t i;
> > >>      char *prefix = NULL;
> > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
> > >>          }
> > >>  
> > >>          qdict_split_flat_key(ent->key, &prefix, &suffix);
> > >> -
> > >>          child = qdict_get(two_level, prefix);
> > >> +        child_dict = qobject_to(QDict, child);
> > >> +
> > >> +        if (child) {
> > >> +            /*
> > >> +             * An existing child must be a dict and @ent must be a
> > >> +             * dict member (i.e. suffix not null), or else @ent
> > >> +             * clashes.
> > >> +             */
> > >> +            if (!child_dict || !suffix) {
> > >> +                error_setg(errp,
> > >> +                           "Cannot mix scalar and non-scalar keys");
> > >> +                goto error;
> > >> +            }
> > >> +        } else if (suffix) {
> > >> +            child_dict = qdict_new();
> > >> +            qdict_put(two_level, prefix, child_dict);
> > >> +        } else {
> > >> +            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> > >> +        }
> > >
> > > At least, can you please move the else branch to the if below so that
> > > the addition of the new entry is symmetrical for both scalars and dicts?
> > > As the code is, it mixes the conflict check, creation of the child dict
> > > and addition of scalars (but not to the child dict) in a weird way in a
> > > single if block.
> > >
> > > Or maybe organise it like this:
> > >
> > > if (child && !(child_dict && suffix)) {
> > >     error
> > > }
> > >
> > > if (suffix) {
> > >     if (!child_dict) {
> > >         create it
> > >         add it to two_level
> > >     }
> > >     add entry to child_dict
> > > } else {
> > >     add entry to two_level
> > > }
> > 
> > Fleshing out...
> > 
> >         if (child && !child_dict) {
> >             /*
> >              * @prefix already exists and it's a non-dictionary,
> >              * i.e. we've seen a scalar with key @prefix.  The same
> >              * key can't occur twice, therefore suffix must be
> >              * non-null.
> >              */
> >             assert(suffix);
> >             /*
> >              * @ent has key @prefix.@suffix, but we've already seen
> >              * key @prefix: clash.
> >              */
> >             error_setg(errp, "Cannot mix scalar and non-scalar keys");
> >             goto error;
> >         }
> 
> This catches "foo.bar" after "foo", but not "foo" after "foo.bar".
> 
> >         if (suffix) {
> >             if (!child_dict) {
> >                 child_dict = qdict_new();
> >                 qdict_put(two_level, prefix, child_dict);
> >             }
> >             qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
> >         } else {
> >             assert(!child);
> 
> So "foo" after "foo.bar" would fail this assertion.

We got coverage of bad inputs in tests/check-qdict.c

If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios
we should extend it to make sure we detect them during testing


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit
Posted by Markus Armbruster, 1 week ago
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote:
>> Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
>> > Kevin Wolf <kwolf@redhat.com> writes:
>> > 
>> > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
>> > >> When you mix scalar and non-scalar keys, whether you get an "already
>> > >> set as scalar" or an "already set as dict" error depends on qdict
>> > >> iteration order.  Neither message makes much sense.  Replace by
>> > >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
>> > >> message we get for mixing list and non-list keys.
>> > >> 
>> > >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
>> > >> and add a comment.
>> > >> 
>> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > >
>> > > To be honest, I found the old version of the loop more obvious.
>> > >
>> > >>  qobject/block-qdict.c | 42 +++++++++++++++++++++---------------------
>> > >>  1 file changed, 21 insertions(+), 21 deletions(-)
>> > >> 
>> > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
>> > >> index a4e1c8d08f..35e9052816 100644
>> > >> --- a/qobject/block-qdict.c
>> > >> +++ b/qobject/block-qdict.c
>> > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
>> > >>  QObject *qdict_crumple(const QDict *src, Error **errp)
>> > >>  {
>> > >>      const QDictEntry *ent;
>> > >> -    QDict *two_level, *multi_level = NULL;
>> > >> +    QDict *two_level, *multi_level = NULL, *child_dict;
>> > >>      QObject *dst = NULL, *child;
>> > >>      size_t i;
>> > >>      char *prefix = NULL;
>> > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
>> > >>          }
>> > >>  
>> > >>          qdict_split_flat_key(ent->key, &prefix, &suffix);
>> > >> -
>> > >>          child = qdict_get(two_level, prefix);
>> > >> +        child_dict = qobject_to(QDict, child);
>> > >> +
>> > >> +        if (child) {
>> > >> +            /*
>> > >> +             * An existing child must be a dict and @ent must be a
>> > >> +             * dict member (i.e. suffix not null), or else @ent
>> > >> +             * clashes.
>> > >> +             */
>> > >> +            if (!child_dict || !suffix) {
>> > >> +                error_setg(errp,
>> > >> +                           "Cannot mix scalar and non-scalar keys");
>> > >> +                goto error;
>> > >> +            }
>> > >> +        } else if (suffix) {
>> > >> +            child_dict = qdict_new();
>> > >> +            qdict_put(two_level, prefix, child_dict);
>> > >> +        } else {
>> > >> +            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
>> > >> +        }
>> > >
>> > > At least, can you please move the else branch to the if below so that
>> > > the addition of the new entry is symmetrical for both scalars and dicts?
>> > > As the code is, it mixes the conflict check, creation of the child dict
>> > > and addition of scalars (but not to the child dict) in a weird way in a
>> > > single if block.
>> > >
>> > > Or maybe organise it like this:
>> > >
>> > > if (child && !(child_dict && suffix)) {
>> > >     error
>> > > }
>> > >
>> > > if (suffix) {
>> > >     if (!child_dict) {
>> > >         create it
>> > >         add it to two_level
>> > >     }
>> > >     add entry to child_dict
>> > > } else {
>> > >     add entry to two_level
>> > > }
>> > 
>> > Fleshing out...
>> > 
>> >         if (child && !child_dict) {
>> >             /*
>> >              * @prefix already exists and it's a non-dictionary,
>> >              * i.e. we've seen a scalar with key @prefix.  The same
>> >              * key can't occur twice, therefore suffix must be
>> >              * non-null.
>> >              */
>> >             assert(suffix);
>> >             /*
>> >              * @ent has key @prefix.@suffix, but we've already seen
>> >              * key @prefix: clash.
>> >              */
>> >             error_setg(errp, "Cannot mix scalar and non-scalar keys");
>> >             goto error;
>> >         }
>> 
>> This catches "foo.bar" after "foo", but not "foo" after "foo.bar".
>> 
>> >         if (suffix) {
>> >             if (!child_dict) {
>> >                 child_dict = qdict_new();
>> >                 qdict_put(two_level, prefix, child_dict);
>> >             }
>> >             qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
>> >         } else {
>> >             assert(!child);
>> 
>> So "foo" after "foo.bar" would fail this assertion.
>
> We got coverage of bad inputs in tests/check-qdict.c
>
> If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios
> we should extend it to make sure we detect them during testing

Depends on qdict_first() / qdict_next() traversal order, which makes it
awkward.

Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit
Posted by Markus Armbruster, 1 week ago
Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
>> >> When you mix scalar and non-scalar keys, whether you get an "already
>> >> set as scalar" or an "already set as dict" error depends on qdict
>> >> iteration order.  Neither message makes much sense.  Replace by
>> >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
>> >> message we get for mixing list and non-list keys.
>> >> 
>> >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
>> >> and add a comment.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > To be honest, I found the old version of the loop more obvious.
>> >
>> >>  qobject/block-qdict.c | 42 +++++++++++++++++++++---------------------
>> >>  1 file changed, 21 insertions(+), 21 deletions(-)
>> >> 
>> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
>> >> index a4e1c8d08f..35e9052816 100644
>> >> --- a/qobject/block-qdict.c
>> >> +++ b/qobject/block-qdict.c
>> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
>> >>  QObject *qdict_crumple(const QDict *src, Error **errp)
>> >>  {
>> >>      const QDictEntry *ent;
>> >> -    QDict *two_level, *multi_level = NULL;
>> >> +    QDict *two_level, *multi_level = NULL, *child_dict;
>> >>      QObject *dst = NULL, *child;
>> >>      size_t i;
>> >>      char *prefix = NULL;
>> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
>> >>          }
>> >>  
>> >>          qdict_split_flat_key(ent->key, &prefix, &suffix);
>> >> -
>> >>          child = qdict_get(two_level, prefix);
>> >> +        child_dict = qobject_to(QDict, child);
>> >> +
>> >> +        if (child) {
>> >> +            /*
>> >> +             * An existing child must be a dict and @ent must be a
>> >> +             * dict member (i.e. suffix not null), or else @ent
>> >> +             * clashes.
>> >> +             */
>> >> +            if (!child_dict || !suffix) {
>> >> +                error_setg(errp,
>> >> +                           "Cannot mix scalar and non-scalar keys");
>> >> +                goto error;
>> >> +            }
>> >> +        } else if (suffix) {
>> >> +            child_dict = qdict_new();
>> >> +            qdict_put(two_level, prefix, child_dict);
>> >> +        } else {
>> >> +            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
>> >> +        }
>> >
>> > At least, can you please move the else branch to the if below so that
>> > the addition of the new entry is symmetrical for both scalars and dicts?
>> > As the code is, it mixes the conflict check, creation of the child dict
>> > and addition of scalars (but not to the child dict) in a weird way in a
>> > single if block.
>> >
>> > Or maybe organise it like this:
>> >
>> > if (child && !(child_dict && suffix)) {
>> >     error
>> > }
>> >
>> > if (suffix) {
>> >     if (!child_dict) {
>> >         create it
>> >         add it to two_level
>> >     }
>> >     add entry to child_dict
>> > } else {
>> >     add entry to two_level
>> > }
>> 
>> Fleshing out...
>> 
>>         if (child && !child_dict) {
>>             /*
>>              * @prefix already exists and it's a non-dictionary,
>>              * i.e. we've seen a scalar with key @prefix.  The same
>>              * key can't occur twice, therefore suffix must be
>>              * non-null.
>>              */
>>             assert(suffix);
>>             /*
>>              * @ent has key @prefix.@suffix, but we've already seen
>>              * key @prefix: clash.
>>              */
>>             error_setg(errp, "Cannot mix scalar and non-scalar keys");
>>             goto error;
>>         }
>
> This catches "foo.bar" after "foo", but not "foo" after "foo.bar".
>
>>         if (suffix) {
>>             if (!child_dict) {
>>                 child_dict = qdict_new();
>>                 qdict_put(two_level, prefix, child_dict);
>>             }
>>             qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
>>         } else {
>>             assert(!child);
>
> So "foo" after "foo.bar" would fail this assertion.
>
>>             qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
>>         }
>> 
>> Okay?
>
> Kevin

I feel dumb.  Next try:

        if (child) {
            /*
             * If @child_dict, then all previous keys with this prefix
             * had a suffix.  If @suffix, this one has one as well,
             * and we're good, else there's a clash.
             */
            if (!child_dict || !suffix) {
                error_setg(errp, "Cannot mix scalar and non-scalar keys");
                goto error;
            }
        }

        if (suffix) {
            if (!child_dict) {
                child_dict = qdict_new();
                qdict_put(two_level, prefix, child_dict);
            }
            qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
        } else {
            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
        }

Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit
Posted by Kevin Wolf, 1 week ago
Am 14.06.2018 um 13:52 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> >> >> When you mix scalar and non-scalar keys, whether you get an "already
> >> >> set as scalar" or an "already set as dict" error depends on qdict
> >> >> iteration order.  Neither message makes much sense.  Replace by
> >> >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
> >> >> message we get for mixing list and non-list keys.
> >> >> 
> >> >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
> >> >> and add a comment.
> >> >> 
> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> >
> >> > To be honest, I found the old version of the loop more obvious.
> >> >
> >> >>  qobject/block-qdict.c | 42 +++++++++++++++++++++---------------------
> >> >>  1 file changed, 21 insertions(+), 21 deletions(-)
> >> >> 
> >> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> >> >> index a4e1c8d08f..35e9052816 100644
> >> >> --- a/qobject/block-qdict.c
> >> >> +++ b/qobject/block-qdict.c
> >> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
> >> >>  QObject *qdict_crumple(const QDict *src, Error **errp)
> >> >>  {
> >> >>      const QDictEntry *ent;
> >> >> -    QDict *two_level, *multi_level = NULL;
> >> >> +    QDict *two_level, *multi_level = NULL, *child_dict;
> >> >>      QObject *dst = NULL, *child;
> >> >>      size_t i;
> >> >>      char *prefix = NULL;
> >> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
> >> >>          }
> >> >>  
> >> >>          qdict_split_flat_key(ent->key, &prefix, &suffix);
> >> >> -
> >> >>          child = qdict_get(two_level, prefix);
> >> >> +        child_dict = qobject_to(QDict, child);
> >> >> +
> >> >> +        if (child) {
> >> >> +            /*
> >> >> +             * An existing child must be a dict and @ent must be a
> >> >> +             * dict member (i.e. suffix not null), or else @ent
> >> >> +             * clashes.
> >> >> +             */
> >> >> +            if (!child_dict || !suffix) {
> >> >> +                error_setg(errp,
> >> >> +                           "Cannot mix scalar and non-scalar keys");
> >> >> +                goto error;
> >> >> +            }
> >> >> +        } else if (suffix) {
> >> >> +            child_dict = qdict_new();
> >> >> +            qdict_put(two_level, prefix, child_dict);
> >> >> +        } else {
> >> >> +            qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> >> >> +        }
> >> >
> >> > At least, can you please move the else branch to the if below so that
> >> > the addition of the new entry is symmetrical for both scalars and dicts?
> >> > As the code is, it mixes the conflict check, creation of the child dict
> >> > and addition of scalars (but not to the child dict) in a weird way in a
> >> > single if block.
> >> >
> >> > Or maybe organise it like this:
> >> >
> >> > if (child && !(child_dict && suffix)) {
> >> >     error
> >> > }
> >> >
> >> > if (suffix) {
> >> >     if (!child_dict) {
> >> >         create it
> >> >         add it to two_level
> >> >     }
> >> >     add entry to child_dict
> >> > } else {
> >> >     add entry to two_level
> >> > }
> >> 
> >> Fleshing out...
> >> 
> >>         if (child && !child_dict) {
> >>             /*
> >>              * @prefix already exists and it's a non-dictionary,
> >>              * i.e. we've seen a scalar with key @prefix.  The same
> >>              * key can't occur twice, therefore suffix must be
> >>              * non-null.
> >>              */
> >>             assert(suffix);
> >>             /*
> >>              * @ent has key @prefix.@suffix, but we've already seen
> >>              * key @prefix: clash.
> >>              */
> >>             error_setg(errp, "Cannot mix scalar and non-scalar keys");
> >>             goto error;
> >>         }
> >
> > This catches "foo.bar" after "foo", but not "foo" after "foo.bar".
> >
> >>         if (suffix) {
> >>             if (!child_dict) {
> >>                 child_dict = qdict_new();
> >>                 qdict_put(two_level, prefix, child_dict);
> >>             }
> >>             qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
> >>         } else {
> >>             assert(!child);
> >
> > So "foo" after "foo.bar" would fail this assertion.
> >
> >>             qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> >>         }
> >> 
> >> Okay?
> >
> > Kevin
> 
> I feel dumb.  Next try:
> 
>         if (child) {
>             /*
>              * If @child_dict, then all previous keys with this prefix
>              * had a suffix.  If @suffix, this one has one as well,
>              * and we're good, else there's a clash.
>              */
>             if (!child_dict || !suffix) {
>                 error_setg(errp, "Cannot mix scalar and non-scalar keys");
>                 goto error;
>             }
>         }
> 
>         if (suffix) {
>             if (!child_dict) {
>                 child_dict = qdict_new();
>                 qdict_put(two_level, prefix, child_dict);
>             }
>             qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
>         } else {
>             qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
>         }

This one looks good to me. (Though I feel your comments explains the
!(child_dict && suffix) form that I chose rather than what you actually
have in the code. But I don't mind.)

Kevin

[Qemu-devel] [PATCH 13/18] block-qdict: Simplify qdict_is_list() some
Posted by Markus Armbruster, 1 week ago
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/block-qdict.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index 35e9052816..f24bea30e1 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -317,27 +317,22 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
 
     for (ent = qdict_first(maybe_list); ent != NULL;
          ent = qdict_next(maybe_list, ent)) {
+        int is_index = !qemu_strtoi64(ent->key, NULL, 10, &val);
 
-        if (qemu_strtoi64(ent->key, NULL, 10, &val) == 0) {
-            if (is_list == -1) {
-                is_list = 1;
-            } else if (!is_list) {
-                error_setg(errp,
-                           "Cannot mix list and non-list keys");
-                return -1;
-            }
+        if (is_list == -1) {
+            is_list = is_index;
+        }
+
+        if (is_index != is_list) {
+            error_setg(errp, "Cannot mix list and non-list keys");
+            return -1;
+        }
+
+        if (is_index) {
             len++;
             if (val > max) {
                 max = val;
             }
-        } else {
-            if (is_list == -1) {
-                is_list = 0;
-            } else if (is_list) {
-                error_setg(errp,
-                           "Cannot mix list and non-list keys");
-                return -1;
-            }
         }
     }
 
-- 
2.17.1


[Qemu-devel] [PATCH 14/18] check-block-qdict: Rename qdict_flatten()'s variables for clarity
Posted by Markus Armbruster, 1 week ago
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-block-qdict.c | 57 ++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 5b9f4d506e..29f58a2d3d 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -37,11 +37,11 @@ static void qdict_defaults_test(void)
 
 static void qdict_flatten_test(void)
 {
-    QList *list1 = qlist_new();
-    QList *list2 = qlist_new();
-    QDict *dict1 = qdict_new();
-    QDict *dict2 = qdict_new();
-    QDict *dict3 = qdict_new();
+    QList *e_1 = qlist_new();
+    QList *e = qlist_new();
+    QDict *e_1_2 = qdict_new();
+    QDict *f = qdict_new();
+    QDict *root = qdict_new();
 
     /*
      * Test the flattening of
@@ -79,35 +79,36 @@ static void qdict_flatten_test(void)
      * }
      */
 
-    qdict_put_int(dict1, "a", 0);
-    qdict_put_int(dict1, "b", 1);
+    qdict_put_int(e_1_2, "a", 0);
+    qdict_put_int(e_1_2, "b", 1);
 
-    qlist_append_int(list1, 23);
-    qlist_append_int(list1, 66);
-    qlist_append(list1, dict1);
-    qlist_append_int(list2, 42);
-    qlist_append(list2, list1);
+    qlist_append_int(e_1, 23);
+    qlist_append_int(e_1, 66);
+    qlist_append(e_1, e_1_2);
+    qlist_append_int(e, 42);
+    qlist_append(e, e_1);
 
-    qdict_put_int(dict2, "c", 2);
-    qdict_put_int(dict2, "d", 3);
-    qdict_put(dict3, "e", list2);
-    qdict_put(dict3, "f", dict2);
-    qdict_put_int(dict3, "g", 4);
+    qdict_put_int(f, "c", 2);
+    qdict_put_int(f, "d", 3);
 
-    qdict_flatten(dict3);
+    qdict_put(root, "e", e);
+    qdict_put(root, "f", f);
+    qdict_put_int(root, "g", 4);
 
-    g_assert(qdict_get_int(dict3, "e.0") == 42);
-    g_assert(qdict_get_int(dict3, "e.1.0") == 23);
-    g_assert(qdict_get_int(dict3, "e.1.1") == 66);
-    g_assert(qdict_get_int(dict3, "e.1.2.a") == 0);
-    g_assert(qdict_get_int(dict3, "e.1.2.b") == 1);
-    g_assert(qdict_get_int(dict3, "f.c") == 2);
-    g_assert(qdict_get_int(dict3, "f.d") == 3);
-    g_assert(qdict_get_int(dict3, "g") == 4);
+    qdict_flatten(root);
 
-    g_assert(qdict_size(dict3) == 8);
+    g_assert(qdict_get_int(root, "e.0") == 42);
+    g_assert(qdict_get_int(root, "e.1.0") == 23);
+    g_assert(qdict_get_int(root, "e.1.1") == 66);
+    g_assert(qdict_get_int(root, "e.1.2.a") == 0);
+    g_assert(qdict_get_int(root, "e.1.2.b") == 1);
+    g_assert(qdict_get_int(root, "f.c") == 2);
+    g_assert(qdict_get_int(root, "f.d") == 3);
+    g_assert(qdict_get_int(root, "g") == 4);
 
-    qobject_unref(dict3);
+    g_assert(qdict_size(root) == 8);
+
+    qobject_unref(root);
 }
 
 static void qdict_array_split_test(void)
-- 
2.17.1


[Qemu-devel] [PATCH 15/18] check-block-qdict: Cover flattening of empty lists and dictionaries
Posted by Markus Armbruster, 1 week ago
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-block-qdict.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 29f58a2d3d..2da16f01a6 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -41,6 +41,8 @@ static void qdict_flatten_test(void)
     QList *e = qlist_new();
     QDict *e_1_2 = qdict_new();
     QDict *f = qdict_new();
+    QList *y = qlist_new();
+    QDict *z = qdict_new();
     QDict *root = qdict_new();
 
     /*
@@ -62,7 +64,9 @@ static void qdict_flatten_test(void)
      *         "c": 2,
      *         "d": 3,
      *     },
-     *     "g": 4
+     *     "g": 4,
+     *     "y": [{}],
+     *     "z": {"a": []}
      * }
      *
      * to
@@ -77,6 +81,8 @@ static void qdict_flatten_test(void)
      *     "f.d": 3,
      *     "g": 4
      * }
+     *
+     * Note that "y" and "z" get eaten.
      */
 
     qdict_put_int(e_1_2, "a", 0);
@@ -91,9 +97,15 @@ static void qdict_flatten_test(void)
     qdict_put_int(f, "c", 2);
     qdict_put_int(f, "d", 3);
 
+    qlist_append(y, qdict_new());
+
+    qdict_put(z, "a", qlist_new());
+
     qdict_put(root, "e", e);
     qdict_put(root, "f", f);
     qdict_put_int(root, "g", 4);
+    qdict_put(root, "y", y);
+    qdict_put(root, "z", z);
 
     qdict_flatten(root);
 
-- 
2.17.1


[Qemu-devel] [PATCH 16/18] block: Fix -blockdev / blockdev-add for empty objects and arrays
Posted by Markus Armbruster, 1 week ago
-blockdev and blockdev-add silently ignore empty objects and arrays in
their argument.  That's because qmp_blockdev_add() converts the
argument to a flat QDict, and qdict_flatten() eats empty QDict and
QList members.  For instance, we ignore an empty BlockdevOptions
member @cache.  No real harm, as absent means the same as empty there.

Thus, the flaw puts an artificial restriction on the QAPI schema: we
can't have potentially empty objects and arrays within
BlockdevOptions, except when they're optional and "empty" has the same
meaning as "absent".

Our QAPI schema satisfies this restriction (I checked), but it's a
trap for the unwary, and a temptation to employ awkward workarounds
for the wary.  Let's get rid of it.

Change qdict_flatten() and qdict_crumple() to treat empty dictionaries
and lists exactly like scalars.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/block-qdict.c     | 54 ++++++++++++++++++++++++---------------
 tests/check-block-qdict.c | 38 +++++++++++++++++++++------
 2 files changed, 63 insertions(+), 29 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index f24bea30e1..0b2ae02627 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -56,6 +56,8 @@ static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
 {
     QObject *value;
     const QListEntry *entry;
+    QDict *dict_val;
+    QList *list_val;
     char *new_key;
     int i;
 
@@ -69,16 +71,18 @@ static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
 
     for (i = 0; entry; entry = qlist_next(entry), i++) {
         value = qlist_entry_obj(entry);
+        dict_val = qobject_to(QDict, value);
+        list_val = qobject_to(QList, value);
         new_key = g_strdup_printf("%s.%i", prefix, i);
 
         /*
          * Flatten non-empty QDict and QList recursively into @target,
          * copy other objects to @target
          */
-        if (qobject_type(value) == QTYPE_QDICT) {
-            qdict_flatten_qdict(qobject_to(QDict, value), target, new_key);
-        } else if (qobject_type(value) == QTYPE_QLIST) {
-            qdict_flatten_qlist(qobject_to(QList, value), target, new_key);
+        if (dict_val && qdict_size(dict_val)) {
+            qdict_flatten_qdict(dict_val, target, new_key);
+        } else if (list_val && !qlist_empty(list_val)) {
+            qdict_flatten_qlist(list_val, target, new_key);
         } else {
             qdict_put_obj(target, new_key, qobject_ref(value));
         }
@@ -91,6 +95,8 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
 {
     QObject *value;
     const QDictEntry *entry, *next;
+    QDict *dict_val;
+    QList *list_val;
     char *new_key;
 
     entry = qdict_first(qdict);
@@ -98,6 +104,8 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
     while (entry != NULL) {
         next = qdict_next(qdict, entry);
         value = qdict_entry_value(entry);
+        dict_val = qobject_to(QDict, value);
+        list_val = qobject_to(QList, value);
         new_key = NULL;
 
         if (prefix) {
@@ -108,12 +116,12 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
          * Flatten non-empty QDict and QList recursively into @target,
          * copy other objects to @target
          */
-        if (qobject_type(value) == QTYPE_QDICT) {
-            qdict_flatten_qdict(qobject_to(QDict, value), target,
+        if (dict_val && qdict_size(dict_val)) {
+            qdict_flatten_qdict(dict_val, target,
                                 new_key ? new_key : entry->key);
             qdict_del(qdict, entry->key);
-        } else if (qobject_type(value) == QTYPE_QLIST) {
-            qdict_flatten_qlist(qobject_to(QList, value), target,
+        } else if (list_val && !qlist_empty(list_val)) {
+            qdict_flatten_qlist(list_val, target,
                                 new_key ? new_key : entry->key);
             qdict_del(qdict, entry->key);
         } else if (target != qdict) {
@@ -127,10 +135,11 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
 }
 
 /**
- * qdict_flatten(): For each nested QDict with key x, all fields with key y
- * are moved to this QDict and their key is renamed to "x.y". For each nested
- * QList with key x, the field at index y is moved to this QDict with the key
- * "x.y" (i.e., the reverse of what qdict_array_split() does).
+ * qdict_flatten(): For each nested non-empty QDict with key x, all
+ * fields with key y are moved to this QDict and their key is renamed
+ * to "x.y". For each nested non-empty QList with key x, the field at
+ * index y is moved to this QDict with the key "x.y" (i.e., the
+ * reverse of what qdict_array_split() does).
  * This operation is applied recursively for nested QDicts and QLists.
  */
 void qdict_flatten(QDict *qdict)
@@ -361,8 +370,8 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
  * @src: the original flat dictionary (only scalar values) to crumple
  *
  * Takes a flat dictionary whose keys use '.' separator to indicate
- * nesting, and values are scalars, and crumples it into a nested
- * structure.
+ * nesting, and values are scalars, empty dictionaries or empty lists,
+ * and crumples it into a nested structure.
  *
  * To include a literal '.' in a key name, it must be escaped as '..'
  *
@@ -399,6 +408,8 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
 {
     const QDictEntry *ent;
     QDict *two_level, *multi_level = NULL, *child_dict;
+    QDict *dict_val;
+    QList *list_val;
     QObject *dst = NULL, *child;
     size_t i;
     char *prefix = NULL;
@@ -409,10 +420,11 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
 
     /* Step 1: split our totally flat dict into a two level dict */
     for (ent = qdict_first(src); ent != NULL; ent = qdict_next(src, ent)) {
-        if (qobject_type(ent->value) == QTYPE_QDICT ||
-            qobject_type(ent->value) == QTYPE_QLIST) {
-            error_setg(errp, "Value %s is not a scalar",
-                       ent->key);
+        dict_val = qobject_to(QDict, ent->value);
+        list_val = qobject_to(QList, ent->value);
+        if ((dict_val && qdict_size(dict_val))
+            || (list_val && !qlist_empty(list_val))) {
+            error_setg(errp, "Value %s is not flat", ent->key);
             goto error;
         }
 
@@ -451,9 +463,9 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
     multi_level = qdict_new();
     for (ent = qdict_first(two_level); ent != NULL;
          ent = qdict_next(two_level, ent)) {
-        QDict *dict = qobject_to(QDict, ent->value);
-        if (dict) {
-            child = qdict_crumple(dict, errp);
+        dict_val = qobject_to(QDict, ent->value);
+        if (dict_val && qdict_size(dict_val)) {
+            child = qdict_crumple(dict_val, errp);
             if (!child) {
                 goto error;
             }
diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 2da16f01a6..1d20fccbd4 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -79,10 +79,10 @@ static void qdict_flatten_test(void)
      *     "e.1.2.b": 1,
      *     "f.c": 2,
      *     "f.d": 3,
-     *     "g": 4
+     *     "g": 4,
+     *     "y.0": {},
+     *     "z.a": []
      * }
-     *
-     * Note that "y" and "z" get eaten.
      */
 
     qdict_put_int(e_1_2, "a", 0);
@@ -117,8 +117,10 @@ static void qdict_flatten_test(void)
     g_assert(qdict_get_int(root, "f.c") == 2);
     g_assert(qdict_get_int(root, "f.d") == 3);
     g_assert(qdict_get_int(root, "g") == 4);
+    g_assert(!qdict_size(qdict_get_qdict(root, "y.0")));
+    g_assert(qlist_empty(qdict_get_qlist(root, "z.a")));
 
-    g_assert(qdict_size(root) == 8);
+    g_assert(qdict_size(root) == 10);
 
     qobject_unref(root);
 }
@@ -387,7 +389,8 @@ static void qdict_join_test(void)
 static void qdict_crumple_test_recursive(void)
 {
     QDict *src, *dst, *rule, *vnc, *acl, *listen;
-    QList *rules;
+    QDict *empty, *empty_dict, *empty_list_0;
+    QList *rules, *empty_list, *empty_dict_a;
 
     src = qdict_new();
     qdict_put_str(src, "vnc.listen.addr", "127.0.0.1");
@@ -399,10 +402,12 @@ static void qdict_crumple_test_recursive(void)
     qdict_put_str(src, "vnc.acl.default", "deny");
     qdict_put_str(src, "vnc.acl..name", "acl0");
     qdict_put_str(src, "vnc.acl.rule..name", "acl0");
+    qdict_put(src, "empty.dict.a", qlist_new());
+    qdict_put(src, "empty.list.0", qdict_new());
 
     dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
     g_assert(dst);
-    g_assert_cmpint(qdict_size(dst), ==, 1);
+    g_assert_cmpint(qdict_size(dst), ==, 2);
 
     vnc = qdict_get_qdict(dst, "vnc");
     g_assert(vnc);
@@ -440,6 +445,21 @@ static void qdict_crumple_test_recursive(void)
     g_assert_cmpstr("acl0", ==, qdict_get_str(vnc, "acl.name"));
     g_assert_cmpstr("acl0", ==, qdict_get_str(acl, "rule.name"));
 
+    empty = qdict_get_qdict(dst, "empty");
+    g_assert(empty);
+    g_assert_cmpint(qdict_size(empty), ==, 2);
+    empty_dict = qdict_get_qdict(empty, "dict");
+    g_assert(empty_dict);
+    g_assert_cmpint(qdict_size(empty_dict), ==, 1);
+    empty_dict_a = qdict_get_qlist(empty_dict, "a");
+    g_assert(empty_dict_a && qlist_empty(empty_dict_a));
+    empty_list = qdict_get_qlist(empty, "list");
+    g_assert(empty_list);
+    g_assert_cmpint(qlist_size(empty_list), ==, 1);
+    empty_list_0 = qobject_to(QDict, qlist_pop(empty_list));
+    g_assert(empty_list_0);
+    g_assert_cmpint(qdict_size(empty_list_0), ==, 0);
+
     qobject_unref(src);
     qobject_unref(dst);
 }
@@ -587,7 +607,7 @@ static void qdict_rename_keys_test(void)
 
 static void qdict_crumple_test_bad_inputs(void)
 {
-    QDict *src;
+    QDict *src, *nested;
     Error *error = NULL;
 
     src = qdict_new();
@@ -614,7 +634,9 @@ static void qdict_crumple_test_bad_inputs(void)
 
     src = qdict_new();
     /* The input should be flat, ie no dicts or lists */
-    qdict_put(src, "rule.a", qdict_new());
+    nested = qdict_new();
+    qdict_put(nested, "x", qdict_new());
+    qdict_put(src, "rule.a", nested);
     qdict_put_str(src, "rule.b", "allow");
 
     g_assert(qdict_crumple(src, &error) == NULL);
-- 
2.17.1


[Qemu-devel] [PATCH 17/18] rbd: New parameter auth-client-required
Posted by Markus Armbruster, 1 week ago
Parameter auth-client-required lets you configure authentication
methods.  We tried to provide that in v2.9.0, but backed out due to
interface design doubts (commit 464444fcc16).

This commit is similar to what we backed out, but simpler: we use a
list of enumeration values instead of a list of objects with a member
of enumeration type.

Let's review our reasons for backing out the first try, as stated in
the commit message:

    * The implementation uses deprecated rados_conf_set() key
      "auth_supported".  No biggie.

Fixed: we use "auth-client-required".

    * The implementation makes -drive silently ignore invalid parameters
      "auth" and "auth-supported.*.X" where X isn't "auth".  Fixable (in
      fact I'm going to fix similar bugs around parameter server), so
      again no biggie.

That fix is commit 2836284db60.  This commit doesn't bring the bugs
back.

    * BlockdevOptionsRbd member @password-secret applies only to
      authentication method cephx.  Should it be a variant member of
      RbdAuthMethod?

We've had time to ponder, and we decided to stick to the way Ceph
configuration works: the key configured separately, and silently
ignored if the authentication method doesn't use it.

    * BlockdevOptionsRbd member @user could apply to both methods cephx
      and none, but I'm not sure it's actually used with none.  If it
      isn't, should it be a variant member of RbdAuthMethod?

Likewise.

    * The client offers a *set* of authentication methods, not a list.
      Should the methods be optional members of BlockdevOptionsRbd instead
      of members of list @auth-supported?  The latter begs the question
      what multiple entries for the same method mean.  Trivial question
      now that RbdAuthMethod contains nothing but @type, but less so when
      RbdAuthMethod acquires other members, such the ones discussed above.

Again, we decided to stick to the way Ceph configuration works, except
we make auth-client-required a list of enumeration values instead of a
string containing keywords separated by delimiters.

    * How BlockdevOptionsRbd member @auth-supported interacts with
      settings from a configuration file specified with @conf is
      undocumented.  I suspect it's untested, too.

Not actually true, the documentation for @conf says "Values in the
configuration file will be overridden by options specified via QAPI",
and we've tested this.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c          | 42 ++++++++++++++++++++++++++++++++----------
 qapi/block-core.json | 13 +++++++++++++
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 82346a2a5e..ea0575d068 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -240,20 +240,42 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp)
 
 
 static int qemu_rbd_set_auth(rados_t cluster, const char *secretid,
+                             BlockdevOptionsRbd *opts,
                              Error **errp)
 {
-    if (secretid == 0) {
-        return 0;
-    }
+    char *acr;
+    int r;
+    GString *accu;
+    RbdAuthModeList *auth;
+
+    if (secretid) {
+        gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
+                                                        errp);
+        if (!secret) {
+            return -1;
+        }
 
-    gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
-                                                    errp);
-    if (!secret) {
-        return -1;
+        rados_conf_set(cluster, "key", secret);
+        g_free(secret);
     }
 
-    rados_conf_set(cluster, "key", secret);
-    g_free(secret);
+    if (opts->has_auth_client_required) {
+        accu = g_string_new("");
+        for (auth = opts->auth_client_required; auth; auth = auth->next) {
+            if (accu->str[0]) {
+                g_string_append_c(accu, ';');
+            }
+            g_string_append(accu, RbdAuthMode_str(auth->value));
+        }
+        acr = g_string_free(accu, FALSE);
+        r = rados_conf_set(cluster, "auth_client_required", acr);
+        g_free(acr);
+        if (r < 0) {
+            error_setg_errno(errp, -r,
+                             "Could not set 'auth_client_required'");
+            return r;
+        }
+    }
 
     return 0;
 }
@@ -585,7 +607,7 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
         }
     }
 
-    if (qemu_rbd_set_auth(*cluster, secretid, errp) < 0) {
+    if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) {
         r = -EIO;
         goto failed_shutdown;
     }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4b1de474a9..841d196a21 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3093,6 +3093,14 @@
             '*timeout': 'int' } }
 
 
+##
+# @RbdAuthMode:
+#
+# Since: 3.0
+##
+{ 'enum': 'RbdAuthMode',
+  'data': [ 'cephx', 'none' ] }
+
 ##
 # @BlockdevOptionsRbd:
 #
@@ -3108,6 +3116,10 @@
 #
 # @user:               Ceph id name.
 #
+# @auth-client-required: Acceptable authentication modes.
+#                      This maps to Ceph configuration option
+#                      "auth_client_required".  (Since 3.0)
+#
 # @server:             Monitor host address and port.  This maps
 #                      to the "mon_host" Ceph option.
 #
@@ -3119,6 +3131,7 @@
             '*conf': 'str',
             '*snapshot': 'str',
             '*user': 'str',
+            '*auth-client-required': ['RbdAuthMode'],
             '*server': ['InetSocketAddressBase'] } }
 
 ##
-- 
2.17.1


[Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret
Posted by Markus Armbruster, 1 week ago
Legacy -drive supports "password-secret" parameter that isn't
available with -blockdev / blockdev-add.  That's because we backed out
our first try to provide it there due to interface design doubts, in
commit 577d8c9a811, v2.9.0.

This is the second try.  It brings back the parameter, except it's
named "key-secret" now.

Let's review our reasons for backing out the first try, as stated in
the commit message:

    * BlockdevOptionsRbd member @password-secret isn't actually a
      password, it's a key generated by Ceph.

Addressed by the rename.

    * We're not sure where member @password-secret belongs (see the
      previous commit).

See previous commit.

    * How @password-secret interacts with settings from a configuration
      file specified with @conf is undocumented.

Not actually true, the documentation for @conf says "Values in the
configuration file will be overridden by options specified via QAPI",
and we've tested this.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c          | 41 +++++++++++++++++++++++++----------------
 qapi/block-core.json |  6 ++++++
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ea0575d068..f2c6965418 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -239,24 +239,25 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp)
 }
 
 
-static int qemu_rbd_set_auth(rados_t cluster, const char *secretid,
-                             BlockdevOptionsRbd *opts,
+static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts,
                              Error **errp)
 {
-    char *acr;
+    char *key, *acr;
     int r;
     GString *accu;
     RbdAuthModeList *auth;
 
-    if (secretid) {
-        gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
-                                                        errp);
-        if (!secret) {
-            return -1;
+    if (opts->key_secret) {
+        key = qcrypto_secret_lookup_as_base64(opts->key_secret, errp);
+        if (!key) {
+            return -EIO;
+        }
+        r = rados_conf_set(cluster, "key", key);
+        g_free(key);
+        if (r < 0) {
+            error_setg_errno(errp, -r, "Could not set 'key'");
+            return r;
         }
-
-        rados_conf_set(cluster, "key", secret);
-        g_free(secret);
     }
 
     if (opts->has_auth_client_required) {
@@ -367,9 +368,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-/* FIXME Deprecate and remove keypairs or make it available in QMP.
- * password_secret should eventually be configurable in opts->location. Support
- * for it in .bdrv_open will make it work here as well. */
+/* FIXME Deprecate and remove keypairs or make it available in QMP. */
 static int qemu_rbd_do_create(BlockdevCreateOptions *options,
                               const char *keypairs, const char *password_secret,
                               Error **errp)
@@ -575,6 +574,16 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
     Error *local_err = NULL;
     int r;
 
+    if (secretid) {
+        if (opts->key_secret) {
+            error_setg(errp,
+                       "Legacy 'password-secret' clashes with 'key-secret'");
+            return -EINVAL;
+        }
+        opts->key_secret = g_strdup(secretid);
+        opts->has_key_secret = true;
+    }
+
     mon_host = qemu_rbd_mon_host(opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -607,8 +616,8 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
         }
     }
 
-    if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) {
-        r = -EIO;
+    r = qemu_rbd_set_auth(*cluster, opts, errp);
+    if (r < 0) {
         goto failed_shutdown;
     }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 841d196a21..3eb536de5b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3120,6 +3120,11 @@
 #                      This maps to Ceph configuration option
 #                      "auth_client_required".  (Since 3.0)
 #
+# @key-secret:         ID of a QCryptoSecret object providing a key
+#                      for cephx authentication.
+#                      This maps to Ceph configuration option
+#                      "key".  (Since 3.0)
+#
 # @server:             Monitor host address and port.  This maps
 #                      to the "mon_host" Ceph option.
 #
@@ -3132,6 +3137,7 @@
             '*snapshot': 'str',
             '*user': 'str',
             '*auth-client-required': ['RbdAuthMode'],
+            '*key-secret': 'str',
             '*server': ['InetSocketAddressBase'] } }
 
 ##
-- 
2.17.1


Re: [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret
Posted by Daniel P. Berrangé, 1 week ago
On Tue, Jun 12, 2018 at 02:58:21PM +0200, Markus Armbruster wrote:
> Legacy -drive supports "password-secret" parameter that isn't
> available with -blockdev / blockdev-add.  That's because we backed out
> our first try to provide it there due to interface design doubts, in
> commit 577d8c9a811, v2.9.0.
> 
> This is the second try.  It brings back the parameter, except it's
> named "key-secret" now.
> 
> Let's review our reasons for backing out the first try, as stated in
> the commit message:
> 
>     * BlockdevOptionsRbd member @password-secret isn't actually a
>       password, it's a key generated by Ceph.

I thought about that when I first added password-secret, but felt
that it is still effectively acting as a password to authenticate
to the server, and calling it password-secret made it clearer that
it was related to the authentication phase, and not for example,
disk encryption.

> 
> Addressed by the rename.
> 
>     * We're not sure where member @password-secret belongs (see the
>       previous commit).
> 
> See previous commit.
> 
>     * How @password-secret interacts with settings from a configuration
>       file specified with @conf is undocumented.
> 
> Not actually true, the documentation for @conf says "Values in the
> configuration file will be overridden by options specified via QAPI",
> and we've tested this.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c          | 41 +++++++++++++++++++++++++----------------
>  qapi/block-core.json |  6 ++++++
>  2 files changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index ea0575d068..f2c6965418 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -239,24 +239,25 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp)
>  }
>  
>  
> -static int qemu_rbd_set_auth(rados_t cluster, const char *secretid,
> -                             BlockdevOptionsRbd *opts,
> +static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts,
>                               Error **errp)
>  {
> -    char *acr;
> +    char *key, *acr;
>      int r;
>      GString *accu;
>      RbdAuthModeList *auth;
>  
> -    if (secretid) {
> -        gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
> -                                                        errp);
> -        if (!secret) {
> -            return -1;
> +    if (opts->key_secret) {
> +        key = qcrypto_secret_lookup_as_base64(opts->key_secret, errp);
> +        if (!key) {
> +            return -EIO;
> +        }
> +        r = rados_conf_set(cluster, "key", key);
> +        g_free(key);
> +        if (r < 0) {
> +            error_setg_errno(errp, -r, "Could not set 'key'");
> +            return r;
>          }
> -
> -        rados_conf_set(cluster, "key", secret);
> -        g_free(secret);
>      }
>  
>      if (opts->has_auth_client_required) {
> @@ -367,9 +368,7 @@ static QemuOptsList runtime_opts = {
>      },
>  };
>  
> -/* FIXME Deprecate and remove keypairs or make it available in QMP.
> - * password_secret should eventually be configurable in opts->location. Support
> - * for it in .bdrv_open will make it work here as well. */
> +/* FIXME Deprecate and remove keypairs or make it available in QMP. */
>  static int qemu_rbd_do_create(BlockdevCreateOptions *options,
>                                const char *keypairs, const char *password_secret,
>                                Error **errp)
> @@ -575,6 +574,16 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
>      Error *local_err = NULL;
>      int r;
>  
> +    if (secretid) {
> +        if (opts->key_secret) {
> +            error_setg(errp,
> +                       "Legacy 'password-secret' clashes with 'key-secret'");
> +            return -EINVAL;
> +        }
> +        opts->key_secret = g_strdup(secretid);
> +        opts->has_key_secret = true;
> +    }
> +
>      mon_host = qemu_rbd_mon_host(opts, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -607,8 +616,8 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
>          }
>      }
>  
> -    if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) {
> -        r = -EIO;
> +    r = qemu_rbd_set_auth(*cluster, opts, errp);
> +    if (r < 0) {
>          goto failed_shutdown;
>      }
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 841d196a21..3eb536de5b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3120,6 +3120,11 @@
>  #                      This maps to Ceph configuration option
>  #                      "auth_client_required".  (Since 3.0)
>  #
> +# @key-secret:         ID of a QCryptoSecret object providing a key
> +#                      for cephx authentication.
> +#                      This maps to Ceph configuration option
> +#                      "key".  (Since 3.0)
> +#
>  # @server:             Monitor host address and port.  This maps
>  #                      to the "mon_host" Ceph option.
>  #
> @@ -3132,6 +3137,7 @@
>              '*snapshot': 'str',
>              '*user': 'str',
>              '*auth-client-required': ['RbdAuthMode'],
> +            '*key-secret': 'str',
>              '*server': ['InetSocketAddressBase'] } }
>  
>  ##
> -- 
> 2.17.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret
Posted by Markus Armbruster, 1 week ago
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jun 12, 2018 at 02:58:21PM +0200, Markus Armbruster wrote:
>> Legacy -drive supports "password-secret" parameter that isn't
>> available with -blockdev / blockdev-add.  That's because we backed out
>> our first try to provide it there due to interface design doubts, in
>> commit 577d8c9a811, v2.9.0.
>> 
>> This is the second try.  It brings back the parameter, except it's
>> named "key-secret" now.
>> 
>> Let's review our reasons for backing out the first try, as stated in
>> the commit message:
>> 
>>     * BlockdevOptionsRbd member @password-secret isn't actually a
>>       password, it's a key generated by Ceph.
>
> I thought about that when I first added password-secret, but felt
> that it is still effectively acting as a password to authenticate
> to the server, and calling it password-secret made it clearer that
> it was related to the authentication phase, and not for example,
> disk encryption.

I feel it's best to stick to the names Ceph uses, and Ceph calls it
"key".

[...]