* [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication
@ 2018-06-07 6:25 Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 01/19] rbd: Drop deprecated -drive parameter "filename" Markus Armbruster
` (19 more replies)
0 siblings, 20 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
This series is RFC because:
* It clashes with parts of Max's "[PATCH 00/13] block: Try to create
well typed json:{} filenames". I missed that one until too late,
sorry.
- I stole "[PATCH 06/13] block: Add block-specific QDict header",
and tacked on fixups I'd like to have.
- My qobject_input_visitor_new_flat_confused() addresses the same
general problem as Max's qdict_stringify_for_keyval(): the block
layer's confused use of QObject types. My solution fixes a number
of bugs in -blockdev, blockdev-add and -drive. If Max's solution
adds further value, we need to merge the two somehow.
- I changed qdict_flatten() to fix -blockdev and blockdev-add for
empty objects and arrays. Max fixed it to not mess up shallow
clones. We need to merge the two.
* Rbd testing is incomplete. Jeff Cody volunteered to test on his
rig. Results should be in soon.
Perhaps the series should be split in two: PATCH 01-17 are
configuration fixes, PATCH 18-19 are rbd authentication work. I may
still do that for the non-RFC patch submission.
Markus Armbruster (18):
rbd: Drop deprecated -drive parameter "filename"
iscsi: Drop deprecated -drive parameter "filename"
fixup block: Add block-specific QDict header
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 | 15 +
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, 1573 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 01/19] rbd: Drop deprecated -drive parameter "filename"
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 02/19] iscsi: " Markus Armbruster
` (18 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 02/19] iscsi: Drop deprecated -drive parameter "filename"
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 01/19] rbd: Drop deprecated -drive parameter "filename" Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 03/19] block: Add block-specific QDict header Markus Armbruster
` (17 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 03/19] block: Add block-specific QDict header
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 01/19] rbd: Drop deprecated -drive parameter "filename" Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 02/19] iscsi: " Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 04/19] fixup " Markus Armbruster
` (16 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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>
---
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 | 35 +++++++++++++++++++++++++++++++++++
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, 59 insertions(+), 17 deletions(-)
create mode 100644 include/block/qdict.h
diff --git a/block.c b/block.c
index 501b64c819..700f4f2d4c 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 549fee9b69..3c5bd46663 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..825e096a72
--- /dev/null
+++ b/include/block/qdict.h
@@ -0,0 +1,35 @@
+/*
+ * Special QDict functions used by the block layer
+ *
+ * Copyright (c) 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"
+#include "qapi/qmp/qobject.h"
+#include "qapi/error.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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 04/19] fixup block: Add block-specific QDict header
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (2 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 03/19] block: Add block-specific QDict header Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 05/19] qobject: Move block-specific qdict code to block-qdict.c Markus Armbruster
` (15 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/block/qdict.h | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/include/block/qdict.h b/include/block/qdict.h
index 825e096a72..71c037afba 100644
--- a/include/block/qdict.h
+++ b/include/block/qdict.h
@@ -1,7 +1,7 @@
/*
* Special QDict functions used by the block layer
*
- * Copyright (c) 2018 Red Hat, Inc.
+ * 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.
@@ -11,9 +11,6 @@
#define BLOCK_QDICT_H
#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qobject.h"
-#include "qapi/error.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);
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 05/19] qobject: Move block-specific qdict code to block-qdict.c
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (3 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 04/19] fixup " Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars Markus Armbruster
` (14 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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 41cd3736a9..21f4bc1806 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1365,6 +1365,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 9854e7794b..b4dd140805 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)
@@ -582,6 +584,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 \
@@ -612,6 +615,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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (4 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 05/19] qobject: Move block-specific qdict code to block-qdict.c Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-11 14:44 ` [Qemu-devel] [Qemu-block] " Max Reitz
2018-06-12 13:38 ` [Qemu-devel] " Kevin Wolf
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 07/19] block: Fix -drive " Markus Armbruster
` (13 subsequent siblings)
19 siblings, 2 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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 3c5bd46663..a81ad4aef0 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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 07/19] block: Fix -drive for certain non-string scalars
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (5 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 08/19] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts() Markus Armbruster
` (12 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 08/19] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts()
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (6 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 07/19] block: Fix -drive " Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 09/19] block: Factor out qobject_input_visitor_new_flat_confused() Markus Armbruster
` (11 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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 a81ad4aef0..495276836f 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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 09/19] block: Factor out qobject_input_visitor_new_flat_confused()
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (7 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 08/19] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts() Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 10/19] block: Make remaining uses of qobject input visitor more robust Markus Armbruster
` (10 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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 495276836f..d7f0405662 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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 10/19] block: Make remaining uses of qobject input visitor more robust
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (8 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 09/19] block: Factor out qobject_input_visitor_new_flat_confused() Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-12 14:43 ` Kevin Wolf
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 11/19] block-qdict: Simplify qdict_flatten_qdict() Markus Armbruster
` (9 subsequent siblings)
19 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 11/19] block-qdict: Simplify qdict_flatten_qdict()
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (9 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 10/19] block: Make remaining uses of qobject input visitor more robust Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 12/19] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist() Markus Armbruster
` (8 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 12/19] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist()
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (10 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 11/19] block-qdict: Simplify qdict_flatten_qdict() Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 13/19] block-qdict: Clean up qdict_crumple() a bit Markus Armbruster
` (7 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 13/19] block-qdict: Clean up qdict_crumple() a bit
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (11 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 12/19] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist() Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 14/19] block-qdict: Simplify qdict_is_list() some Markus Armbruster
` (6 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 14/19] block-qdict: Simplify qdict_is_list() some
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (12 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 13/19] block-qdict: Clean up qdict_crumple() a bit Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 15/19] check-block-qdict: Rename qdict_flatten()'s variables for clarity Markus Armbruster
` (5 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 15/19] check-block-qdict: Rename qdict_flatten()'s variables for clarity
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (13 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 14/19] block-qdict: Simplify qdict_is_list() some Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 16/19] check-block-qdict: Cover flattening of empty lists and dictionaries Markus Armbruster
` (4 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 16/19] check-block-qdict: Cover flattening of empty lists and dictionaries
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (14 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 15/19] check-block-qdict: Rename qdict_flatten()'s variables for clarity Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 17/19] block: Fix -blockdev / blockdev-add for empty objects and arrays Markus Armbruster
` (3 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 17/19] block: Fix -blockdev / blockdev-add for empty objects and arrays
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (15 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 16/19] check-block-qdict: Cover flattening of empty lists and dictionaries Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 18/19] rbd: New parameter auth-client-required Markus Armbruster
` (2 subsequent siblings)
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
-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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 18/19] rbd: New parameter auth-client-required
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (16 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 17/19] block: Fix -blockdev / blockdev-add for empty objects and arrays Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 19/19] rbd: New parameter key-secret Markus Armbruster
2018-06-07 21:33 ` [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Jeff Cody
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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 | 11 +++++++++++
2 files changed, 43 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..6964fb707f 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,8 @@
#
# @user: Ceph id name.
#
+# @auth-client-required: Acceptable authentication modes (since 3.0).
+#
# @server: Monitor host address and port. This maps
# to the "mon_host" Ceph option.
#
@@ -3119,6 +3129,7 @@
'*conf': 'str',
'*snapshot': 'str',
'*user': 'str',
+ '*auth-client-required': ['RbdAuthMode'],
'*server': ['InetSocketAddressBase'] } }
##
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [RFC PATCH 19/19] rbd: New parameter key-secret
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (17 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 18/19] rbd: New parameter auth-client-required Markus Armbruster
@ 2018-06-07 6:25 ` Markus Armbruster
2018-06-07 21:33 ` [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Jeff Cody
19 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-07 6:25 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
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 | 4 ++++
2 files changed, 29 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 6964fb707f..4bd85dd1bb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3118,6 +3118,9 @@
#
# @auth-client-required: Acceptable authentication modes (since 3.0).
#
+# @key-secret: ID of a QCryptoSecret object providing a key
+# for cephx authentication (since 3.0)
+#
# @server: Monitor host address and port. This maps
# to the "mon_host" Ceph option.
#
@@ -3130,6 +3133,7 @@
'*snapshot': 'str',
'*user': 'str',
'*auth-client-required': ['RbdAuthMode'],
+ '*key-secret': 'str',
'*server': ['InetSocketAddressBase'] } }
##
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
` (18 preceding siblings ...)
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 19/19] rbd: New parameter key-secret Markus Armbruster
@ 2018-06-07 21:33 ` Jeff Cody
2018-06-12 12:55 ` Jeff Cody
19 siblings, 1 reply; 28+ messages in thread
From: Jeff Cody @ 2018-06-07 21:33 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-block, kwolf, mreitz, eblake
On Thu, Jun 07, 2018 at 08:25:40AM +0200, Markus Armbruster wrote:
> This series is RFC because:
>
> * It clashes with parts of Max's "[PATCH 00/13] block: Try to create
> well typed json:{} filenames". I missed that one until too late,
> sorry.
>
> - I stole "[PATCH 06/13] block: Add block-specific QDict header",
> and tacked on fixups I'd like to have.
>
> - My qobject_input_visitor_new_flat_confused() addresses the same
> general problem as Max's qdict_stringify_for_keyval(): the block
> layer's confused use of QObject types. My solution fixes a number
> of bugs in -blockdev, blockdev-add and -drive. If Max's solution
> adds further value, we need to merge the two somehow.
>
> - I changed qdict_flatten() to fix -blockdev and blockdev-add for
> empty objects and arrays. Max fixed it to not mess up shallow
> clones. We need to merge the two.
>
> * Rbd testing is incomplete. Jeff Cody volunteered to test on his
> rig. Results should be in soon.
>
Here are some results from auth testing of various combinations; I haven't
completed all the combinations in my matrix yet, but what I have completed
looks like what I would expect.
These were all tested with blockdev-add QAPI commands against this patch
series.
I'll be away on PTO tomorrow (Friday), so I'll conclude testing on Monday.
Warning, long lines below, so don't read it on a vt220 (apologies in
advance if you do...):
Server | Client-Side (qemu host)
------------+--------------------------------------------------------------------------------------------------
| |
ServerAuth | user key-secret /etc/ceph/keyring auth-client-required | Result
------------+--------------------------------------------------------------------------------------------------
cephx, none | --- --- --- --- {"return": {}}
cephx, none | --- --- valid --- {"return": {}}
cephx, none | --- --- invalid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid --- --- {"return": {}}
cephx, none | --- invalid --- --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- invalid valid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid invalid --- {"return": {}}
cephx, none | admin --- valid ---
cephx, none | admin --- invalid ---
cephx, none | invalid --- valid ---
cephx, none | invalid --- invalid ---
cephx, none | --- --- --- none {"return": {}}
cephx, none | --- --- valid none {"return": {}}
cephx, none | --- --- invalid none {"return": {}}
cephx, none | --- valid --- none {"return": {}}
cephx, none | --- invalid --- none {"return": {}}
cephx, none | --- invalid valid none {"return": {}}
cephx, none | --- valid invalid none {"return": {}}
cephx, none | admin --- valid none {"return": {}}
cephx, none | admin --- invalid none {"return": {}}
cephx, none | invalid --- valid none {"return": {}}
cephx, none | invalid --- invalid none {"return": {}}
|
cephx, none | --- --- --- cephx {"error": {"class": "GenericError", "desc": "error connecting: No such file or directory"}}
cephx, none | --- --- valid cephx {"return": {}}
cephx, none | --- --- invalid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid --- cephx {"return": {}}
cephx, none | --- invalid --- cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- invalid valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid invalid cephx {"return": {}}
cephx, none | admin --- valid cephx {"return": {}}
cephx, none | invalid --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | invalid --- invalid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
|
cephx, none | --- --- --- cephx, none {"return": {}}
cephx, none | --- --- valid cephx, none {"return": {}}
cephx, none | --- --- invalid cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid --- cephx, none {"return": {}}
cephx, none | --- invalid --- cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- invalid valid cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid invalid cephx, none {"return": {}}
cephx, none | admin --- valid cephx, none {"return": {}}
cephx, none | invalid --- valid cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
|
none | --- --- --- none {"return": {}}
none | --- --- valid none {"return": {}}
none | --- --- invalid none {"return": {}}
none | --- valid --- none {"return": {}}
none | --- invalid --- none {"return": {}}
none | admin --- valid none {"return": {}}
none | invalid --- valid none {"return": {}}
|
none | --- --- --- cephx {"error": {"class": "GenericError", "desc": "error connecting: No such file or directory"}}
none | --- --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
none | --- --- invalid cephx {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
none | --- valid --- cephx {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
none | --- invalid --- cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
none | admin --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
none | invalid --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
|
none | --- --- --- cephx, none
none | --- --- valid cephx, none
none | --- --- invalid cephx, none
none | --- valid --- cephx, none
none | --- invalid --- cephx, none
none | admin --- valid cephx, none
none | invalid --- valid cephx, none
|
cephx | --- --- --- ---
cephx | --- --- valid ---
cephx | --- --- invalid ---
cephx | --- valid --- ---
cephx | --- invalid --- ---
cephx | admin --- valid ---
cephx | invalid --- valid ---
|
cephx | --- --- --- none
cephx | --- --- valid none
cephx | --- --- invalid none
cephx | --- valid --- none
cephx | --- invalid --- none
cephx | admin --- valid none
cephx | invalid --- valid none
|
cephx | --- --- --- cephx {"error": {"class": "GenericError", "desc": "error connecting: No such file or directory"}}
cephx | --- --- valid cephx {"return": {}}
cephx | --- --- invalid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx | --- valid --- cephx {"return": {}}
cephx | --- invalid --- cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx | admin --- valid cephx {"return": {}}
cephx | invalid --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
|
cephx | --- --- --- cephx, none
cephx | --- --- valid cephx, none
cephx | --- --- invalid cephx, none
cephx | --- valid --- cephx, none
cephx | --- invalid --- cephx, none
cephx | admin --- valid cephx, none
cephx | invalid --- valid cephx, none
> Perhaps the series should be split in two: PATCH 01-17 are
> configuration fixes, PATCH 18-19 are rbd authentication work. I may
> still do that for the non-RFC patch submission.
>
> Markus Armbruster (18):
> rbd: Drop deprecated -drive parameter "filename"
> iscsi: Drop deprecated -drive parameter "filename"
> fixup block: Add block-specific QDict header
> 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 | 15 +
> 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, 1573 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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars Markus Armbruster
@ 2018-06-11 14:44 ` Max Reitz
2018-06-12 13:38 ` [Qemu-devel] " Kevin Wolf
1 sibling, 0 replies; 28+ messages in thread
From: Max Reitz @ 2018-06-11 14:44 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1: Type: text/plain, Size: 6813 bytes --]
On 2018-06-07 08:25, Markus Armbruster wrote:
> 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.
(I'll just chime in because why not.)
> 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.
Agreed. The way there probably is to (as always) unify how the block
drivers convert their QDict to their own BlockdevOptions, and then pull
that out into block.c, but let's see how far off that still is.
> Next best would be to fix the block layer to always pass correctly
> typed QDicts to the BlockDriver methods. Also beyond my reach.
I tried and failed.
(http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00061.html)
But that will probably be an intermediate step before we get to
BlockdevOptions.
> 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().
I'm calling qdict_stringify_for_keyval() only from a single function,
and it's immediately followed by a qdict_crumple(), so I suppose
replacing those invocations in my series would be trivial.
OTOH, just using qdict_stringify_for_keyval() in your
qdict_crumple_for_keyval_qiv() function would be trivial as well. :-)
But then again, it probably makes more sense to combine the two
functions because I too assumed the QDict to be flattened before the
function call for simplification. This assumption makes more sense if
the function proceeds to call qdict_crumple().
> 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.
Sounds good!
> 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(-)
[...]
> @@ -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
>
Looks functionally very much equivalent to my version, so no complaints
from me.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication
2018-06-07 21:33 ` [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Jeff Cody
@ 2018-06-12 12:55 ` Jeff Cody
2018-06-12 19:04 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Cody @ 2018-06-12 12:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-block, kwolf, mreitz, eblake
On Thu, Jun 07, 2018 at 05:33:03PM -0400, Jeff Cody wrote:
> On Thu, Jun 07, 2018 at 08:25:40AM +0200, Markus Armbruster wrote:
> > This series is RFC because:
> >
> > * It clashes with parts of Max's "[PATCH 00/13] block: Try to create
> > well typed json:{} filenames". I missed that one until too late,
> > sorry.
> >
> > - I stole "[PATCH 06/13] block: Add block-specific QDict header",
> > and tacked on fixups I'd like to have.
> >
> > - My qobject_input_visitor_new_flat_confused() addresses the same
> > general problem as Max's qdict_stringify_for_keyval(): the block
> > layer's confused use of QObject types. My solution fixes a number
> > of bugs in -blockdev, blockdev-add and -drive. If Max's solution
> > adds further value, we need to merge the two somehow.
> >
> > - I changed qdict_flatten() to fix -blockdev and blockdev-add for
> > empty objects and arrays. Max fixed it to not mess up shallow
> > clones. We need to merge the two.
> >
> > * Rbd testing is incomplete. Jeff Cody volunteered to test on his
> > rig. Results should be in soon.
> >
>
> Here are some results from auth testing of various combinations; I haven't
> completed all the combinations in my matrix yet, but what I have completed
> looks like what I would expect.
>
> These were all tested with blockdev-add QAPI commands against this patch
> series.
>
> I'll be away on PTO tomorrow (Friday), so I'll conclude testing on Monday.
>
> Warning, long lines below, so don't read it on a vt220 (apologies in
> advance if you do...):
>
Below is the rest of the matrix filled out. Everything looks OK to me, the
ones that were a bit different than I expected were when the server was
'none', and we passed an bad key-secret. But that isn't a qemu/qapi issue,
and not really an issue at all (just different from what I expected).
Completed tests:
Server | Client-Side (qemu host)
------------+--------------------------------------------------------------------------------------------------
| |
ServerAuth | user key-secret /etc/ceph/keyring auth-client-required | Result
------------+--------------------------------------------------------------------------------------------------
cephx, none | --- --- --- --- {"return": {}}
cephx, none | --- --- valid --- {"return": {}}
cephx, none | --- --- invalid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid --- --- {"return": {}}
cephx, none | --- invalid --- --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- invalid valid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid invalid --- {"return": {}}
cephx, none | admin --- valid --- {"return": {}}
cephx, none | admin --- invalid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | invalid --- valid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | invalid --- invalid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- --- --- none {"return": {}}
cephx, none | --- --- valid none {"return": {}}
cephx, none | --- --- invalid none {"return": {}}
cephx, none | --- valid --- none {"return": {}}
cephx, none | --- invalid --- none {"return": {}}
cephx, none | --- invalid valid none {"return": {}}
cephx, none | --- valid invalid none {"return": {}}
cephx, none | admin --- valid none {"return": {}}
cephx, none | admin --- invalid none {"return": {}}
cephx, none | invalid --- valid none {"return": {}}
cephx, none | invalid --- invalid none {"return": {}}
|
cephx, none | --- --- --- cephx {"error": {"class": "GenericError", "desc": "error connecting: No such file or directory"}}
cephx, none | --- --- valid cephx {"return": {}}
cephx, none | --- --- invalid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid --- cephx {"return": {}}
cephx, none | --- invalid --- cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- invalid valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid invalid cephx {"return": {}}
cephx, none | admin --- valid cephx {"return": {}}
cephx, none | invalid --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | invalid --- invalid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
|
cephx, none | --- --- --- cephx, none {"return": {}}
cephx, none | --- --- valid cephx, none {"return": {}}
cephx, none | --- --- invalid cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid --- cephx, none {"return": {}}
cephx, none | --- invalid --- cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- invalid valid cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx, none | --- valid invalid cephx, none {"return": {}}
cephx, none | admin --- valid cephx, none {"return": {}}
cephx, none | invalid --- valid cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
|
none | --- --- --- none {"return": {}}
none | --- --- valid none {"return": {}}
none | --- --- invalid none {"return": {}}
none | --- valid --- none {"return": {}}
none | --- invalid --- none {"return": {}}
none | admin --- valid none {"return": {}}
none | invalid --- valid none {"return": {}}
|
none | --- --- --- cephx {"error": {"class": "GenericError", "desc": "error connecting: No such file or directory"}}
none | --- --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
none | --- --- invalid cephx {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
none | --- valid --- cephx {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
none | --- invalid --- cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
none | admin --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
none | invalid --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
|
none | --- --- --- cephx, none {"return": {}}
none | --- --- valid cephx, none {"return": {}}
none | --- --- invalid cephx, none {"return": {}}
none | --- valid --- cephx, none {"return": {}}
none | --- invalid --- cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
none | admin --- valid cephx, none {"return": {}}
none | invalid --- valid cephx, none {"return": {}}
cephx, none | --- --- --- --- {"return": {}}
cephx, none | --- --- valid --- {"return": {}}
|
none | --- --- --- --- {"return": {}}
|
cephx | --- --- --- --- {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
cephx | --- --- valid --- {"return": {}}
cephx | --- --- invalid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx | --- valid --- --- {"return": {}}
cephx | --- invalid --- --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx | admin --- valid --- {"return": {}}
cephx | invalid --- valid --- {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
|
cephx | --- --- --- none
cephx | --- --- valid none {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
cephx | --- --- invalid none {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
cephx | --- valid --- none {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
cephx | --- invalid --- none {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
cephx | admin --- valid none {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
cephx | invalid --- valid none {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
|
cephx | --- --- --- cephx {"error": {"class": "GenericError", "desc": "error connecting: No such file or directory"}}
cephx | --- --- valid cephx {"return": {}}
cephx | --- --- invalid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx | --- valid --- cephx {"return": {}}
cephx | --- invalid --- cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx | admin --- valid cephx {"return": {}}
cephx | invalid --- valid cephx {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
|
cephx | --- --- --- cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Operation not supported"}}
cephx | --- --- valid cephx, none {"return": {}}
cephx | --- --- invalid cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx | --- valid --- cephx, none {"return": {}}
cephx | --- invalid --- cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
cephx | admin --- valid cephx, none {"return": {}}
cephx | invalid --- valid cephx, none {"error": {"class": "GenericError", "desc": "error connecting: Invalid argument"}}
>
>
>
>
>
> > Perhaps the series should be split in two: PATCH 01-17 are
> > configuration fixes, PATCH 18-19 are rbd authentication work. I may
> > still do that for the non-RFC patch submission.
> >
> > Markus Armbruster (18):
> > rbd: Drop deprecated -drive parameter "filename"
> > iscsi: Drop deprecated -drive parameter "filename"
> > fixup block: Add block-specific QDict header
> > 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 | 15 +
> > 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, 1573 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
> >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars Markus Armbruster
2018-06-11 14:44 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2018-06-12 13:38 ` Kevin Wolf
2018-06-12 16:24 ` Markus Armbruster
1 sibling, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2018-06-12 13:38 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-block, mreitz, jcody, eblake
Am 07.06.2018 um 08:25 hat Markus Armbruster geschrieben:
> 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
I don't think the error message was intended to be part of your command
line, and I also don't have a binary called qemu-system-x86. :-)
> 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>
> +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";
This fits in a single line.
> + 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;
> +}
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 10/19] block: Make remaining uses of qobject input visitor more robust
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 10/19] block: Make remaining uses of qobject input visitor more robust Markus Armbruster
@ 2018-06-12 14:43 ` Kevin Wolf
2018-06-12 16:32 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2018-06-12 14:43 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-block, mreitz, jcody, eblake
Am 07.06.2018 um 08:25 hat Markus Armbruster geschrieben:
> 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()
Do you mean block_crypto_open_opts_init()?
> * 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);
At least this &error_abort is not correct:
$ ./qemu-img create -f qcow2 --object secret,id=sec0,data=foo -oencrypt.format=luks,encrypt.key-secret=sec0 /tmp/test.qcow2 64M
Formatting '/tmp/test.qcow2', fmt=qcow2 size=67108864 encrypt.format=luks encrypt.key-secret=sec0 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,encrypt.foo=1,encrypt.foo.bar=2,file.filename=/tmp/test.qcow2
Unexpected error in qdict_crumple() at qobject/block-qdict.c:452:
qemu-system-x86_64: -drive driver=qcow2,encrypt.foo=1,encrypt.foo.bar=2,file.filename=/tmp/test.qcow2: Key foo prefix is already set as a dict
Abgebrochen (Speicherabzug geschrieben)
> 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);
The other two &error_aborts _might_ be correct today because I couldn't
see callers where the keys weren't already checked by QemuOpts, but I'm
not sure if I looked at everything nor whether review would catch a
future patch that added a new such call.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars
2018-06-12 13:38 ` [Qemu-devel] " Kevin Wolf
@ 2018-06-12 16:24 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-12 16:24 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jcody, qemu-devel, qemu-block, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> Am 07.06.2018 um 08:25 hat Markus Armbruster geschrieben:
>> 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
>
> I don't think the error message was intended to be part of your command
> line, and I also don't have a binary called qemu-system-x86. :-)
Uh, I managed to combine a pasto with a typo. Fixing...
>> 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>
>
>> +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";
>
> This fits in a single line.
Pushing column 77, for an effective line width of 65 characters. I hate
that :)
I could be persuaded to add qbool_to_string().
>> + 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;
>> +}
>
> Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 10/19] block: Make remaining uses of qobject input visitor more robust
2018-06-12 14:43 ` Kevin Wolf
@ 2018-06-12 16:32 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-12 16:32 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Markus Armbruster, jcody, qemu-devel, qemu-block, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> Am 07.06.2018 um 08:25 hat Markus Armbruster geschrieben:
>> 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()
>
> Do you mean block_crypto_open_opts_init()?
Yes. Fixing...
>> * 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);
>
> At least this &error_abort is not correct:
>
> $ ./qemu-img create -f qcow2 --object secret,id=sec0,data=foo -oencrypt.format=luks,encrypt.key-secret=sec0 /tmp/test.qcow2 64M
> Formatting '/tmp/test.qcow2', fmt=qcow2 size=67108864 encrypt.format=luks encrypt.key-secret=sec0 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> $ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,encrypt.foo=1,encrypt.foo.bar=2,file.filename=/tmp/test.qcow2
> Unexpected error in qdict_crumple() at qobject/block-qdict.c:452:
> qemu-system-x86_64: -drive driver=qcow2,encrypt.foo=1,encrypt.foo.bar=2,file.filename=/tmp/test.qcow2: Key foo prefix is already set as a dict
> Abgebrochen (Speicherabzug geschrieben)
>
>> 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);
>
> The other two &error_aborts _might_ be correct today because I couldn't
> see callers where the keys weren't already checked by QemuOpts, but I'm
> not sure if I looked at everything nor whether review would catch a
> future patch that added a new such call.
Okay, I'm getting rid of all three.
Thanks!
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication
2018-06-12 12:55 ` Jeff Cody
@ 2018-06-12 19:04 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-06-12 19:04 UTC (permalink / raw)
To: Jeff Cody; +Cc: Markus Armbruster, kwolf, qemu-devel, qemu-block, mreitz
Jeff Cody <jcody@redhat.com> writes:
> On Thu, Jun 07, 2018 at 05:33:03PM -0400, Jeff Cody wrote:
>> Here are some results from auth testing of various combinations; I haven't
>> completed all the combinations in my matrix yet, but what I have completed
>> looks like what I would expect.
>>
>> These were all tested with blockdev-add QAPI commands against this patch
>> series.
>>
>> I'll be away on PTO tomorrow (Friday), so I'll conclude testing on Monday.
>>
>> Warning, long lines below, so don't read it on a vt220 (apologies in
>> advance if you do...):
>>
>
> Below is the rest of the matrix filled out. Everything looks OK to me, the
> ones that were a bit different than I expected were when the server was
> 'none', and we passed an bad key-secret. But that isn't a qemu/qapi issue,
> and not really an issue at all (just different from what I expected).
>
> Completed tests:
[...]
Looks good to me. Thanks for your help!
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2018-06-12 19:04 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-07 6:25 [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 01/19] rbd: Drop deprecated -drive parameter "filename" Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 02/19] iscsi: " Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 03/19] block: Add block-specific QDict header Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 04/19] fixup " Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 05/19] qobject: Move block-specific qdict code to block-qdict.c Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars Markus Armbruster
2018-06-11 14:44 ` [Qemu-devel] [Qemu-block] " Max Reitz
2018-06-12 13:38 ` [Qemu-devel] " Kevin Wolf
2018-06-12 16:24 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 07/19] block: Fix -drive " Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 08/19] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts() Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 09/19] block: Factor out qobject_input_visitor_new_flat_confused() Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 10/19] block: Make remaining uses of qobject input visitor more robust Markus Armbruster
2018-06-12 14:43 ` Kevin Wolf
2018-06-12 16:32 ` Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 11/19] block-qdict: Simplify qdict_flatten_qdict() Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 12/19] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist() Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 13/19] block-qdict: Clean up qdict_crumple() a bit Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 14/19] block-qdict: Simplify qdict_is_list() some Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 15/19] check-block-qdict: Rename qdict_flatten()'s variables for clarity Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 16/19] check-block-qdict: Cover flattening of empty lists and dictionaries Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 17/19] block: Fix -blockdev / blockdev-add for empty objects and arrays Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 18/19] rbd: New parameter auth-client-required Markus Armbruster
2018-06-07 6:25 ` [Qemu-devel] [RFC PATCH 19/19] rbd: New parameter key-secret Markus Armbruster
2018-06-07 21:33 ` [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication Jeff Cody
2018-06-12 12:55 ` Jeff Cody
2018-06-12 19:04 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.