* [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication
@ 2018-06-12 12:58 Markus Armbruster
2018-06-12 12:58 ` [Qemu-devel] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename" Markus Armbruster
` (19 more replies)
0 siblings, 20 replies; 32+ messages in thread
From: Markus Armbruster @ 2018-06-12 12:58 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, jcody, eblake
PATCH 01-17 are configuration fixes and cleanup, in particular
-blockdev driver=nfs,... and -drive driver=(nbd|sheepdog|ssh),... with
non-string scalars.
PATCH 18-19 provide support for configuring rbd authentication.
I'm happy to split the series if that helps.
Since the RFC post, I amended the documentation of the rbd
authentication parameters, and squashed PATCH 04 into 03.
Jeff Cody tested the RFC on his Ceph rig. All results match
expectations.
The series clashes with Max's "[PATCH 00/13] block: Try to create well
typed json:{} filenames", but Max has rebased his work on mine.
Markus Armbruster (17):
rbd: Drop deprecated -drive parameter "filename"
iscsi: Drop deprecated -drive parameter "filename"
qobject: Move block-specific qdict code to block-qdict.c
block: Fix -blockdev for certain non-string scalars
block: Fix -drive for certain non-string scalars
block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts()
block: Factor out qobject_input_visitor_new_flat_confused()
block: Make remaining uses of qobject input visitor more robust
block-qdict: Simplify qdict_flatten_qdict()
block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist()
block-qdict: Clean up qdict_crumple() a bit
block-qdict: Simplify qdict_is_list() some
check-block-qdict: Rename qdict_flatten()'s variables for clarity
check-block-qdict: Cover flattening of empty lists and dictionaries
block: Fix -blockdev / blockdev-add for empty objects and arrays
rbd: New parameter auth-client-required
rbd: New parameter key-secret
Max Reitz (1):
block: Add block-specific QDict header
MAINTAINERS | 2 +
block.c | 1 +
block/crypto.c | 6 +-
block/gluster.c | 1 +
block/iscsi.c | 24 +-
block/nbd.c | 16 +-
block/nfs.c | 8 +-
block/parallels.c | 11 +-
block/qcow.c | 11 +-
block/qcow2.c | 11 +-
block/qed.c | 11 +-
block/quorum.c | 1 +
block/rbd.c | 85 +++--
block/sheepdog.c | 23 +-
block/snapshot.c | 1 +
block/ssh.c | 16 +-
block/vdi.c | 4 +-
block/vhdx.c | 11 +-
block/vpc.c | 11 +-
block/vvfat.c | 1 +
block/vxhs.c | 1 +
blockdev.c | 1 +
include/block/qdict.h | 34 ++
include/qapi/qmp/qdict.h | 17 -
qapi/block-core.json | 19 +
qobject/Makefile.objs | 1 +
qobject/block-qdict.c | 722 ++++++++++++++++++++++++++++++++++++++
qobject/qdict.c | 628 ---------------------------------
tests/Makefile.include | 4 +
tests/check-block-qdict.c | 690 ++++++++++++++++++++++++++++++++++++
tests/check-qdict.c | 641 ---------------------------------
tests/check-qobject.c | 1 +
tests/test-replication.c | 1 +
util/qemu-config.c | 1 +
34 files changed, 1577 insertions(+), 1439 deletions(-)
create mode 100644 include/block/qdict.h
create mode 100644 qobject/block-qdict.c
create mode 100644 tests/check-block-qdict.c
--
2.17.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [Qemu-devel] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename" 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 02/18] iscsi: " Markus Armbruster ` (18 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 02/18] iscsi: Drop deprecated -drive parameter "filename" 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename" Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header Markus Armbruster ` (17 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename" Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 02/18] iscsi: " Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 15:02 ` Kevin Wolf 2018-06-12 12:58 ` [Qemu-devel] [PATCH 04/18] qobject: Move block-specific qdict code to block-qdict.c Markus Armbruster ` (16 subsequent siblings) 19 siblings, 1 reply; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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> [Copright note tweaked, superfluous includes dropped] Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block.c | 1 + block/gluster.c | 1 + block/iscsi.c | 1 + block/nbd.c | 1 + block/nfs.c | 1 + block/parallels.c | 1 + block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/quorum.c | 1 + block/rbd.c | 1 + block/sheepdog.c | 1 + block/snapshot.c | 1 + block/ssh.c | 1 + block/vhdx.c | 1 + block/vpc.c | 1 + block/vvfat.c | 1 + block/vxhs.c | 1 + blockdev.c | 1 + include/block/qdict.h | 32 ++++++++++++++++++++++++++++++++ include/qapi/qmp/qdict.h | 17 ----------------- qobject/qdict.c | 1 + tests/check-qdict.c | 1 + tests/check-qobject.c | 1 + tests/test-replication.c | 1 + util/qemu-config.c | 1 + 26 files changed, 56 insertions(+), 17 deletions(-) create mode 100644 include/block/qdict.h diff --git a/block.c b/block.c index 50887087f3..afe30caac3 100644 --- a/block.c +++ b/block.c @@ -27,6 +27,7 @@ #include "block/block_int.h" #include "block/blockjob.h" #include "block/nbd.h" +#include "block/qdict.h" #include "qemu/error-report.h" #include "module_block.h" #include "qemu/module.h" diff --git a/block/gluster.c b/block/gluster.c index 9900b6420c..b5fe7f3e87 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -11,6 +11,7 @@ #include "qemu/osdep.h" #include <glusterfs/api/glfs.h> #include "block/block_int.h" +#include "block/qdict.h" #include "qapi/error.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" diff --git a/block/iscsi.c b/block/iscsi.c index 7e3ea72bd2..9f00fb47a5 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -33,6 +33,7 @@ #include "qemu/bitops.h" #include "qemu/bitmap.h" #include "block/block_int.h" +#include "block/qdict.h" #include "scsi/constants.h" #include "qemu/iov.h" #include "qemu/option.h" diff --git a/block/nbd.c b/block/nbd.c index ff8333e3c1..d6c4c4ddbc 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -28,6 +28,7 @@ #include "qemu/osdep.h" #include "nbd-client.h" +#include "block/qdict.h" #include "qapi/error.h" #include "qemu/uri.h" #include "block/block_int.h" diff --git a/block/nfs.c b/block/nfs.c index 3349b67a76..3170b059b3 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -29,6 +29,7 @@ #include "qemu/error-report.h" #include "qapi/error.h" #include "block/block_int.h" +#include "block/qdict.h" #include "trace.h" #include "qemu/iov.h" #include "qemu/option.h" diff --git a/block/parallels.c b/block/parallels.c index 6e9c37f44e..c1d9643498 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -31,6 +31,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "block/block_int.h" +#include "block/qdict.h" #include "sysemu/block-backend.h" #include "qemu/module.h" #include "qemu/option.h" diff --git a/block/qcow.c b/block/qcow.c index 1f866af0d3..8c08908fd8 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -26,6 +26,7 @@ #include "qapi/error.h" #include "qemu/error-report.h" #include "block/block_int.h" +#include "block/qdict.h" #include "sysemu/block-backend.h" #include "qemu/module.h" #include "qemu/option.h" diff --git a/block/qcow2.c b/block/qcow2.c index 6fa5e1d71a..d2d955f984 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -24,6 +24,7 @@ #include "qemu/osdep.h" #include "block/block_int.h" +#include "block/qdict.h" #include "sysemu/block-backend.h" #include "qemu/module.h" #include <zlib.h> diff --git a/block/qed.c b/block/qed.c index 65cfe92393..324a953cbc 100644 --- a/block/qed.c +++ b/block/qed.c @@ -13,6 +13,7 @@ */ #include "qemu/osdep.h" +#include "block/qdict.h" #include "qapi/error.h" #include "qemu/timer.h" #include "qemu/bswap.h" diff --git a/block/quorum.c b/block/quorum.c index b6476c405a..9152da8c58 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -17,6 +17,7 @@ #include "qemu/cutils.h" #include "qemu/option.h" #include "block/block_int.h" +#include "block/qdict.h" #include "qapi/error.h" #include "qapi/qapi-events-block.h" #include "qapi/qmp/qdict.h" diff --git a/block/rbd.c b/block/rbd.c index 40c6e4185f..9659c7361f 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -18,6 +18,7 @@ #include "qemu/error-report.h" #include "qemu/option.h" #include "block/block_int.h" +#include "block/qdict.h" #include "crypto/secret.h" #include "qemu/cutils.h" #include "qapi/qmp/qstring.h" diff --git a/block/sheepdog.c b/block/sheepdog.c index 7b98725af7..2e1f0e6eca 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -24,6 +24,7 @@ #include "qemu/option.h" #include "qemu/sockets.h" #include "block/block_int.h" +#include "block/qdict.h" #include "sysemu/block-backend.h" #include "qemu/bitops.h" #include "qemu/cutils.h" diff --git a/block/snapshot.c b/block/snapshot.c index 2953d96c06..f9903bc94e 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -25,6 +25,7 @@ #include "qemu/osdep.h" #include "block/snapshot.h" #include "block/block_int.h" +#include "block/qdict.h" #include "qapi/error.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" diff --git a/block/ssh.c b/block/ssh.c index 4c4fa3ccfc..eec37dd27c 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -28,6 +28,7 @@ #include <libssh2_sftp.h> #include "block/block_int.h" +#include "block/qdict.h" #include "qapi/error.h" #include "qemu/error-report.h" #include "qemu/option.h" diff --git a/block/vhdx.c b/block/vhdx.c index 0831c5c5f4..2e32e24514 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -18,6 +18,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "block/block_int.h" +#include "block/qdict.h" #include "sysemu/block-backend.h" #include "qemu/module.h" #include "qemu/option.h" diff --git a/block/vpc.c b/block/vpc.c index 0ebfcd3cc8..41c8c980f1 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -26,6 +26,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "block/block_int.h" +#include "block/qdict.h" #include "sysemu/block-backend.h" #include "qemu/module.h" #include "qemu/option.h" diff --git a/block/vvfat.c b/block/vvfat.c index 662dca0114..4595f335b8 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -27,6 +27,7 @@ #include <dirent.h> #include "qapi/error.h" #include "block/block_int.h" +#include "block/qdict.h" #include "qemu/module.h" #include "qemu/option.h" #include "qemu/bswap.h" diff --git a/block/vxhs.c b/block/vxhs.c index 339e23218d..0cb0a007e9 100644 --- a/block/vxhs.c +++ b/block/vxhs.c @@ -12,6 +12,7 @@ #include <qnio/qnio_api.h> #include <sys/param.h> #include "block/block_int.h" +#include "block/qdict.h" #include "qapi/qmp/qerror.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" diff --git a/blockdev.c b/blockdev.c index 8de95be8f4..721dc9a4fc 100644 --- a/blockdev.c +++ b/blockdev.c @@ -35,6 +35,7 @@ #include "sysemu/blockdev.h" #include "hw/block/block.h" #include "block/blockjob.h" +#include "block/qdict.h" #include "block/throttle-groups.h" #include "monitor/monitor.h" #include "qemu/error-report.h" diff --git a/include/block/qdict.h b/include/block/qdict.h new file mode 100644 index 0000000000..71c037afba --- /dev/null +++ b/include/block/qdict.h @@ -0,0 +1,32 @@ +/* + * Special QDict functions used by the block layer + * + * Copyright (c) 2013-2018 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + */ + +#ifndef BLOCK_QDICT_H +#define BLOCK_QDICT_H + +#include "qapi/qmp/qdict.h" + +void qdict_copy_default(QDict *dst, QDict *src, const char *key); +void qdict_set_default_str(QDict *dst, const char *key, const char *val); + +void qdict_join(QDict *dest, QDict *src, bool overwrite); + +void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start); +void qdict_array_split(QDict *src, QList **dst); +int qdict_array_entries(QDict *src, const char *subqdict); +QObject *qdict_crumple(const QDict *src, Error **errp); +void qdict_flatten(QDict *qdict); + +typedef struct QDictRenames { + const char *from; + const char *to; +} QDictRenames; +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp); + +#endif diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h index 921a28d2d3..7f3ec10a10 100644 --- a/include/qapi/qmp/qdict.h +++ b/include/qapi/qmp/qdict.h @@ -67,23 +67,6 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key, bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value); const char *qdict_get_try_str(const QDict *qdict, const char *key); -void qdict_copy_default(QDict *dst, QDict *src, const char *key); -void qdict_set_default_str(QDict *dst, const char *key, const char *val); - QDict *qdict_clone_shallow(const QDict *src); -void qdict_flatten(QDict *qdict); - -void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start); -void qdict_array_split(QDict *src, QList **dst); -int qdict_array_entries(QDict *src, const char *subqdict); -QObject *qdict_crumple(const QDict *src, Error **errp); - -void qdict_join(QDict *dest, QDict *src, bool overwrite); - -typedef struct QDictRenames { - const char *from; - const char *to; -} QDictRenames; -bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp); #endif /* QDICT_H */ diff --git a/qobject/qdict.c b/qobject/qdict.c index 22800eeceb..0554c64553 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -11,6 +11,7 @@ */ #include "qemu/osdep.h" +#include "block/qdict.h" #include "qapi/qmp/qnum.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qbool.h" diff --git a/tests/check-qdict.c b/tests/check-qdict.c index eba5d3528e..93e2112b6d 100644 --- a/tests/check-qdict.c +++ b/tests/check-qdict.c @@ -11,6 +11,7 @@ */ #include "qemu/osdep.h" +#include "block/qdict.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qlist.h" #include "qapi/qmp/qnum.h" diff --git a/tests/check-qobject.c b/tests/check-qobject.c index 5cb08fcb63..16ccbde82c 100644 --- a/tests/check-qobject.c +++ b/tests/check-qobject.c @@ -8,6 +8,7 @@ */ #include "qemu/osdep.h" +#include "block/qdict.h" #include "qapi/qmp/qbool.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qlist.h" diff --git a/tests/test-replication.c b/tests/test-replication.c index 68c0d04f2a..c8165ae954 100644 --- a/tests/test-replication.c +++ b/tests/test-replication.c @@ -15,6 +15,7 @@ #include "qemu/option.h" #include "replication.h" #include "block/block_int.h" +#include "block/qdict.h" #include "sysemu/block-backend.h" #define IMG_SIZE (64 * 1024 * 1024) diff --git a/util/qemu-config.c b/util/qemu-config.c index 14d84022dc..9d2e278e29 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -1,4 +1,5 @@ #include "qemu/osdep.h" +#include "block/qdict.h" /* for qdict_extract_subqdict() */ #include "qapi/error.h" #include "qapi/qapi-commands-misc.h" #include "qapi/qmp/qdict.h" -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header 2018-06-12 12:58 ` [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header Markus Armbruster @ 2018-06-12 15:02 ` Kevin Wolf 2018-06-12 16:40 ` Markus Armbruster 0 siblings, 1 reply; 32+ messages in thread From: Kevin Wolf @ 2018-06-12 15:02 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, qemu-block, mreitz, jcody, eblake Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > From: Max Reitz <mreitz@redhat.com> > > There are numerous QDict functions that have been introduced for and are > used only by the block layer. Move their declarations into an own > header file to reflect that. > > While qdict_extract_subqdict() is in fact used outside of the block > layer (in util/qemu-config.c), it is still a function related very > closely to how the block layer works with nested QDicts, namely by > sometimes flattening them. Therefore, its declaration is put into this > header as well and util/qemu-config.c includes it with a comment stating > exactly which function it needs. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Max Reitz <mreitz@redhat.com> > Message-Id: <20180509165530.29561-7-mreitz@redhat.com> > [Copright note tweaked, superfluous includes dropped] s/Copright/Copyright/ Kevin ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header 2018-06-12 15:02 ` Kevin Wolf @ 2018-06-12 16:40 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 16:40 UTC (permalink / raw) To: Kevin Wolf; +Cc: Markus Armbruster, jcody, qemu-devel, qemu-block, mreitz Kevin Wolf <kwolf@redhat.com> writes: > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: >> From: Max Reitz <mreitz@redhat.com> >> >> There are numerous QDict functions that have been introduced for and are >> used only by the block layer. Move their declarations into an own >> header file to reflect that. >> >> While qdict_extract_subqdict() is in fact used outside of the block >> layer (in util/qemu-config.c), it is still a function related very >> closely to how the block layer works with nested QDicts, namely by >> sometimes flattening them. Therefore, its declaration is put into this >> header as well and util/qemu-config.c includes it with a comment stating >> exactly which function it needs. >> >> Suggested-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> Message-Id: <20180509165530.29561-7-mreitz@redhat.com> >> [Copright note tweaked, superfluous includes dropped] > > s/Copright/Copyright/ Fixing, thanks! ^ permalink raw reply [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 04/18] qobject: Move block-specific qdict code to block-qdict.c 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (2 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 05/18] block: Fix -blockdev for certain non-string scalars Markus Armbruster ` (15 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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 a40f558694..f44ef871d0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1366,6 +1366,8 @@ F: qemu-img* F: qemu-io* F: tests/qemu-iotests/ F: util/qemu-progress.c +F: qobject/block-qdict.c +F: test/check-block-qdict.c T: git git://repo.or.cz/qemu/kevin.git block Block I/O path diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs index 002d25873a..7b12c9cacf 100644 --- a/qobject/Makefile.objs +++ b/qobject/Makefile.objs @@ -1,2 +1,3 @@ util-obj-y = qnull.o qnum.o qstring.o qdict.o qlist.o qbool.o qlit.o util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o +util-obj-y += block-qdict.o diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c new file mode 100644 index 0000000000..fb92010dc5 --- /dev/null +++ b/qobject/block-qdict.c @@ -0,0 +1,640 @@ +/* + * Special QDict functions used by the block layer + * + * Copyright (c) 2013-2018 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "block/qdict.h" +#include "qapi/qmp/qlist.h" +#include "qemu/cutils.h" +#include "qapi/error.h" + +/** + * qdict_copy_default(): If no entry mapped by 'key' exists in 'dst' yet, the + * value of 'key' in 'src' is copied there (and the refcount increased + * accordingly). + */ +void qdict_copy_default(QDict *dst, QDict *src, const char *key) +{ + QObject *val; + + if (qdict_haskey(dst, key)) { + return; + } + + val = qdict_get(src, key); + if (val) { + qdict_put_obj(dst, key, qobject_ref(val)); + } +} + +/** + * qdict_set_default_str(): If no entry mapped by 'key' exists in 'dst' yet, a + * new QString initialised by 'val' is put there. + */ +void qdict_set_default_str(QDict *dst, const char *key, const char *val) +{ + if (qdict_haskey(dst, key)) { + return; + } + + qdict_put_str(dst, key, val); +} + +static void qdict_flatten_qdict(QDict *qdict, QDict *target, + const char *prefix); + +static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix) +{ + QObject *value; + const QListEntry *entry; + char *new_key; + int i; + + /* This function is never called with prefix == NULL, i.e., it is always + * called from within qdict_flatten_q(list|dict)(). Therefore, it does not + * need to remove list entries during the iteration (the whole list will be + * deleted eventually anyway from qdict_flatten_qdict()). */ + assert(prefix); + + entry = qlist_first(qlist); + + for (i = 0; entry; entry = qlist_next(entry), i++) { + value = qlist_entry_obj(entry); + new_key = g_strdup_printf("%s.%i", prefix, i); + + if (qobject_type(value) == QTYPE_QDICT) { + qdict_flatten_qdict(qobject_to(QDict, value), target, new_key); + } else if (qobject_type(value) == QTYPE_QLIST) { + qdict_flatten_qlist(qobject_to(QList, value), target, new_key); + } else { + /* All other types are moved to the target unchanged. */ + qdict_put_obj(target, new_key, qobject_ref(value)); + } + + g_free(new_key); + } +} + +static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix) +{ + QObject *value; + const QDictEntry *entry, *next; + char *new_key; + bool delete; + + entry = qdict_first(qdict); + + while (entry != NULL) { + + next = qdict_next(qdict, entry); + value = qdict_entry_value(entry); + new_key = NULL; + delete = false; + + if (prefix) { + new_key = g_strdup_printf("%s.%s", prefix, entry->key); + } + + if (qobject_type(value) == QTYPE_QDICT) { + /* Entries of QDicts are processed recursively, the QDict object + * itself disappears. */ + qdict_flatten_qdict(qobject_to(QDict, value), target, + new_key ? new_key : entry->key); + delete = true; + } else if (qobject_type(value) == QTYPE_QLIST) { + qdict_flatten_qlist(qobject_to(QList, value), target, + new_key ? new_key : entry->key); + delete = true; + } else if (prefix) { + /* All other objects are moved to the target unchanged. */ + qdict_put_obj(target, new_key, qobject_ref(value)); + delete = true; + } + + g_free(new_key); + + if (delete) { + qdict_del(qdict, entry->key); + + /* Restart loop after modifying the iterated QDict */ + entry = qdict_first(qdict); + continue; + } + + entry = next; + } +} + +/** + * qdict_flatten(): For each nested QDict with key x, all fields with key y + * are moved to this QDict and their key is renamed to "x.y". For each nested + * QList with key x, the field at index y is moved to this QDict with the key + * "x.y" (i.e., the reverse of what qdict_array_split() does). + * This operation is applied recursively for nested QDicts and QLists. + */ +void qdict_flatten(QDict *qdict) +{ + qdict_flatten_qdict(qdict, qdict, NULL); +} + +/* extract all the src QDict entries starting by start into dst */ +void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start) + +{ + const QDictEntry *entry, *next; + const char *p; + + *dst = qdict_new(); + entry = qdict_first(src); + + while (entry != NULL) { + next = qdict_next(src, entry); + if (strstart(entry->key, start, &p)) { + qdict_put_obj(*dst, p, qobject_ref(entry->value)); + qdict_del(src, entry->key); + } + entry = next; + } +} + +static int qdict_count_prefixed_entries(const QDict *src, const char *start) +{ + const QDictEntry *entry; + int count = 0; + + for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) { + if (strstart(entry->key, start, NULL)) { + if (count == INT_MAX) { + return -ERANGE; + } + count++; + } + } + + return count; +} + +/** + * qdict_array_split(): This function moves array-like elements of a QDict into + * a new QList. Every entry in the original QDict with a key "%u" or one + * prefixed "%u.", where %u designates an unsigned integer starting at 0 and + * incrementally counting up, will be moved to a new QDict at index %u in the + * output QList with the key prefix removed, if that prefix is "%u.". If the + * whole key is just "%u", the whole QObject will be moved unchanged without + * creating a new QDict. The function terminates when there is no entry in the + * QDict with a prefix directly (incrementally) following the last one; it also + * returns if there are both entries with "%u" and "%u." for the same index %u. + * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "4.y": 1, "o.o": 7, "2": 66} + * (or {"1.x": 0, "4.y": 1, "0.a": 42, "o.o": 7, "0.b": 23, "2": 66}) + * => [{"a": 42, "b": 23}, {"x": 0}, 66] + * and {"4.y": 1, "o.o": 7} (remainder of the old QDict) + */ +void qdict_array_split(QDict *src, QList **dst) +{ + unsigned i; + + *dst = qlist_new(); + + for (i = 0; i < UINT_MAX; i++) { + QObject *subqobj; + bool is_subqdict; + QDict *subqdict; + char indexstr[32], prefix[32]; + size_t snprintf_ret; + + snprintf_ret = snprintf(indexstr, 32, "%u", i); + assert(snprintf_ret < 32); + + subqobj = qdict_get(src, indexstr); + + snprintf_ret = snprintf(prefix, 32, "%u.", i); + assert(snprintf_ret < 32); + + /* Overflow is the same as positive non-zero results */ + is_subqdict = qdict_count_prefixed_entries(src, prefix); + + /* + * There may be either a single subordinate object (named + * "%u") or multiple objects (each with a key prefixed "%u."), + * but not both. + */ + if (!subqobj == !is_subqdict) { + break; + } + + if (is_subqdict) { + qdict_extract_subqdict(src, &subqdict, prefix); + assert(qdict_size(subqdict) > 0); + } else { + qobject_ref(subqobj); + qdict_del(src, indexstr); + } + + qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict)); + } +} + +/** + * qdict_split_flat_key: + * @key: the key string to split + * @prefix: non-NULL pointer to hold extracted prefix + * @suffix: non-NULL pointer to remaining suffix + * + * Given a flattened key such as 'foo.0.bar', split it into two parts + * at the first '.' separator. Allows double dot ('..') to escape the + * normal separator. + * + * e.g. + * 'foo.0.bar' -> prefix='foo' and suffix='0.bar' + * 'foo..0.bar' -> prefix='foo.0' and suffix='bar' + * + * The '..' sequence will be unescaped in the returned 'prefix' + * string. The 'suffix' string will be left in escaped format, so it + * can be fed back into the qdict_split_flat_key() key as the input + * later. + * + * The caller is responsible for freeing the string returned in @prefix + * using g_free(). + */ +static void qdict_split_flat_key(const char *key, char **prefix, + const char **suffix) +{ + const char *separator; + size_t i, j; + + /* Find first '.' separator, but if there is a pair '..' + * that acts as an escape, so skip over '..' */ + separator = NULL; + do { + if (separator) { + separator += 2; + } else { + separator = key; + } + separator = strchr(separator, '.'); + } while (separator && separator[1] == '.'); + + if (separator) { + *prefix = g_strndup(key, separator - key); + *suffix = separator + 1; + } else { + *prefix = g_strdup(key); + *suffix = NULL; + } + + /* Unescape the '..' sequence into '.' */ + for (i = 0, j = 0; (*prefix)[i] != '\0'; i++, j++) { + if ((*prefix)[i] == '.') { + assert((*prefix)[i + 1] == '.'); + i++; + } + (*prefix)[j] = (*prefix)[i]; + } + (*prefix)[j] = '\0'; +} + +/** + * qdict_is_list: + * @maybe_list: dict to check if keys represent list elements. + * + * Determine whether all keys in @maybe_list are valid list elements. + * If @maybe_list is non-zero in length and all the keys look like + * valid list indexes, this will return 1. If @maybe_list is zero + * length or all keys are non-numeric then it will return 0 to indicate + * it is a normal qdict. If there is a mix of numeric and non-numeric + * keys, or the list indexes are non-contiguous, an error is reported. + * + * Returns: 1 if a valid list, 0 if a dict, -1 on error + */ +static int qdict_is_list(QDict *maybe_list, Error **errp) +{ + const QDictEntry *ent; + ssize_t len = 0; + ssize_t max = -1; + int is_list = -1; + int64_t val; + + for (ent = qdict_first(maybe_list); ent != NULL; + ent = qdict_next(maybe_list, ent)) { + + if (qemu_strtoi64(ent->key, NULL, 10, &val) == 0) { + if (is_list == -1) { + is_list = 1; + } else if (!is_list) { + error_setg(errp, + "Cannot mix list and non-list keys"); + return -1; + } + len++; + if (val > max) { + max = val; + } + } else { + if (is_list == -1) { + is_list = 0; + } else if (is_list) { + error_setg(errp, + "Cannot mix list and non-list keys"); + return -1; + } + } + } + + if (is_list == -1) { + assert(!qdict_size(maybe_list)); + is_list = 0; + } + + /* NB this isn't a perfect check - e.g. it won't catch + * a list containing '1', '+1', '01', '3', but that + * does not matter - we've still proved that the + * input is a list. It is up the caller to do a + * stricter check if desired */ + if (len != (max + 1)) { + error_setg(errp, "List indices are not contiguous, " + "saw %zd elements but %zd largest index", + len, max); + return -1; + } + + return is_list; +} + +/** + * qdict_crumple: + * @src: the original flat dictionary (only scalar values) to crumple + * + * Takes a flat dictionary whose keys use '.' separator to indicate + * nesting, and values are scalars, and crumples it into a nested + * structure. + * + * To include a literal '.' in a key name, it must be escaped as '..' + * + * For example, an input of: + * + * { 'foo.0.bar': 'one', 'foo.0.wizz': '1', + * 'foo.1.bar': 'two', 'foo.1.wizz': '2' } + * + * will result in an output of: + * + * { + * 'foo': [ + * { 'bar': 'one', 'wizz': '1' }, + * { 'bar': 'two', 'wizz': '2' } + * ], + * } + * + * The following scenarios in the input dict will result in an + * error being returned: + * + * - Any values in @src are non-scalar types + * - If keys in @src imply that a particular level is both a + * list and a dict. e.g., "foo.0.bar" and "foo.eek.bar". + * - If keys in @src imply that a particular level is a list, + * but the indices are non-contiguous. e.g. "foo.0.bar" and + * "foo.2.bar" without any "foo.1.bar" present. + * - If keys in @src represent list indexes, but are not in + * the "%zu" format. e.g. "foo.+0.bar" + * + * Returns: either a QDict or QList for the nested data structure, or NULL + * on error + */ +QObject *qdict_crumple(const QDict *src, Error **errp) +{ + const QDictEntry *ent; + QDict *two_level, *multi_level = NULL; + QObject *dst = NULL, *child; + size_t i; + char *prefix = NULL; + const char *suffix = NULL; + int is_list; + + two_level = qdict_new(); + + /* Step 1: split our totally flat dict into a two level dict */ + for (ent = qdict_first(src); ent != NULL; ent = qdict_next(src, ent)) { + if (qobject_type(ent->value) == QTYPE_QDICT || + qobject_type(ent->value) == QTYPE_QLIST) { + error_setg(errp, "Value %s is not a scalar", + ent->key); + goto error; + } + + qdict_split_flat_key(ent->key, &prefix, &suffix); + + child = qdict_get(two_level, prefix); + if (suffix) { + QDict *child_dict = qobject_to(QDict, child); + if (!child_dict) { + if (child) { + error_setg(errp, "Key %s prefix is already set as a scalar", + prefix); + goto error; + } + + child_dict = qdict_new(); + qdict_put_obj(two_level, prefix, QOBJECT(child_dict)); + } + + qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); + } else { + if (child) { + error_setg(errp, "Key %s prefix is already set as a dict", + prefix); + goto error; + } + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); + } + + g_free(prefix); + prefix = NULL; + } + + /* Step 2: optionally process the two level dict recursively + * into a multi-level dict */ + multi_level = qdict_new(); + for (ent = qdict_first(two_level); ent != NULL; + ent = qdict_next(two_level, ent)) { + QDict *dict = qobject_to(QDict, ent->value); + if (dict) { + child = qdict_crumple(dict, errp); + if (!child) { + goto error; + } + + qdict_put_obj(multi_level, ent->key, child); + } else { + qdict_put_obj(multi_level, ent->key, qobject_ref(ent->value)); + } + } + qobject_unref(two_level); + two_level = NULL; + + /* Step 3: detect if we need to turn our dict into list */ + is_list = qdict_is_list(multi_level, errp); + if (is_list < 0) { + goto error; + } + + if (is_list) { + dst = QOBJECT(qlist_new()); + + for (i = 0; i < qdict_size(multi_level); i++) { + char *key = g_strdup_printf("%zu", i); + + child = qdict_get(multi_level, key); + g_free(key); + + if (!child) { + error_setg(errp, "Missing list index %zu", i); + goto error; + } + + qlist_append_obj(qobject_to(QList, dst), qobject_ref(child)); + } + qobject_unref(multi_level); + multi_level = NULL; + } else { + dst = QOBJECT(multi_level); + } + + return dst; + + error: + g_free(prefix); + qobject_unref(multi_level); + qobject_unref(two_level); + qobject_unref(dst); + return NULL; +} + +/** + * qdict_array_entries(): Returns the number of direct array entries if the + * sub-QDict of src specified by the prefix in subqdict (or src itself for + * prefix == "") is valid as an array, i.e. the length of the created list if + * the sub-QDict would become empty after calling qdict_array_split() on it. If + * the array is not valid, -EINVAL is returned. + */ +int qdict_array_entries(QDict *src, const char *subqdict) +{ + const QDictEntry *entry; + unsigned i; + unsigned entries = 0; + size_t subqdict_len = strlen(subqdict); + + assert(!subqdict_len || subqdict[subqdict_len - 1] == '.'); + + /* qdict_array_split() loops until UINT_MAX, but as we want to return + * negative errors, we only have a signed return value here. Any additional + * entries will lead to -EINVAL. */ + for (i = 0; i < INT_MAX; i++) { + QObject *subqobj; + int subqdict_entries; + char *prefix = g_strdup_printf("%s%u.", subqdict, i); + + subqdict_entries = qdict_count_prefixed_entries(src, prefix); + + /* Remove ending "." */ + prefix[strlen(prefix) - 1] = 0; + subqobj = qdict_get(src, prefix); + + g_free(prefix); + + if (subqdict_entries < 0) { + return subqdict_entries; + } + + /* There may be either a single subordinate object (named "%u") or + * multiple objects (each with a key prefixed "%u."), but not both. */ + if (subqobj && subqdict_entries) { + return -EINVAL; + } else if (!subqobj && !subqdict_entries) { + break; + } + + entries += subqdict_entries ? subqdict_entries : 1; + } + + /* Consider everything handled that isn't part of the given sub-QDict */ + for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) { + if (!strstart(qdict_entry_key(entry), subqdict, NULL)) { + entries++; + } + } + + /* Anything left in the sub-QDict that wasn't handled? */ + if (qdict_size(src) != entries) { + return -EINVAL; + } + + return i; +} + +/** + * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all + * elements from src to dest. + * + * If an element from src has a key already present in dest, it will not be + * moved unless overwrite is true. + * + * If overwrite is true, the conflicting values in dest will be discarded and + * replaced by the corresponding values from src. + * + * Therefore, with overwrite being true, the src QDict will always be empty when + * this function returns. If overwrite is false, the src QDict will be empty + * iff there were no conflicts. + */ +void qdict_join(QDict *dest, QDict *src, bool overwrite) +{ + const QDictEntry *entry, *next; + + entry = qdict_first(src); + while (entry) { + next = qdict_next(src, entry); + + if (overwrite || !qdict_haskey(dest, entry->key)) { + qdict_put_obj(dest, entry->key, qobject_ref(entry->value)); + qdict_del(src, entry->key); + } + + entry = next; + } +} + +/** + * qdict_rename_keys(): Rename keys in qdict according to the replacements + * specified in the array renames. The array must be terminated by an entry + * with from = NULL. + * + * The renames are performed individually in the order of the array, so entries + * may be renamed multiple times and may or may not conflict depending on the + * order of the renames array. + * + * Returns true for success, false in error cases. + */ +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp) +{ + QObject *qobj; + + while (renames->from) { + if (qdict_haskey(qdict, renames->from)) { + if (qdict_haskey(qdict, renames->to)) { + error_setg(errp, "'%s' and its alias '%s' can't be used at the " + "same time", renames->to, renames->from); + return false; + } + + qobj = qdict_get(qdict, renames->from); + qdict_put_obj(qdict, renames->to, qobject_ref(qobj)); + qdict_del(qdict, renames->from); + } + + renames++; + } + return true; +} diff --git a/qobject/qdict.c b/qobject/qdict.c index 0554c64553..3d8c2f7bbc 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -11,17 +11,11 @@ */ #include "qemu/osdep.h" -#include "block/qdict.h" #include "qapi/qmp/qnum.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qbool.h" -#include "qapi/qmp/qlist.h" #include "qapi/qmp/qnull.h" #include "qapi/qmp/qstring.h" -#include "qapi/error.h" -#include "qemu/queue.h" -#include "qemu-common.h" -#include "qemu/cutils.h" /** * qdict_new(): Create a new QDict @@ -464,626 +458,3 @@ void qdict_destroy_obj(QObject *obj) g_free(qdict); } - -/** - * qdict_copy_default(): If no entry mapped by 'key' exists in 'dst' yet, the - * value of 'key' in 'src' is copied there (and the refcount increased - * accordingly). - */ -void qdict_copy_default(QDict *dst, QDict *src, const char *key) -{ - QObject *val; - - if (qdict_haskey(dst, key)) { - return; - } - - val = qdict_get(src, key); - if (val) { - qdict_put_obj(dst, key, qobject_ref(val)); - } -} - -/** - * qdict_set_default_str(): If no entry mapped by 'key' exists in 'dst' yet, a - * new QString initialised by 'val' is put there. - */ -void qdict_set_default_str(QDict *dst, const char *key, const char *val) -{ - if (qdict_haskey(dst, key)) { - return; - } - - qdict_put_str(dst, key, val); -} - -static void qdict_flatten_qdict(QDict *qdict, QDict *target, - const char *prefix); - -static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix) -{ - QObject *value; - const QListEntry *entry; - char *new_key; - int i; - - /* This function is never called with prefix == NULL, i.e., it is always - * called from within qdict_flatten_q(list|dict)(). Therefore, it does not - * need to remove list entries during the iteration (the whole list will be - * deleted eventually anyway from qdict_flatten_qdict()). */ - assert(prefix); - - entry = qlist_first(qlist); - - for (i = 0; entry; entry = qlist_next(entry), i++) { - value = qlist_entry_obj(entry); - new_key = g_strdup_printf("%s.%i", prefix, i); - - if (qobject_type(value) == QTYPE_QDICT) { - qdict_flatten_qdict(qobject_to(QDict, value), target, new_key); - } else if (qobject_type(value) == QTYPE_QLIST) { - qdict_flatten_qlist(qobject_to(QList, value), target, new_key); - } else { - /* All other types are moved to the target unchanged. */ - qdict_put_obj(target, new_key, qobject_ref(value)); - } - - g_free(new_key); - } -} - -static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix) -{ - QObject *value; - const QDictEntry *entry, *next; - char *new_key; - bool delete; - - entry = qdict_first(qdict); - - while (entry != NULL) { - - next = qdict_next(qdict, entry); - value = qdict_entry_value(entry); - new_key = NULL; - delete = false; - - if (prefix) { - new_key = g_strdup_printf("%s.%s", prefix, entry->key); - } - - if (qobject_type(value) == QTYPE_QDICT) { - /* Entries of QDicts are processed recursively, the QDict object - * itself disappears. */ - qdict_flatten_qdict(qobject_to(QDict, value), target, - new_key ? new_key : entry->key); - delete = true; - } else if (qobject_type(value) == QTYPE_QLIST) { - qdict_flatten_qlist(qobject_to(QList, value), target, - new_key ? new_key : entry->key); - delete = true; - } else if (prefix) { - /* All other objects are moved to the target unchanged. */ - qdict_put_obj(target, new_key, qobject_ref(value)); - delete = true; - } - - g_free(new_key); - - if (delete) { - qdict_del(qdict, entry->key); - - /* Restart loop after modifying the iterated QDict */ - entry = qdict_first(qdict); - continue; - } - - entry = next; - } -} - -/** - * qdict_flatten(): For each nested QDict with key x, all fields with key y - * are moved to this QDict and their key is renamed to "x.y". For each nested - * QList with key x, the field at index y is moved to this QDict with the key - * "x.y" (i.e., the reverse of what qdict_array_split() does). - * This operation is applied recursively for nested QDicts and QLists. - */ -void qdict_flatten(QDict *qdict) -{ - qdict_flatten_qdict(qdict, qdict, NULL); -} - -/* extract all the src QDict entries starting by start into dst */ -void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start) - -{ - const QDictEntry *entry, *next; - const char *p; - - *dst = qdict_new(); - entry = qdict_first(src); - - while (entry != NULL) { - next = qdict_next(src, entry); - if (strstart(entry->key, start, &p)) { - qdict_put_obj(*dst, p, qobject_ref(entry->value)); - qdict_del(src, entry->key); - } - entry = next; - } -} - -static int qdict_count_prefixed_entries(const QDict *src, const char *start) -{ - const QDictEntry *entry; - int count = 0; - - for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) { - if (strstart(entry->key, start, NULL)) { - if (count == INT_MAX) { - return -ERANGE; - } - count++; - } - } - - return count; -} - -/** - * qdict_array_split(): This function moves array-like elements of a QDict into - * a new QList. Every entry in the original QDict with a key "%u" or one - * prefixed "%u.", where %u designates an unsigned integer starting at 0 and - * incrementally counting up, will be moved to a new QDict at index %u in the - * output QList with the key prefix removed, if that prefix is "%u.". If the - * whole key is just "%u", the whole QObject will be moved unchanged without - * creating a new QDict. The function terminates when there is no entry in the - * QDict with a prefix directly (incrementally) following the last one; it also - * returns if there are both entries with "%u" and "%u." for the same index %u. - * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "4.y": 1, "o.o": 7, "2": 66} - * (or {"1.x": 0, "4.y": 1, "0.a": 42, "o.o": 7, "0.b": 23, "2": 66}) - * => [{"a": 42, "b": 23}, {"x": 0}, 66] - * and {"4.y": 1, "o.o": 7} (remainder of the old QDict) - */ -void qdict_array_split(QDict *src, QList **dst) -{ - unsigned i; - - *dst = qlist_new(); - - for (i = 0; i < UINT_MAX; i++) { - QObject *subqobj; - bool is_subqdict; - QDict *subqdict; - char indexstr[32], prefix[32]; - size_t snprintf_ret; - - snprintf_ret = snprintf(indexstr, 32, "%u", i); - assert(snprintf_ret < 32); - - subqobj = qdict_get(src, indexstr); - - snprintf_ret = snprintf(prefix, 32, "%u.", i); - assert(snprintf_ret < 32); - - /* Overflow is the same as positive non-zero results */ - is_subqdict = qdict_count_prefixed_entries(src, prefix); - - // There may be either a single subordinate object (named "%u") or - // multiple objects (each with a key prefixed "%u."), but not both. - if (!subqobj == !is_subqdict) { - break; - } - - if (is_subqdict) { - qdict_extract_subqdict(src, &subqdict, prefix); - assert(qdict_size(subqdict) > 0); - } else { - qobject_ref(subqobj); - qdict_del(src, indexstr); - } - - qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict)); - } -} - -/** - * qdict_split_flat_key: - * @key: the key string to split - * @prefix: non-NULL pointer to hold extracted prefix - * @suffix: non-NULL pointer to remaining suffix - * - * Given a flattened key such as 'foo.0.bar', split it into two parts - * at the first '.' separator. Allows double dot ('..') to escape the - * normal separator. - * - * e.g. - * 'foo.0.bar' -> prefix='foo' and suffix='0.bar' - * 'foo..0.bar' -> prefix='foo.0' and suffix='bar' - * - * The '..' sequence will be unescaped in the returned 'prefix' - * string. The 'suffix' string will be left in escaped format, so it - * can be fed back into the qdict_split_flat_key() key as the input - * later. - * - * The caller is responsible for freeing the string returned in @prefix - * using g_free(). - */ -static void qdict_split_flat_key(const char *key, char **prefix, - const char **suffix) -{ - const char *separator; - size_t i, j; - - /* Find first '.' separator, but if there is a pair '..' - * that acts as an escape, so skip over '..' */ - separator = NULL; - do { - if (separator) { - separator += 2; - } else { - separator = key; - } - separator = strchr(separator, '.'); - } while (separator && separator[1] == '.'); - - if (separator) { - *prefix = g_strndup(key, separator - key); - *suffix = separator + 1; - } else { - *prefix = g_strdup(key); - *suffix = NULL; - } - - /* Unescape the '..' sequence into '.' */ - for (i = 0, j = 0; (*prefix)[i] != '\0'; i++, j++) { - if ((*prefix)[i] == '.') { - assert((*prefix)[i + 1] == '.'); - i++; - } - (*prefix)[j] = (*prefix)[i]; - } - (*prefix)[j] = '\0'; -} - -/** - * qdict_is_list: - * @maybe_list: dict to check if keys represent list elements. - * - * Determine whether all keys in @maybe_list are valid list elements. - * If @maybe_list is non-zero in length and all the keys look like - * valid list indexes, this will return 1. If @maybe_list is zero - * length or all keys are non-numeric then it will return 0 to indicate - * it is a normal qdict. If there is a mix of numeric and non-numeric - * keys, or the list indexes are non-contiguous, an error is reported. - * - * Returns: 1 if a valid list, 0 if a dict, -1 on error - */ -static int qdict_is_list(QDict *maybe_list, Error **errp) -{ - const QDictEntry *ent; - ssize_t len = 0; - ssize_t max = -1; - int is_list = -1; - int64_t val; - - for (ent = qdict_first(maybe_list); ent != NULL; - ent = qdict_next(maybe_list, ent)) { - - if (qemu_strtoi64(ent->key, NULL, 10, &val) == 0) { - if (is_list == -1) { - is_list = 1; - } else if (!is_list) { - error_setg(errp, - "Cannot mix list and non-list keys"); - return -1; - } - len++; - if (val > max) { - max = val; - } - } else { - if (is_list == -1) { - is_list = 0; - } else if (is_list) { - error_setg(errp, - "Cannot mix list and non-list keys"); - return -1; - } - } - } - - if (is_list == -1) { - assert(!qdict_size(maybe_list)); - is_list = 0; - } - - /* NB this isn't a perfect check - e.g. it won't catch - * a list containing '1', '+1', '01', '3', but that - * does not matter - we've still proved that the - * input is a list. It is up the caller to do a - * stricter check if desired */ - if (len != (max + 1)) { - error_setg(errp, "List indices are not contiguous, " - "saw %zd elements but %zd largest index", - len, max); - return -1; - } - - return is_list; -} - -/** - * qdict_crumple: - * @src: the original flat dictionary (only scalar values) to crumple - * - * Takes a flat dictionary whose keys use '.' separator to indicate - * nesting, and values are scalars, and crumples it into a nested - * structure. - * - * To include a literal '.' in a key name, it must be escaped as '..' - * - * For example, an input of: - * - * { 'foo.0.bar': 'one', 'foo.0.wizz': '1', - * 'foo.1.bar': 'two', 'foo.1.wizz': '2' } - * - * will result in an output of: - * - * { - * 'foo': [ - * { 'bar': 'one', 'wizz': '1' }, - * { 'bar': 'two', 'wizz': '2' } - * ], - * } - * - * The following scenarios in the input dict will result in an - * error being returned: - * - * - Any values in @src are non-scalar types - * - If keys in @src imply that a particular level is both a - * list and a dict. e.g., "foo.0.bar" and "foo.eek.bar". - * - If keys in @src imply that a particular level is a list, - * but the indices are non-contiguous. e.g. "foo.0.bar" and - * "foo.2.bar" without any "foo.1.bar" present. - * - If keys in @src represent list indexes, but are not in - * the "%zu" format. e.g. "foo.+0.bar" - * - * Returns: either a QDict or QList for the nested data structure, or NULL - * on error - */ -QObject *qdict_crumple(const QDict *src, Error **errp) -{ - const QDictEntry *ent; - QDict *two_level, *multi_level = NULL; - QObject *dst = NULL, *child; - size_t i; - char *prefix = NULL; - const char *suffix = NULL; - int is_list; - - two_level = qdict_new(); - - /* Step 1: split our totally flat dict into a two level dict */ - for (ent = qdict_first(src); ent != NULL; ent = qdict_next(src, ent)) { - if (qobject_type(ent->value) == QTYPE_QDICT || - qobject_type(ent->value) == QTYPE_QLIST) { - error_setg(errp, "Value %s is not a scalar", - ent->key); - goto error; - } - - qdict_split_flat_key(ent->key, &prefix, &suffix); - - child = qdict_get(two_level, prefix); - if (suffix) { - QDict *child_dict = qobject_to(QDict, child); - if (!child_dict) { - if (child) { - error_setg(errp, "Key %s prefix is already set as a scalar", - prefix); - goto error; - } - - child_dict = qdict_new(); - qdict_put_obj(two_level, prefix, QOBJECT(child_dict)); - } - - qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); - } else { - if (child) { - error_setg(errp, "Key %s prefix is already set as a dict", - prefix); - goto error; - } - qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); - } - - g_free(prefix); - prefix = NULL; - } - - /* Step 2: optionally process the two level dict recursively - * into a multi-level dict */ - multi_level = qdict_new(); - for (ent = qdict_first(two_level); ent != NULL; - ent = qdict_next(two_level, ent)) { - QDict *dict = qobject_to(QDict, ent->value); - if (dict) { - child = qdict_crumple(dict, errp); - if (!child) { - goto error; - } - - qdict_put_obj(multi_level, ent->key, child); - } else { - qdict_put_obj(multi_level, ent->key, qobject_ref(ent->value)); - } - } - qobject_unref(two_level); - two_level = NULL; - - /* Step 3: detect if we need to turn our dict into list */ - is_list = qdict_is_list(multi_level, errp); - if (is_list < 0) { - goto error; - } - - if (is_list) { - dst = QOBJECT(qlist_new()); - - for (i = 0; i < qdict_size(multi_level); i++) { - char *key = g_strdup_printf("%zu", i); - - child = qdict_get(multi_level, key); - g_free(key); - - if (!child) { - error_setg(errp, "Missing list index %zu", i); - goto error; - } - - qlist_append_obj(qobject_to(QList, dst), qobject_ref(child)); - } - qobject_unref(multi_level); - multi_level = NULL; - } else { - dst = QOBJECT(multi_level); - } - - return dst; - - error: - g_free(prefix); - qobject_unref(multi_level); - qobject_unref(two_level); - qobject_unref(dst); - return NULL; -} - -/** - * qdict_array_entries(): Returns the number of direct array entries if the - * sub-QDict of src specified by the prefix in subqdict (or src itself for - * prefix == "") is valid as an array, i.e. the length of the created list if - * the sub-QDict would become empty after calling qdict_array_split() on it. If - * the array is not valid, -EINVAL is returned. - */ -int qdict_array_entries(QDict *src, const char *subqdict) -{ - const QDictEntry *entry; - unsigned i; - unsigned entries = 0; - size_t subqdict_len = strlen(subqdict); - - assert(!subqdict_len || subqdict[subqdict_len - 1] == '.'); - - /* qdict_array_split() loops until UINT_MAX, but as we want to return - * negative errors, we only have a signed return value here. Any additional - * entries will lead to -EINVAL. */ - for (i = 0; i < INT_MAX; i++) { - QObject *subqobj; - int subqdict_entries; - char *prefix = g_strdup_printf("%s%u.", subqdict, i); - - subqdict_entries = qdict_count_prefixed_entries(src, prefix); - - /* Remove ending "." */ - prefix[strlen(prefix) - 1] = 0; - subqobj = qdict_get(src, prefix); - - g_free(prefix); - - if (subqdict_entries < 0) { - return subqdict_entries; - } - - /* There may be either a single subordinate object (named "%u") or - * multiple objects (each with a key prefixed "%u."), but not both. */ - if (subqobj && subqdict_entries) { - return -EINVAL; - } else if (!subqobj && !subqdict_entries) { - break; - } - - entries += subqdict_entries ? subqdict_entries : 1; - } - - /* Consider everything handled that isn't part of the given sub-QDict */ - for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) { - if (!strstart(qdict_entry_key(entry), subqdict, NULL)) { - entries++; - } - } - - /* Anything left in the sub-QDict that wasn't handled? */ - if (qdict_size(src) != entries) { - return -EINVAL; - } - - return i; -} - -/** - * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all - * elements from src to dest. - * - * If an element from src has a key already present in dest, it will not be - * moved unless overwrite is true. - * - * If overwrite is true, the conflicting values in dest will be discarded and - * replaced by the corresponding values from src. - * - * Therefore, with overwrite being true, the src QDict will always be empty when - * this function returns. If overwrite is false, the src QDict will be empty - * iff there were no conflicts. - */ -void qdict_join(QDict *dest, QDict *src, bool overwrite) -{ - const QDictEntry *entry, *next; - - entry = qdict_first(src); - while (entry) { - next = qdict_next(src, entry); - - if (overwrite || !qdict_haskey(dest, entry->key)) { - qdict_put_obj(dest, entry->key, qobject_ref(entry->value)); - qdict_del(src, entry->key); - } - - entry = next; - } -} - -/** - * qdict_rename_keys(): Rename keys in qdict according to the replacements - * specified in the array renames. The array must be terminated by an entry - * with from = NULL. - * - * The renames are performed individually in the order of the array, so entries - * may be renamed multiple times and may or may not conflict depending on the - * order of the renames array. - * - * Returns true for success, false in error cases. - */ -bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp) -{ - QObject *qobj; - - while (renames->from) { - if (qdict_haskey(qdict, renames->from)) { - if (qdict_haskey(qdict, renames->to)) { - error_setg(errp, "'%s' and its alias '%s' can't be used at the " - "same time", renames->to, renames->from); - return false; - } - - qobj = qdict_get(qdict, renames->from); - qdict_put_obj(qdict, renames->to, qobject_ref(qobj)); - qdict_del(qdict, renames->from); - } - - renames++; - } - return true; -} diff --git a/tests/Makefile.include b/tests/Makefile.include index d098a104bb..2023e257fd 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -40,6 +40,8 @@ SYSEMU_TARGET_LIST := $(subst -softmmu.mak,,$(notdir \ check-unit-y = tests/check-qdict$(EXESUF) gcov-files-check-qdict-y = qobject/qdict.c +check-unit-y = tests/check-block-qdict$(EXESUF) +gcov-files-check-block-qdict-y = qobject/block-qdict.c check-unit-y += tests/test-char$(EXESUF) gcov-files-check-qdict-y = chardev/char.c check-unit-y += tests/check-qnum$(EXESUF) @@ -585,6 +587,7 @@ GENERATED_FILES += tests/test-qapi-types.h tests/test-qapi-visit.h \ test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \ tests/check-qlist.o tests/check-qnull.o tests/check-qobject.o \ tests/check-qjson.o tests/check-qlit.o \ + tests/check-block-qtest.o \ tests/test-coroutine.o tests/test-string-output-visitor.o \ tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \ tests/test-clone-visitor.o \ @@ -615,6 +618,7 @@ test-block-obj-y = $(block-obj-y) $(test-io-obj-y) tests/iothread.o tests/check-qnum$(EXESUF): tests/check-qnum.o $(test-util-obj-y) tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y) tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y) +tests/check-block-qdict$(EXESUF): tests/check-block-qdict.o $(test-util-obj-y) tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y) tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y) tests/check-qobject$(EXESUF): tests/check-qobject.o $(test-util-obj-y) diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c new file mode 100644 index 0000000000..5b9f4d506e --- /dev/null +++ b/tests/check-block-qdict.c @@ -0,0 +1,655 @@ +/* + * Unit-tests for Block layer QDict extras + * + * Copyright (c) 2013-2018 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "block/qdict.h" +#include "qapi/qmp/qlist.h" +#include "qapi/qmp/qnum.h" +#include "qapi/error.h" + +static void qdict_defaults_test(void) +{ + QDict *dict, *copy; + + dict = qdict_new(); + copy = qdict_new(); + + qdict_set_default_str(dict, "foo", "abc"); + qdict_set_default_str(dict, "foo", "def"); + g_assert_cmpstr(qdict_get_str(dict, "foo"), ==, "abc"); + qdict_set_default_str(dict, "bar", "ghi"); + + qdict_copy_default(copy, dict, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "foo"), ==, "abc"); + qdict_set_default_str(copy, "bar", "xyz"); + qdict_copy_default(copy, dict, "bar"); + g_assert_cmpstr(qdict_get_str(copy, "bar"), ==, "xyz"); + + qobject_unref(copy); + qobject_unref(dict); +} + +static void qdict_flatten_test(void) +{ + QList *list1 = qlist_new(); + QList *list2 = qlist_new(); + QDict *dict1 = qdict_new(); + QDict *dict2 = qdict_new(); + QDict *dict3 = qdict_new(); + + /* + * Test the flattening of + * + * { + * "e": [ + * 42, + * [ + * 23, + * 66, + * { + * "a": 0, + * "b": 1 + * } + * ] + * ], + * "f": { + * "c": 2, + * "d": 3, + * }, + * "g": 4 + * } + * + * to + * + * { + * "e.0": 42, + * "e.1.0": 23, + * "e.1.1": 66, + * "e.1.2.a": 0, + * "e.1.2.b": 1, + * "f.c": 2, + * "f.d": 3, + * "g": 4 + * } + */ + + qdict_put_int(dict1, "a", 0); + qdict_put_int(dict1, "b", 1); + + qlist_append_int(list1, 23); + qlist_append_int(list1, 66); + qlist_append(list1, dict1); + qlist_append_int(list2, 42); + qlist_append(list2, list1); + + qdict_put_int(dict2, "c", 2); + qdict_put_int(dict2, "d", 3); + qdict_put(dict3, "e", list2); + qdict_put(dict3, "f", dict2); + qdict_put_int(dict3, "g", 4); + + qdict_flatten(dict3); + + g_assert(qdict_get_int(dict3, "e.0") == 42); + g_assert(qdict_get_int(dict3, "e.1.0") == 23); + g_assert(qdict_get_int(dict3, "e.1.1") == 66); + g_assert(qdict_get_int(dict3, "e.1.2.a") == 0); + g_assert(qdict_get_int(dict3, "e.1.2.b") == 1); + g_assert(qdict_get_int(dict3, "f.c") == 2); + g_assert(qdict_get_int(dict3, "f.d") == 3); + g_assert(qdict_get_int(dict3, "g") == 4); + + g_assert(qdict_size(dict3) == 8); + + qobject_unref(dict3); +} + +static void qdict_array_split_test(void) +{ + QDict *test_dict = qdict_new(); + QDict *dict1, *dict2; + QNum *int1; + QList *test_list; + + /* + * Test the split of + * + * { + * "1.x": 0, + * "4.y": 1, + * "0.a": 42, + * "o.o": 7, + * "0.b": 23, + * "2": 66 + * } + * + * to + * + * [ + * { + * "a": 42, + * "b": 23 + * }, + * { + * "x": 0 + * }, + * 66 + * ] + * + * and + * + * { + * "4.y": 1, + * "o.o": 7 + * } + * + * (remaining in the old QDict) + * + * This example is given in the comment of qdict_array_split(). + */ + + qdict_put_int(test_dict, "1.x", 0); + qdict_put_int(test_dict, "4.y", 1); + qdict_put_int(test_dict, "0.a", 42); + qdict_put_int(test_dict, "o.o", 7); + qdict_put_int(test_dict, "0.b", 23); + qdict_put_int(test_dict, "2", 66); + + qdict_array_split(test_dict, &test_list); + + dict1 = qobject_to(QDict, qlist_pop(test_list)); + dict2 = qobject_to(QDict, qlist_pop(test_list)); + int1 = qobject_to(QNum, qlist_pop(test_list)); + + g_assert(dict1); + g_assert(dict2); + g_assert(int1); + g_assert(qlist_empty(test_list)); + + qobject_unref(test_list); + + g_assert(qdict_get_int(dict1, "a") == 42); + g_assert(qdict_get_int(dict1, "b") == 23); + + g_assert(qdict_size(dict1) == 2); + + qobject_unref(dict1); + + g_assert(qdict_get_int(dict2, "x") == 0); + + g_assert(qdict_size(dict2) == 1); + + qobject_unref(dict2); + + g_assert_cmpint(qnum_get_int(int1), ==, 66); + + qobject_unref(int1); + + g_assert(qdict_get_int(test_dict, "4.y") == 1); + g_assert(qdict_get_int(test_dict, "o.o") == 7); + + g_assert(qdict_size(test_dict) == 2); + + qobject_unref(test_dict); + + /* + * Test the split of + * + * { + * "0": 42, + * "1": 23, + * "1.x": 84 + * } + * + * to + * + * [ + * 42 + * ] + * + * and + * + * { + * "1": 23, + * "1.x": 84 + * } + * + * That is, test whether splitting stops if there is both an entry with key + * of "%u" and other entries with keys prefixed "%u." for the same index. + */ + + test_dict = qdict_new(); + + qdict_put_int(test_dict, "0", 42); + qdict_put_int(test_dict, "1", 23); + qdict_put_int(test_dict, "1.x", 84); + + qdict_array_split(test_dict, &test_list); + + int1 = qobject_to(QNum, qlist_pop(test_list)); + + g_assert(int1); + g_assert(qlist_empty(test_list)); + + qobject_unref(test_list); + + g_assert_cmpint(qnum_get_int(int1), ==, 42); + + qobject_unref(int1); + + g_assert(qdict_get_int(test_dict, "1") == 23); + g_assert(qdict_get_int(test_dict, "1.x") == 84); + + g_assert(qdict_size(test_dict) == 2); + + qobject_unref(test_dict); +} + +static void qdict_array_entries_test(void) +{ + QDict *dict = qdict_new(); + + g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 0); + + qdict_put_int(dict, "bar", 0); + qdict_put_int(dict, "baz.0", 0); + g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 0); + + qdict_put_int(dict, "foo.1", 0); + g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, -EINVAL); + qdict_put_int(dict, "foo.0", 0); + g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 2); + qdict_put_int(dict, "foo.bar", 0); + g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, -EINVAL); + qdict_del(dict, "foo.bar"); + + qdict_put_int(dict, "foo.2.a", 0); + qdict_put_int(dict, "foo.2.b", 0); + qdict_put_int(dict, "foo.2.c", 0); + g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 3); + g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL); + + qobject_unref(dict); + + dict = qdict_new(); + qdict_put_int(dict, "1", 0); + g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL); + qdict_put_int(dict, "0", 0); + g_assert_cmpint(qdict_array_entries(dict, ""), ==, 2); + qdict_put_int(dict, "bar", 0); + g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL); + qdict_del(dict, "bar"); + + qdict_put_int(dict, "2.a", 0); + qdict_put_int(dict, "2.b", 0); + qdict_put_int(dict, "2.c", 0); + g_assert_cmpint(qdict_array_entries(dict, ""), ==, 3); + + qobject_unref(dict); +} + +static void qdict_join_test(void) +{ + QDict *dict1, *dict2; + bool overwrite = false; + int i; + + dict1 = qdict_new(); + dict2 = qdict_new(); + + /* Test everything once without overwrite and once with */ + do { + /* Test empty dicts */ + qdict_join(dict1, dict2, overwrite); + + g_assert(qdict_size(dict1) == 0); + g_assert(qdict_size(dict2) == 0); + + /* First iteration: Test movement */ + /* Second iteration: Test empty source and non-empty destination */ + qdict_put_int(dict2, "foo", 42); + + for (i = 0; i < 2; i++) { + qdict_join(dict1, dict2, overwrite); + + g_assert(qdict_size(dict1) == 1); + g_assert(qdict_size(dict2) == 0); + + g_assert(qdict_get_int(dict1, "foo") == 42); + } + + /* Test non-empty source and destination without conflict */ + qdict_put_int(dict2, "bar", 23); + + qdict_join(dict1, dict2, overwrite); + + g_assert(qdict_size(dict1) == 2); + g_assert(qdict_size(dict2) == 0); + + g_assert(qdict_get_int(dict1, "foo") == 42); + g_assert(qdict_get_int(dict1, "bar") == 23); + + /* Test conflict */ + qdict_put_int(dict2, "foo", 84); + + qdict_join(dict1, dict2, overwrite); + + g_assert(qdict_size(dict1) == 2); + g_assert(qdict_size(dict2) == !overwrite); + + g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42)); + g_assert(qdict_get_int(dict1, "bar") == 23); + + if (!overwrite) { + g_assert(qdict_get_int(dict2, "foo") == 84); + } + + /* Check the references */ + g_assert(qdict_get(dict1, "foo")->base.refcnt == 1); + g_assert(qdict_get(dict1, "bar")->base.refcnt == 1); + + if (!overwrite) { + g_assert(qdict_get(dict2, "foo")->base.refcnt == 1); + } + + /* Clean up */ + qdict_del(dict1, "foo"); + qdict_del(dict1, "bar"); + + if (!overwrite) { + qdict_del(dict2, "foo"); + } + } while (overwrite ^= true); + + qobject_unref(dict1); + qobject_unref(dict2); +} + +static void qdict_crumple_test_recursive(void) +{ + QDict *src, *dst, *rule, *vnc, *acl, *listen; + QList *rules; + + src = qdict_new(); + qdict_put_str(src, "vnc.listen.addr", "127.0.0.1"); + qdict_put_str(src, "vnc.listen.port", "5901"); + qdict_put_str(src, "vnc.acl.rules.0.match", "fred"); + qdict_put_str(src, "vnc.acl.rules.0.policy", "allow"); + qdict_put_str(src, "vnc.acl.rules.1.match", "bob"); + qdict_put_str(src, "vnc.acl.rules.1.policy", "deny"); + qdict_put_str(src, "vnc.acl.default", "deny"); + qdict_put_str(src, "vnc.acl..name", "acl0"); + qdict_put_str(src, "vnc.acl.rule..name", "acl0"); + + dst = qobject_to(QDict, qdict_crumple(src, &error_abort)); + g_assert(dst); + g_assert_cmpint(qdict_size(dst), ==, 1); + + vnc = qdict_get_qdict(dst, "vnc"); + g_assert(vnc); + g_assert_cmpint(qdict_size(vnc), ==, 3); + + listen = qdict_get_qdict(vnc, "listen"); + g_assert(listen); + g_assert_cmpint(qdict_size(listen), ==, 2); + g_assert_cmpstr("127.0.0.1", ==, qdict_get_str(listen, "addr")); + g_assert_cmpstr("5901", ==, qdict_get_str(listen, "port")); + + acl = qdict_get_qdict(vnc, "acl"); + g_assert(acl); + g_assert_cmpint(qdict_size(acl), ==, 3); + + rules = qdict_get_qlist(acl, "rules"); + g_assert(rules); + g_assert_cmpint(qlist_size(rules), ==, 2); + + rule = qobject_to(QDict, qlist_pop(rules)); + g_assert(rule); + g_assert_cmpint(qdict_size(rule), ==, 2); + g_assert_cmpstr("fred", ==, qdict_get_str(rule, "match")); + g_assert_cmpstr("allow", ==, qdict_get_str(rule, "policy")); + qobject_unref(rule); + + rule = qobject_to(QDict, qlist_pop(rules)); + g_assert(rule); + g_assert_cmpint(qdict_size(rule), ==, 2); + g_assert_cmpstr("bob", ==, qdict_get_str(rule, "match")); + g_assert_cmpstr("deny", ==, qdict_get_str(rule, "policy")); + qobject_unref(rule); + + /* With recursive crumpling, we should see all names unescaped */ + g_assert_cmpstr("acl0", ==, qdict_get_str(vnc, "acl.name")); + g_assert_cmpstr("acl0", ==, qdict_get_str(acl, "rule.name")); + + qobject_unref(src); + qobject_unref(dst); +} + +static void qdict_crumple_test_empty(void) +{ + QDict *src, *dst; + + src = qdict_new(); + + dst = qobject_to(QDict, qdict_crumple(src, &error_abort)); + + g_assert_cmpint(qdict_size(dst), ==, 0); + + qobject_unref(src); + qobject_unref(dst); +} + +static int qdict_count_entries(QDict *dict) +{ + const QDictEntry *e; + int count = 0; + + for (e = qdict_first(dict); e; e = qdict_next(dict, e)) { + count++; + } + + return count; +} + +static void qdict_rename_keys_test(void) +{ + QDict *dict = qdict_new(); + QDict *copy; + QDictRenames *renames; + Error *local_err = NULL; + + qdict_put_str(dict, "abc", "foo"); + qdict_put_str(dict, "abcdef", "bar"); + qdict_put_int(dict, "number", 42); + qdict_put_bool(dict, "flag", true); + qdict_put_null(dict, "nothing"); + + /* Empty rename list */ + renames = (QDictRenames[]) { + { NULL, "this can be anything" } + }; + copy = qdict_clone_shallow(dict); + qdict_rename_keys(copy, renames, &error_abort); + + g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar"); + g_assert_cmpint(qdict_get_int(copy, "number"), ==, 42); + g_assert_cmpint(qdict_get_bool(copy, "flag"), ==, true); + g_assert(qobject_type(qdict_get(copy, "nothing")) == QTYPE_QNULL); + g_assert_cmpint(qdict_count_entries(copy), ==, 5); + + qobject_unref(copy); + + /* Simple rename of all entries */ + renames = (QDictRenames[]) { + { "abc", "str1" }, + { "abcdef", "str2" }, + { "number", "int" }, + { "flag", "bool" }, + { "nothing", "null" }, + { NULL , NULL } + }; + copy = qdict_clone_shallow(dict); + qdict_rename_keys(copy, renames, &error_abort); + + g_assert(!qdict_haskey(copy, "abc")); + g_assert(!qdict_haskey(copy, "abcdef")); + g_assert(!qdict_haskey(copy, "number")); + g_assert(!qdict_haskey(copy, "flag")); + g_assert(!qdict_haskey(copy, "nothing")); + + g_assert_cmpstr(qdict_get_str(copy, "str1"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "str2"), ==, "bar"); + g_assert_cmpint(qdict_get_int(copy, "int"), ==, 42); + g_assert_cmpint(qdict_get_bool(copy, "bool"), ==, true); + g_assert(qobject_type(qdict_get(copy, "null")) == QTYPE_QNULL); + g_assert_cmpint(qdict_count_entries(copy), ==, 5); + + qobject_unref(copy); + + /* Renames are processed top to bottom */ + renames = (QDictRenames[]) { + { "abc", "tmp" }, + { "abcdef", "abc" }, + { "number", "abcdef" }, + { "flag", "number" }, + { "nothing", "flag" }, + { "tmp", "nothing" }, + { NULL , NULL } + }; + copy = qdict_clone_shallow(dict); + qdict_rename_keys(copy, renames, &error_abort); + + g_assert_cmpstr(qdict_get_str(copy, "nothing"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "bar"); + g_assert_cmpint(qdict_get_int(copy, "abcdef"), ==, 42); + g_assert_cmpint(qdict_get_bool(copy, "number"), ==, true); + g_assert(qobject_type(qdict_get(copy, "flag")) == QTYPE_QNULL); + g_assert(!qdict_haskey(copy, "tmp")); + g_assert_cmpint(qdict_count_entries(copy), ==, 5); + + qobject_unref(copy); + + /* Conflicting rename */ + renames = (QDictRenames[]) { + { "abcdef", "abc" }, + { NULL , NULL } + }; + copy = qdict_clone_shallow(dict); + qdict_rename_keys(copy, renames, &local_err); + + g_assert(local_err != NULL); + error_free(local_err); + local_err = NULL; + + g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar"); + g_assert_cmpint(qdict_get_int(copy, "number"), ==, 42); + g_assert_cmpint(qdict_get_bool(copy, "flag"), ==, true); + g_assert(qobject_type(qdict_get(copy, "nothing")) == QTYPE_QNULL); + g_assert_cmpint(qdict_count_entries(copy), ==, 5); + + qobject_unref(copy); + + /* Renames in an empty dict */ + renames = (QDictRenames[]) { + { "abcdef", "abc" }, + { NULL , NULL } + }; + + qobject_unref(dict); + dict = qdict_new(); + + qdict_rename_keys(dict, renames, &error_abort); + g_assert(qdict_first(dict) == NULL); + + qobject_unref(dict); +} + +static void qdict_crumple_test_bad_inputs(void) +{ + QDict *src; + Error *error = NULL; + + src = qdict_new(); + /* rule.0 can't be both a string and a dict */ + qdict_put_str(src, "rule.0", "fred"); + qdict_put_str(src, "rule.0.policy", "allow"); + + g_assert(qdict_crumple(src, &error) == NULL); + g_assert(error != NULL); + error_free(error); + error = NULL; + qobject_unref(src); + + src = qdict_new(); + /* rule can't be both a list and a dict */ + qdict_put_str(src, "rule.0", "fred"); + qdict_put_str(src, "rule.a", "allow"); + + g_assert(qdict_crumple(src, &error) == NULL); + g_assert(error != NULL); + error_free(error); + error = NULL; + qobject_unref(src); + + src = qdict_new(); + /* The input should be flat, ie no dicts or lists */ + qdict_put(src, "rule.a", qdict_new()); + qdict_put_str(src, "rule.b", "allow"); + + g_assert(qdict_crumple(src, &error) == NULL); + g_assert(error != NULL); + error_free(error); + error = NULL; + qobject_unref(src); + + src = qdict_new(); + /* List indexes must not have gaps */ + qdict_put_str(src, "rule.0", "deny"); + qdict_put_str(src, "rule.3", "allow"); + + g_assert(qdict_crumple(src, &error) == NULL); + g_assert(error != NULL); + error_free(error); + error = NULL; + qobject_unref(src); + + src = qdict_new(); + /* List indexes must be in %zu format */ + qdict_put_str(src, "rule.0", "deny"); + qdict_put_str(src, "rule.+1", "allow"); + + g_assert(qdict_crumple(src, &error) == NULL); + g_assert(error != NULL); + error_free(error); + error = NULL; + qobject_unref(src); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + g_test_add_func("/public/defaults", qdict_defaults_test); + g_test_add_func("/public/flatten", qdict_flatten_test); + g_test_add_func("/public/array_split", qdict_array_split_test); + g_test_add_func("/public/array_entries", qdict_array_entries_test); + g_test_add_func("/public/join", qdict_join_test); + g_test_add_func("/public/crumple/recursive", + qdict_crumple_test_recursive); + g_test_add_func("/public/crumple/empty", + qdict_crumple_test_empty); + g_test_add_func("/public/crumple/bad_inputs", + qdict_crumple_test_bad_inputs); + + g_test_add_func("/public/rename_keys", qdict_rename_keys_test); + + return g_test_run(); +} diff --git a/tests/check-qdict.c b/tests/check-qdict.c index 93e2112b6d..86e9fe7dc4 100644 --- a/tests/check-qdict.c +++ b/tests/check-qdict.c @@ -11,13 +11,7 @@ */ #include "qemu/osdep.h" -#include "block/qdict.h" #include "qapi/qmp/qdict.h" -#include "qapi/qmp/qlist.h" -#include "qapi/qmp/qnum.h" -#include "qapi/qmp/qstring.h" -#include "qapi/error.h" -#include "qemu-common.h" /* * Public Interface test-cases @@ -157,28 +151,6 @@ static void qdict_get_try_str_test(void) qobject_unref(tests_dict); } -static void qdict_defaults_test(void) -{ - QDict *dict, *copy; - - dict = qdict_new(); - copy = qdict_new(); - - qdict_set_default_str(dict, "foo", "abc"); - qdict_set_default_str(dict, "foo", "def"); - g_assert_cmpstr(qdict_get_str(dict, "foo"), ==, "abc"); - qdict_set_default_str(dict, "bar", "ghi"); - - qdict_copy_default(copy, dict, "foo"); - g_assert_cmpstr(qdict_get_str(copy, "foo"), ==, "abc"); - qdict_set_default_str(copy, "bar", "xyz"); - qdict_copy_default(copy, dict, "bar"); - g_assert_cmpstr(qdict_get_str(copy, "bar"), ==, "xyz"); - - qobject_unref(copy); - qobject_unref(dict); -} - static void qdict_haskey_not_test(void) { QDict *tests_dict = qdict_new(); @@ -254,606 +226,6 @@ static void qdict_iterapi_test(void) qobject_unref(tests_dict); } -static void qdict_flatten_test(void) -{ - QList *list1 = qlist_new(); - QList *list2 = qlist_new(); - QDict *dict1 = qdict_new(); - QDict *dict2 = qdict_new(); - QDict *dict3 = qdict_new(); - - /* - * Test the flattening of - * - * { - * "e": [ - * 42, - * [ - * 23, - * 66, - * { - * "a": 0, - * "b": 1 - * } - * ] - * ], - * "f": { - * "c": 2, - * "d": 3, - * }, - * "g": 4 - * } - * - * to - * - * { - * "e.0": 42, - * "e.1.0": 23, - * "e.1.1": 66, - * "e.1.2.a": 0, - * "e.1.2.b": 1, - * "f.c": 2, - * "f.d": 3, - * "g": 4 - * } - */ - - qdict_put_int(dict1, "a", 0); - qdict_put_int(dict1, "b", 1); - - qlist_append_int(list1, 23); - qlist_append_int(list1, 66); - qlist_append(list1, dict1); - qlist_append_int(list2, 42); - qlist_append(list2, list1); - - qdict_put_int(dict2, "c", 2); - qdict_put_int(dict2, "d", 3); - qdict_put(dict3, "e", list2); - qdict_put(dict3, "f", dict2); - qdict_put_int(dict3, "g", 4); - - qdict_flatten(dict3); - - g_assert(qdict_get_int(dict3, "e.0") == 42); - g_assert(qdict_get_int(dict3, "e.1.0") == 23); - g_assert(qdict_get_int(dict3, "e.1.1") == 66); - g_assert(qdict_get_int(dict3, "e.1.2.a") == 0); - g_assert(qdict_get_int(dict3, "e.1.2.b") == 1); - g_assert(qdict_get_int(dict3, "f.c") == 2); - g_assert(qdict_get_int(dict3, "f.d") == 3); - g_assert(qdict_get_int(dict3, "g") == 4); - - g_assert(qdict_size(dict3) == 8); - - qobject_unref(dict3); -} - -static void qdict_array_split_test(void) -{ - QDict *test_dict = qdict_new(); - QDict *dict1, *dict2; - QNum *int1; - QList *test_list; - - /* - * Test the split of - * - * { - * "1.x": 0, - * "4.y": 1, - * "0.a": 42, - * "o.o": 7, - * "0.b": 23, - * "2": 66 - * } - * - * to - * - * [ - * { - * "a": 42, - * "b": 23 - * }, - * { - * "x": 0 - * }, - * 66 - * ] - * - * and - * - * { - * "4.y": 1, - * "o.o": 7 - * } - * - * (remaining in the old QDict) - * - * This example is given in the comment of qdict_array_split(). - */ - - qdict_put_int(test_dict, "1.x", 0); - qdict_put_int(test_dict, "4.y", 1); - qdict_put_int(test_dict, "0.a", 42); - qdict_put_int(test_dict, "o.o", 7); - qdict_put_int(test_dict, "0.b", 23); - qdict_put_int(test_dict, "2", 66); - - qdict_array_split(test_dict, &test_list); - - dict1 = qobject_to(QDict, qlist_pop(test_list)); - dict2 = qobject_to(QDict, qlist_pop(test_list)); - int1 = qobject_to(QNum, qlist_pop(test_list)); - - g_assert(dict1); - g_assert(dict2); - g_assert(int1); - g_assert(qlist_empty(test_list)); - - qobject_unref(test_list); - - g_assert(qdict_get_int(dict1, "a") == 42); - g_assert(qdict_get_int(dict1, "b") == 23); - - g_assert(qdict_size(dict1) == 2); - - qobject_unref(dict1); - - g_assert(qdict_get_int(dict2, "x") == 0); - - g_assert(qdict_size(dict2) == 1); - - qobject_unref(dict2); - - g_assert_cmpint(qnum_get_int(int1), ==, 66); - - qobject_unref(int1); - - g_assert(qdict_get_int(test_dict, "4.y") == 1); - g_assert(qdict_get_int(test_dict, "o.o") == 7); - - g_assert(qdict_size(test_dict) == 2); - - qobject_unref(test_dict); - - /* - * Test the split of - * - * { - * "0": 42, - * "1": 23, - * "1.x": 84 - * } - * - * to - * - * [ - * 42 - * ] - * - * and - * - * { - * "1": 23, - * "1.x": 84 - * } - * - * That is, test whether splitting stops if there is both an entry with key - * of "%u" and other entries with keys prefixed "%u." for the same index. - */ - - test_dict = qdict_new(); - - qdict_put_int(test_dict, "0", 42); - qdict_put_int(test_dict, "1", 23); - qdict_put_int(test_dict, "1.x", 84); - - qdict_array_split(test_dict, &test_list); - - int1 = qobject_to(QNum, qlist_pop(test_list)); - - g_assert(int1); - g_assert(qlist_empty(test_list)); - - qobject_unref(test_list); - - g_assert_cmpint(qnum_get_int(int1), ==, 42); - - qobject_unref(int1); - - g_assert(qdict_get_int(test_dict, "1") == 23); - g_assert(qdict_get_int(test_dict, "1.x") == 84); - - g_assert(qdict_size(test_dict) == 2); - - qobject_unref(test_dict); -} - -static void qdict_array_entries_test(void) -{ - QDict *dict = qdict_new(); - - g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 0); - - qdict_put_int(dict, "bar", 0); - qdict_put_int(dict, "baz.0", 0); - g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 0); - - qdict_put_int(dict, "foo.1", 0); - g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, -EINVAL); - qdict_put_int(dict, "foo.0", 0); - g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 2); - qdict_put_int(dict, "foo.bar", 0); - g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, -EINVAL); - qdict_del(dict, "foo.bar"); - - qdict_put_int(dict, "foo.2.a", 0); - qdict_put_int(dict, "foo.2.b", 0); - qdict_put_int(dict, "foo.2.c", 0); - g_assert_cmpint(qdict_array_entries(dict, "foo."), ==, 3); - g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL); - - qobject_unref(dict); - - dict = qdict_new(); - qdict_put_int(dict, "1", 0); - g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL); - qdict_put_int(dict, "0", 0); - g_assert_cmpint(qdict_array_entries(dict, ""), ==, 2); - qdict_put_int(dict, "bar", 0); - g_assert_cmpint(qdict_array_entries(dict, ""), ==, -EINVAL); - qdict_del(dict, "bar"); - - qdict_put_int(dict, "2.a", 0); - qdict_put_int(dict, "2.b", 0); - qdict_put_int(dict, "2.c", 0); - g_assert_cmpint(qdict_array_entries(dict, ""), ==, 3); - - qobject_unref(dict); -} - -static void qdict_join_test(void) -{ - QDict *dict1, *dict2; - bool overwrite = false; - int i; - - dict1 = qdict_new(); - dict2 = qdict_new(); - - /* Test everything once without overwrite and once with */ - do - { - /* Test empty dicts */ - qdict_join(dict1, dict2, overwrite); - - g_assert(qdict_size(dict1) == 0); - g_assert(qdict_size(dict2) == 0); - - /* First iteration: Test movement */ - /* Second iteration: Test empty source and non-empty destination */ - qdict_put_int(dict2, "foo", 42); - - for (i = 0; i < 2; i++) { - qdict_join(dict1, dict2, overwrite); - - g_assert(qdict_size(dict1) == 1); - g_assert(qdict_size(dict2) == 0); - - g_assert(qdict_get_int(dict1, "foo") == 42); - } - - /* Test non-empty source and destination without conflict */ - qdict_put_int(dict2, "bar", 23); - - qdict_join(dict1, dict2, overwrite); - - g_assert(qdict_size(dict1) == 2); - g_assert(qdict_size(dict2) == 0); - - g_assert(qdict_get_int(dict1, "foo") == 42); - g_assert(qdict_get_int(dict1, "bar") == 23); - - /* Test conflict */ - qdict_put_int(dict2, "foo", 84); - - qdict_join(dict1, dict2, overwrite); - - g_assert(qdict_size(dict1) == 2); - g_assert(qdict_size(dict2) == !overwrite); - - g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42)); - g_assert(qdict_get_int(dict1, "bar") == 23); - - if (!overwrite) { - g_assert(qdict_get_int(dict2, "foo") == 84); - } - - /* Check the references */ - g_assert(qdict_get(dict1, "foo")->base.refcnt == 1); - g_assert(qdict_get(dict1, "bar")->base.refcnt == 1); - - if (!overwrite) { - g_assert(qdict_get(dict2, "foo")->base.refcnt == 1); - } - - /* Clean up */ - qdict_del(dict1, "foo"); - qdict_del(dict1, "bar"); - - if (!overwrite) { - qdict_del(dict2, "foo"); - } - } - while (overwrite ^= true); - - qobject_unref(dict1); - qobject_unref(dict2); -} - -static void qdict_crumple_test_recursive(void) -{ - QDict *src, *dst, *rule, *vnc, *acl, *listen; - QList *rules; - - src = qdict_new(); - qdict_put_str(src, "vnc.listen.addr", "127.0.0.1"); - qdict_put_str(src, "vnc.listen.port", "5901"); - qdict_put_str(src, "vnc.acl.rules.0.match", "fred"); - qdict_put_str(src, "vnc.acl.rules.0.policy", "allow"); - qdict_put_str(src, "vnc.acl.rules.1.match", "bob"); - qdict_put_str(src, "vnc.acl.rules.1.policy", "deny"); - qdict_put_str(src, "vnc.acl.default", "deny"); - qdict_put_str(src, "vnc.acl..name", "acl0"); - qdict_put_str(src, "vnc.acl.rule..name", "acl0"); - - dst = qobject_to(QDict, qdict_crumple(src, &error_abort)); - g_assert(dst); - g_assert_cmpint(qdict_size(dst), ==, 1); - - vnc = qdict_get_qdict(dst, "vnc"); - g_assert(vnc); - g_assert_cmpint(qdict_size(vnc), ==, 3); - - listen = qdict_get_qdict(vnc, "listen"); - g_assert(listen); - g_assert_cmpint(qdict_size(listen), ==, 2); - g_assert_cmpstr("127.0.0.1", ==, qdict_get_str(listen, "addr")); - g_assert_cmpstr("5901", ==, qdict_get_str(listen, "port")); - - acl = qdict_get_qdict(vnc, "acl"); - g_assert(acl); - g_assert_cmpint(qdict_size(acl), ==, 3); - - rules = qdict_get_qlist(acl, "rules"); - g_assert(rules); - g_assert_cmpint(qlist_size(rules), ==, 2); - - rule = qobject_to(QDict, qlist_pop(rules)); - g_assert(rule); - g_assert_cmpint(qdict_size(rule), ==, 2); - g_assert_cmpstr("fred", ==, qdict_get_str(rule, "match")); - g_assert_cmpstr("allow", ==, qdict_get_str(rule, "policy")); - qobject_unref(rule); - - rule = qobject_to(QDict, qlist_pop(rules)); - g_assert(rule); - g_assert_cmpint(qdict_size(rule), ==, 2); - g_assert_cmpstr("bob", ==, qdict_get_str(rule, "match")); - g_assert_cmpstr("deny", ==, qdict_get_str(rule, "policy")); - qobject_unref(rule); - - /* With recursive crumpling, we should see all names unescaped */ - g_assert_cmpstr("acl0", ==, qdict_get_str(vnc, "acl.name")); - g_assert_cmpstr("acl0", ==, qdict_get_str(acl, "rule.name")); - - qobject_unref(src); - qobject_unref(dst); -} - -static void qdict_crumple_test_empty(void) -{ - QDict *src, *dst; - - src = qdict_new(); - - dst = qobject_to(QDict, qdict_crumple(src, &error_abort)); - - g_assert_cmpint(qdict_size(dst), ==, 0); - - qobject_unref(src); - qobject_unref(dst); -} - -static int qdict_count_entries(QDict *dict) -{ - const QDictEntry *e; - int count = 0; - - for (e = qdict_first(dict); e; e = qdict_next(dict, e)) { - count++; - } - - return count; -} - -static void qdict_rename_keys_test(void) -{ - QDict *dict = qdict_new(); - QDict *copy; - QDictRenames *renames; - Error *local_err = NULL; - - qdict_put_str(dict, "abc", "foo"); - qdict_put_str(dict, "abcdef", "bar"); - qdict_put_int(dict, "number", 42); - qdict_put_bool(dict, "flag", true); - qdict_put_null(dict, "nothing"); - - /* Empty rename list */ - renames = (QDictRenames[]) { - { NULL, "this can be anything" } - }; - copy = qdict_clone_shallow(dict); - qdict_rename_keys(copy, renames, &error_abort); - - g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo"); - g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar"); - g_assert_cmpint(qdict_get_int(copy, "number"), ==, 42); - g_assert_cmpint(qdict_get_bool(copy, "flag"), ==, true); - g_assert(qobject_type(qdict_get(copy, "nothing")) == QTYPE_QNULL); - g_assert_cmpint(qdict_count_entries(copy), ==, 5); - - qobject_unref(copy); - - /* Simple rename of all entries */ - renames = (QDictRenames[]) { - { "abc", "str1" }, - { "abcdef", "str2" }, - { "number", "int" }, - { "flag", "bool" }, - { "nothing", "null" }, - { NULL , NULL } - }; - copy = qdict_clone_shallow(dict); - qdict_rename_keys(copy, renames, &error_abort); - - g_assert(!qdict_haskey(copy, "abc")); - g_assert(!qdict_haskey(copy, "abcdef")); - g_assert(!qdict_haskey(copy, "number")); - g_assert(!qdict_haskey(copy, "flag")); - g_assert(!qdict_haskey(copy, "nothing")); - - g_assert_cmpstr(qdict_get_str(copy, "str1"), ==, "foo"); - g_assert_cmpstr(qdict_get_str(copy, "str2"), ==, "bar"); - g_assert_cmpint(qdict_get_int(copy, "int"), ==, 42); - g_assert_cmpint(qdict_get_bool(copy, "bool"), ==, true); - g_assert(qobject_type(qdict_get(copy, "null")) == QTYPE_QNULL); - g_assert_cmpint(qdict_count_entries(copy), ==, 5); - - qobject_unref(copy); - - /* Renames are processed top to bottom */ - renames = (QDictRenames[]) { - { "abc", "tmp" }, - { "abcdef", "abc" }, - { "number", "abcdef" }, - { "flag", "number" }, - { "nothing", "flag" }, - { "tmp", "nothing" }, - { NULL , NULL } - }; - copy = qdict_clone_shallow(dict); - qdict_rename_keys(copy, renames, &error_abort); - - g_assert_cmpstr(qdict_get_str(copy, "nothing"), ==, "foo"); - g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "bar"); - g_assert_cmpint(qdict_get_int(copy, "abcdef"), ==, 42); - g_assert_cmpint(qdict_get_bool(copy, "number"), ==, true); - g_assert(qobject_type(qdict_get(copy, "flag")) == QTYPE_QNULL); - g_assert(!qdict_haskey(copy, "tmp")); - g_assert_cmpint(qdict_count_entries(copy), ==, 5); - - qobject_unref(copy); - - /* Conflicting rename */ - renames = (QDictRenames[]) { - { "abcdef", "abc" }, - { NULL , NULL } - }; - copy = qdict_clone_shallow(dict); - qdict_rename_keys(copy, renames, &local_err); - - g_assert(local_err != NULL); - error_free(local_err); - local_err = NULL; - - g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo"); - g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar"); - g_assert_cmpint(qdict_get_int(copy, "number"), ==, 42); - g_assert_cmpint(qdict_get_bool(copy, "flag"), ==, true); - g_assert(qobject_type(qdict_get(copy, "nothing")) == QTYPE_QNULL); - g_assert_cmpint(qdict_count_entries(copy), ==, 5); - - qobject_unref(copy); - - /* Renames in an empty dict */ - renames = (QDictRenames[]) { - { "abcdef", "abc" }, - { NULL , NULL } - }; - - qobject_unref(dict); - dict = qdict_new(); - - qdict_rename_keys(dict, renames, &error_abort); - g_assert(qdict_first(dict) == NULL); - - qobject_unref(dict); -} - -static void qdict_crumple_test_bad_inputs(void) -{ - QDict *src; - Error *error = NULL; - - src = qdict_new(); - /* rule.0 can't be both a string and a dict */ - qdict_put_str(src, "rule.0", "fred"); - qdict_put_str(src, "rule.0.policy", "allow"); - - g_assert(qdict_crumple(src, &error) == NULL); - g_assert(error != NULL); - error_free(error); - error = NULL; - qobject_unref(src); - - src = qdict_new(); - /* rule can't be both a list and a dict */ - qdict_put_str(src, "rule.0", "fred"); - qdict_put_str(src, "rule.a", "allow"); - - g_assert(qdict_crumple(src, &error) == NULL); - g_assert(error != NULL); - error_free(error); - error = NULL; - qobject_unref(src); - - src = qdict_new(); - /* The input should be flat, ie no dicts or lists */ - qdict_put(src, "rule.a", qdict_new()); - qdict_put_str(src, "rule.b", "allow"); - - g_assert(qdict_crumple(src, &error) == NULL); - g_assert(error != NULL); - error_free(error); - error = NULL; - qobject_unref(src); - - src = qdict_new(); - /* List indexes must not have gaps */ - qdict_put_str(src, "rule.0", "deny"); - qdict_put_str(src, "rule.3", "allow"); - - g_assert(qdict_crumple(src, &error) == NULL); - g_assert(error != NULL); - error_free(error); - error = NULL; - qobject_unref(src); - - src = qdict_new(); - /* List indexes must be in %zu format */ - qdict_put_str(src, "rule.0", "deny"); - qdict_put_str(src, "rule.+1", "allow"); - - g_assert(qdict_crumple(src, &error) == NULL); - g_assert(error != NULL); - error_free(error); - error = NULL; - qobject_unref(src); -} - /* * Errors test-cases */ @@ -987,29 +359,15 @@ int main(int argc, char **argv) g_test_add_func("/public/get_try_int", qdict_get_try_int_test); g_test_add_func("/public/get_str", qdict_get_str_test); g_test_add_func("/public/get_try_str", qdict_get_try_str_test); - g_test_add_func("/public/defaults", qdict_defaults_test); g_test_add_func("/public/haskey_not", qdict_haskey_not_test); g_test_add_func("/public/haskey", qdict_haskey_test); g_test_add_func("/public/del", qdict_del_test); g_test_add_func("/public/to_qdict", qobject_to_qdict_test); g_test_add_func("/public/iterapi", qdict_iterapi_test); - g_test_add_func("/public/flatten", qdict_flatten_test); - g_test_add_func("/public/array_split", qdict_array_split_test); - g_test_add_func("/public/array_entries", qdict_array_entries_test); - g_test_add_func("/public/join", qdict_join_test); g_test_add_func("/errors/put_exists", qdict_put_exists_test); g_test_add_func("/errors/get_not_exists", qdict_get_not_exists_test); - g_test_add_func("/public/crumple/recursive", - qdict_crumple_test_recursive); - g_test_add_func("/public/crumple/empty", - qdict_crumple_test_empty); - g_test_add_func("/public/crumple/bad_inputs", - qdict_crumple_test_bad_inputs); - - g_test_add_func("/public/rename_keys", qdict_rename_keys_test); - /* The Big one */ if (g_test_slow()) { g_test_add_func("/stress/test", qdict_stress_test); -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 05/18] block: Fix -blockdev for certain non-string scalars 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (3 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 04/18] qobject: Move block-specific qdict code to block-qdict.c Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 06/18] block: Fix -drive " Markus Armbruster ` (14 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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 d2d955f984..0a27afa2ef 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3152,7 +3152,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt qdict_put_str(qdict, "file", bs->node_name); /* Now get the QAPI type BlockdevCreateOptions */ - qobj = qdict_crumple(qdict, errp); + qobj = qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict = qobject_to(QDict, qobj); if (qdict == NULL) { diff --git a/block/qed.c b/block/qed.c index 324a953cbc..88fa36d409 100644 --- a/block/qed.c +++ b/block/qed.c @@ -763,7 +763,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename, qdict_put_str(qdict, "driver", "qed"); qdict_put_str(qdict, "file", bs->node_name); - qobj = qdict_crumple(qdict, errp); + qobj = qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict = qobject_to(QDict, qobj); if (qdict == NULL) { diff --git a/block/rbd.c b/block/rbd.c index 9659c7361f..09720e97c0 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -647,7 +647,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, } /* Convert the remaining options into a QAPI object */ - crumpled = qdict_crumple(options, errp); + crumpled = qdict_crumple_for_keyval_qiv(options, errp); if (crumpled == NULL) { r = -EINVAL; goto out; diff --git a/block/sheepdog.c b/block/sheepdog.c index 2e1f0e6eca..a93f93d360 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -2217,7 +2217,7 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, } /* Get the QAPI object */ - crumpled = qdict_crumple(qdict, errp); + crumpled = qdict_crumple_for_keyval_qiv(qdict, errp); if (crumpled == NULL) { ret = -EINVAL; goto fail; diff --git a/block/vhdx.c b/block/vhdx.c index 2e32e24514..78b29d9fc7 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -2005,7 +2005,7 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, qdict_put_str(qdict, "driver", "vhdx"); qdict_put_str(qdict, "file", bs->node_name); - qobj = qdict_crumple(qdict, errp); + qobj = qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict = qobject_to(QDict, qobj); if (qdict == NULL) { diff --git a/block/vpc.c b/block/vpc.c index 41c8c980f1..16178e5a90 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -1119,7 +1119,7 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, qdict_put_str(qdict, "driver", "vpc"); qdict_put_str(qdict, "file", bs->node_name); - qobj = qdict_crumple(qdict, errp); + qobj = qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict = qobject_to(QDict, qobj); if (qdict == NULL) { diff --git a/include/block/qdict.h b/include/block/qdict.h index 71c037afba..47d9638c37 100644 --- a/include/block/qdict.h +++ b/include/block/qdict.h @@ -21,6 +21,7 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start); void qdict_array_split(QDict *src, QList **dst); int qdict_array_entries(QDict *src, const char *subqdict); QObject *qdict_crumple(const QDict *src, Error **errp); +QObject *qdict_crumple_for_keyval_qiv(QDict *qdict, Error **errp); void qdict_flatten(QDict *qdict); typedef struct QDictRenames { diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c index fb92010dc5..aba372c2eb 100644 --- a/qobject/block-qdict.c +++ b/qobject/block-qdict.c @@ -9,7 +9,10 @@ #include "qemu/osdep.h" #include "block/qdict.h" +#include "qapi/qmp/qbool.h" #include "qapi/qmp/qlist.h" +#include "qapi/qmp/qnum.h" +#include "qapi/qmp/qstring.h" #include "qemu/cutils.h" #include "qapi/error.h" @@ -513,6 +516,60 @@ QObject *qdict_crumple(const QDict *src, Error **errp) return NULL; } +/** + * qdict_crumple_for_keyval_qiv: + * @src: the flat dictionary (only scalar values) to crumple + * @errp: location to store error + * + * Like qdict_crumple(), but additionally transforms scalar values so + * the result can be passed to qobject_input_visitor_new_keyval(). + * + * The block subsystem uses this function to prepare its flat QDict + * with possibly confused scalar types for a visit. It should not be + * used for anything else, and it should go away once the block + * subsystem has been cleaned up. + */ +QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp) +{ + QDict *tmp = NULL; + char *buf; + const char *s; + const QDictEntry *ent; + QObject *dst; + + for (ent = qdict_first(src); ent; ent = qdict_next(src, ent)) { + buf = NULL; + switch (qobject_type(ent->value)) { + case QTYPE_QNULL: + case QTYPE_QSTRING: + continue; + case QTYPE_QNUM: + s = buf = qnum_to_string(qobject_to(QNum, ent->value)); + break; + case QTYPE_QDICT: + case QTYPE_QLIST: + /* @src isn't flat; qdict_crumple() will fail */ + continue; + case QTYPE_QBOOL: + s = qbool_get_bool(qobject_to(QBool, ent->value)) + ? "on" : "off"; + break; + default: + abort(); + } + + if (!tmp) { + tmp = qdict_clone_shallow(src); + } + qdict_put(tmp, ent->key, qstring_from_str(s)); + g_free(buf); + } + + dst = qdict_crumple(tmp ?: src, errp); + qobject_unref(tmp); + return dst; +} + /** * qdict_array_entries(): Returns the number of direct array entries if the * sub-QDict of src specified by the prefix in subqdict (or src itself for -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 06/18] block: Fix -drive for certain non-string scalars 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (4 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 05/18] block: Fix -blockdev for certain non-string scalars Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 07/18] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts() Markus Armbruster ` (13 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 07/18] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts() 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (5 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 06/18] block: Fix -drive " Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 08/18] block: Factor out qobject_input_visitor_new_flat_confused() Markus Armbruster ` (12 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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 0a27afa2ef..8c338661db 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3080,7 +3080,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt Error **errp) { BlockdevCreateOptions *create_options = NULL; - QDict *qdict = NULL; + QDict *qdict; QObject *qobj; Visitor *v; BlockDriverState *bs = NULL; @@ -3153,14 +3153,13 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt /* Now get the QAPI type BlockdevCreateOptions */ qobj = qdict_crumple_for_keyval_qiv(qdict, errp); - qobject_unref(qdict); - qdict = qobject_to(QDict, qobj); - if (qdict == NULL) { + if (!qobj) { ret = -EINVAL; goto finish; } - v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + v = qobject_input_visitor_new_keyval(qobj); + qobject_unref(qobj); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); diff --git a/block/qed.c b/block/qed.c index 88fa36d409..fcec760b26 100644 --- a/block/qed.c +++ b/block/qed.c @@ -722,7 +722,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename, Error **errp) { BlockdevCreateOptions *create_options = NULL; - QDict *qdict = NULL; + QDict *qdict; QObject *qobj; Visitor *v; BlockDriverState *bs = NULL; @@ -764,14 +764,13 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename, qdict_put_str(qdict, "file", bs->node_name); qobj = qdict_crumple_for_keyval_qiv(qdict, errp); - qobject_unref(qdict); - qdict = qobject_to(QDict, qobj); - if (qdict == NULL) { + if (!qobj) { ret = -EINVAL; goto fail; } - v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + v = qobject_input_visitor_new_keyval(qobj); + qobject_unref(qobj); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); diff --git a/block/vhdx.c b/block/vhdx.c index 78b29d9fc7..f2aec3d2cd 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1965,7 +1965,7 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, Error **errp) { BlockdevCreateOptions *create_options = NULL; - QDict *qdict = NULL; + QDict *qdict; QObject *qobj; Visitor *v; BlockDriverState *bs = NULL; @@ -2006,14 +2006,13 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, qdict_put_str(qdict, "file", bs->node_name); qobj = qdict_crumple_for_keyval_qiv(qdict, errp); - qobject_unref(qdict); - qdict = qobject_to(QDict, qobj); - if (qdict == NULL) { + if (!qobj) { ret = -EINVAL; goto fail; } - v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + v = qobject_input_visitor_new_keyval(qobj); + qobject_unref(qobj); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); diff --git a/block/vpc.c b/block/vpc.c index 16178e5a90..a9bb04149d 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -1081,7 +1081,7 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts, Error **errp) { BlockdevCreateOptions *create_options = NULL; - QDict *qdict = NULL; + QDict *qdict; QObject *qobj; Visitor *v; BlockDriverState *bs = NULL; @@ -1120,14 +1120,13 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, qdict_put_str(qdict, "file", bs->node_name); qobj = qdict_crumple_for_keyval_qiv(qdict, errp); - qobject_unref(qdict); - qdict = qobject_to(QDict, qobj); - if (qdict == NULL) { + if (!qobj) { ret = -EINVAL; goto fail; } - v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + v = qobject_input_visitor_new_keyval(qobj); + qobject_unref(qobj); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 08/18] block: Factor out qobject_input_visitor_new_flat_confused() 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (6 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 07/18] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts() Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 09/18] block: Make remaining uses of qobject input visitor more robust Markus Armbruster ` (11 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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 8c338661db..945132f692 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3081,7 +3081,6 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt { BlockdevCreateOptions *create_options = NULL; QDict *qdict; - QObject *qobj; Visitor *v; BlockDriverState *bs = NULL; Error *local_err = NULL; @@ -3152,14 +3151,12 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt qdict_put_str(qdict, "file", bs->node_name); /* Now get the QAPI type BlockdevCreateOptions */ - qobj = qdict_crumple_for_keyval_qiv(qdict, errp); - if (!qobj) { + v = qobject_input_visitor_new_flat_confused(qdict, errp); + if (!v) { ret = -EINVAL; goto finish; } - v = qobject_input_visitor_new_keyval(qobj); - qobject_unref(qobj); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); diff --git a/block/qed.c b/block/qed.c index fcec760b26..2363814538 100644 --- a/block/qed.c +++ b/block/qed.c @@ -723,7 +723,6 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename, { BlockdevCreateOptions *create_options = NULL; QDict *qdict; - QObject *qobj; Visitor *v; BlockDriverState *bs = NULL; Error *local_err = NULL; @@ -763,14 +762,12 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename, qdict_put_str(qdict, "driver", "qed"); qdict_put_str(qdict, "file", bs->node_name); - qobj = qdict_crumple_for_keyval_qiv(qdict, errp); - if (!qobj) { + v = qobject_input_visitor_new_flat_confused(qdict, errp); + if (!v) { ret = -EINVAL; goto fail; } - v = qobject_input_visitor_new_keyval(qobj); - qobject_unref(qobj); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); diff --git a/block/rbd.c b/block/rbd.c index 09720e97c0..82346a2a5e 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -630,7 +630,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, BDRVRBDState *s = bs->opaque; BlockdevOptionsRbd *opts = NULL; Visitor *v; - QObject *crumpled = NULL; const QDictEntry *e; Error *local_err = NULL; char *keypairs, *secretid; @@ -647,16 +646,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, } /* Convert the remaining options into a QAPI object */ - crumpled = qdict_crumple_for_keyval_qiv(options, errp); - if (crumpled == NULL) { + v = qobject_input_visitor_new_flat_confused(options, errp); + if (!v) { r = -EINVAL; goto out; } - v = qobject_input_visitor_new_keyval(crumpled); visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err); visit_free(v); - qobject_unref(crumpled); if (local_err) { error_propagate(errp, local_err); diff --git a/block/sheepdog.c b/block/sheepdog.c index 29e3e1eaaa..665b1763eb 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -539,19 +539,17 @@ static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s, static SocketAddress *sd_server_config(QDict *options, Error **errp) { QDict *server = NULL; - QObject *crumpled_server = NULL; Visitor *iv = NULL; SocketAddress *saddr = NULL; Error *local_err = NULL; qdict_extract_subqdict(options, &server, "server."); - crumpled_server = qdict_crumple_for_keyval_qiv(server, errp); - if (!crumpled_server) { + iv = qobject_input_visitor_new_flat_confused(server, errp); + if (!iv) { goto done; } - iv = qobject_input_visitor_new_keyval(crumpled_server); visit_type_SocketAddress(iv, NULL, &saddr, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -560,7 +558,6 @@ static SocketAddress *sd_server_config(QDict *options, Error **errp) done: visit_free(iv); - qobject_unref(crumpled_server); qobject_unref(server); return saddr; } @@ -2173,7 +2170,6 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, { BlockdevCreateOptions *create_options = NULL; QDict *qdict, *location_qdict; - QObject *crumpled; Visitor *v; char *redundancy; Error *local_err = NULL; @@ -2209,16 +2205,14 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, } /* Get the QAPI object */ - crumpled = qdict_crumple_for_keyval_qiv(qdict, errp); - if (crumpled == NULL) { + v = qobject_input_visitor_new_flat_confused(qdict, errp); + if (!v) { ret = -EINVAL; goto fail; } - v = qobject_input_visitor_new_keyval(crumpled); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); - qobject_unref(crumpled); if (local_err) { error_propagate(errp, local_err); diff --git a/block/ssh.c b/block/ssh.c index bd85d989d5..da7bbf73e2 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -606,7 +606,6 @@ static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp) BlockdevOptionsSsh *result = NULL; QemuOpts *opts = NULL; Error *local_err = NULL; - QObject *crumpled; const QDictEntry *e; Visitor *v; @@ -623,15 +622,13 @@ static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp) } /* Create the QAPI object */ - crumpled = qdict_crumple_for_keyval_qiv(options, errp); - if (crumpled == NULL) { + v = qobject_input_visitor_new_flat_confused(options, errp); + if (!v) { goto fail; } - v = qobject_input_visitor_new_keyval(crumpled); visit_type_BlockdevOptionsSsh(v, NULL, &result, &local_err); visit_free(v); - qobject_unref(crumpled); if (local_err) { error_propagate(errp, local_err); diff --git a/block/vhdx.c b/block/vhdx.c index f2aec3d2cd..a677703a9e 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1966,7 +1966,6 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, { BlockdevCreateOptions *create_options = NULL; QDict *qdict; - QObject *qobj; Visitor *v; BlockDriverState *bs = NULL; Error *local_err = NULL; @@ -2005,14 +2004,12 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, qdict_put_str(qdict, "driver", "vhdx"); qdict_put_str(qdict, "file", bs->node_name); - qobj = qdict_crumple_for_keyval_qiv(qdict, errp); - if (!qobj) { + v = qobject_input_visitor_new_flat_confused(qdict, errp); + if (!v) { ret = -EINVAL; goto fail; } - v = qobject_input_visitor_new_keyval(qobj); - qobject_unref(qobj); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); diff --git a/block/vpc.c b/block/vpc.c index a9bb04149d..bf294abfa7 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -1082,7 +1082,6 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, { BlockdevCreateOptions *create_options = NULL; QDict *qdict; - QObject *qobj; Visitor *v; BlockDriverState *bs = NULL; Error *local_err = NULL; @@ -1119,14 +1118,12 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, qdict_put_str(qdict, "driver", "vpc"); qdict_put_str(qdict, "file", bs->node_name); - qobj = qdict_crumple_for_keyval_qiv(qdict, errp); - if (!qobj) { + v = qobject_input_visitor_new_flat_confused(qdict, errp); + if (!v) { ret = -EINVAL; goto fail; } - v = qobject_input_visitor_new_keyval(qobj); - qobject_unref(qobj); visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); visit_free(v); diff --git a/include/block/qdict.h b/include/block/qdict.h index 47d9638c37..d8cb502d7d 100644 --- a/include/block/qdict.h +++ b/include/block/qdict.h @@ -21,7 +21,6 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start); void qdict_array_split(QDict *src, QList **dst); int qdict_array_entries(QDict *src, const char *subqdict); QObject *qdict_crumple(const QDict *src, Error **errp); -QObject *qdict_crumple_for_keyval_qiv(QDict *qdict, Error **errp); void qdict_flatten(QDict *qdict); typedef struct QDictRenames { @@ -30,4 +29,6 @@ typedef struct QDictRenames { } QDictRenames; bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp); +Visitor *qobject_input_visitor_new_flat_confused(QDict *qdict, + Error **errp); #endif diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c index aba372c2eb..41f39abc4a 100644 --- a/qobject/block-qdict.c +++ b/qobject/block-qdict.c @@ -13,6 +13,7 @@ #include "qapi/qmp/qlist.h" #include "qapi/qmp/qnum.h" #include "qapi/qmp/qstring.h" +#include "qapi/qobject-input-visitor.h" #include "qemu/cutils.h" #include "qapi/error.h" @@ -529,7 +530,7 @@ QObject *qdict_crumple(const QDict *src, Error **errp) * used for anything else, and it should go away once the block * subsystem has been cleaned up. */ -QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp) +static QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp) { QDict *tmp = NULL; char *buf; @@ -695,3 +696,28 @@ bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp) } return true; } + +/* + * Create a QObject input visitor for flat @qdict with possibly + * confused scalar types. + * + * The block subsystem uses this function to visit its flat QDict with + * possibly confused scalar types. It should not be used for anything + * else, and it should go away once the block subsystem has been + * cleaned up. + */ +Visitor *qobject_input_visitor_new_flat_confused(QDict *qdict, + Error **errp) +{ + QObject *crumpled; + Visitor *v; + + crumpled = qdict_crumple_for_keyval_qiv(qdict, errp); + if (!crumpled) { + return NULL; + } + + v = qobject_input_visitor_new_keyval(crumpled); + qobject_unref(crumpled); + return v; +} -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 09/18] block: Make remaining uses of qobject input visitor more robust 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (7 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 08/18] block: Factor out qobject_input_visitor_new_flat_confused() Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 10/18] block-qdict: Simplify qdict_flatten_qdict() Markus Armbruster ` (10 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 10/18] block-qdict: Simplify qdict_flatten_qdict() 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (8 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 09/18] block: Make remaining uses of qobject input visitor more robust Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 11/18] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist() Markus Armbruster ` (9 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 11/18] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist() 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (9 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 10/18] block-qdict: Simplify qdict_flatten_qdict() Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit Markus Armbruster ` (8 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (10 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 11/18] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist() Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 15:39 ` Kevin Wolf 2018-06-12 12:58 ` [Qemu-devel] [PATCH 13/18] block-qdict: Simplify qdict_is_list() some Markus Armbruster ` (7 subsequent siblings) 19 siblings, 1 reply; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit 2018-06-12 12:58 ` [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit Markus Armbruster @ 2018-06-12 15:39 ` Kevin Wolf 2018-06-13 15:23 ` Markus Armbruster 0 siblings, 1 reply; 32+ messages in thread From: Kevin Wolf @ 2018-06-12 15:39 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, qemu-block, mreitz, jcody, eblake Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > When you mix scalar and non-scalar keys, whether you get an "already > set as scalar" or an "already set as dict" error depends on qdict > iteration order. Neither message makes much sense. Replace by > ""Cannot mix scalar and non-scalar keys". This is similar to the > message we get for mixing list and non-list keys. > > I find qdict_crumple()'s first loop hard to understand. Rearrange it > and add a comment. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> To be honest, I found the old version of the loop more obvious. > qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c > index a4e1c8d08f..35e9052816 100644 > --- a/qobject/block-qdict.c > +++ b/qobject/block-qdict.c > @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) > QObject *qdict_crumple(const QDict *src, Error **errp) > { > const QDictEntry *ent; > - QDict *two_level, *multi_level = NULL; > + QDict *two_level, *multi_level = NULL, *child_dict; > QObject *dst = NULL, *child; > size_t i; > char *prefix = NULL; > @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) > } > > qdict_split_flat_key(ent->key, &prefix, &suffix); > - > child = qdict_get(two_level, prefix); > + child_dict = qobject_to(QDict, child); > + > + if (child) { > + /* > + * An existing child must be a dict and @ent must be a > + * dict member (i.e. suffix not null), or else @ent > + * clashes. > + */ > + if (!child_dict || !suffix) { > + error_setg(errp, > + "Cannot mix scalar and non-scalar keys"); > + goto error; > + } > + } else if (suffix) { > + child_dict = qdict_new(); > + qdict_put(two_level, prefix, child_dict); > + } else { > + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > + } At least, can you please move the else branch to the if below so that the addition of the new entry is symmetrical for both scalars and dicts? As the code is, it mixes the conflict check, creation of the child dict and addition of scalars (but not to the child dict) in a weird way in a single if block. Or maybe organise it like this: if (child && !(child_dict && suffix)) { error } if (suffix) { if (!child_dict) { create it add it to two_level } add entry to child_dict } else { add entry to two_level } Kevin ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit 2018-06-12 15:39 ` Kevin Wolf @ 2018-06-13 15:23 ` Markus Armbruster 2018-06-14 8:40 ` Kevin Wolf 0 siblings, 1 reply; 32+ messages in thread From: Markus Armbruster @ 2018-06-13 15:23 UTC (permalink / raw) To: Kevin Wolf; +Cc: Markus Armbruster, jcody, qemu-devel, qemu-block, mreitz Kevin Wolf <kwolf@redhat.com> writes: > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: >> When you mix scalar and non-scalar keys, whether you get an "already >> set as scalar" or an "already set as dict" error depends on qdict >> iteration order. Neither message makes much sense. Replace by >> ""Cannot mix scalar and non-scalar keys". This is similar to the >> message we get for mixing list and non-list keys. >> >> I find qdict_crumple()'s first loop hard to understand. Rearrange it >> and add a comment. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > To be honest, I found the old version of the loop more obvious. > >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- >> 1 file changed, 21 insertions(+), 21 deletions(-) >> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c >> index a4e1c8d08f..35e9052816 100644 >> --- a/qobject/block-qdict.c >> +++ b/qobject/block-qdict.c >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) >> QObject *qdict_crumple(const QDict *src, Error **errp) >> { >> const QDictEntry *ent; >> - QDict *two_level, *multi_level = NULL; >> + QDict *two_level, *multi_level = NULL, *child_dict; >> QObject *dst = NULL, *child; >> size_t i; >> char *prefix = NULL; >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) >> } >> >> qdict_split_flat_key(ent->key, &prefix, &suffix); >> - >> child = qdict_get(two_level, prefix); >> + child_dict = qobject_to(QDict, child); >> + >> + if (child) { >> + /* >> + * An existing child must be a dict and @ent must be a >> + * dict member (i.e. suffix not null), or else @ent >> + * clashes. >> + */ >> + if (!child_dict || !suffix) { >> + error_setg(errp, >> + "Cannot mix scalar and non-scalar keys"); >> + goto error; >> + } >> + } else if (suffix) { >> + child_dict = qdict_new(); >> + qdict_put(two_level, prefix, child_dict); >> + } else { >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); >> + } > > At least, can you please move the else branch to the if below so that > the addition of the new entry is symmetrical for both scalars and dicts? > As the code is, it mixes the conflict check, creation of the child dict > and addition of scalars (but not to the child dict) in a weird way in a > single if block. > > Or maybe organise it like this: > > if (child && !(child_dict && suffix)) { > error > } > > if (suffix) { > if (!child_dict) { > create it > add it to two_level > } > add entry to child_dict > } else { > add entry to two_level > } Fleshing out... if (child && !child_dict) { /* * @prefix already exists and it's a non-dictionary, * i.e. we've seen a scalar with key @prefix. The same * key can't occur twice, therefore suffix must be * non-null. */ assert(suffix); /* * @ent has key @prefix.@suffix, but we've already seen * key @prefix: clash. */ error_setg(errp, "Cannot mix scalar and non-scalar keys"); goto error; } if (suffix) { if (!child_dict) { child_dict = qdict_new(); qdict_put(two_level, prefix, child_dict); } qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); } else { assert(!child); qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); } Okay? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit 2018-06-13 15:23 ` Markus Armbruster @ 2018-06-14 8:40 ` Kevin Wolf 2018-06-14 8:46 ` Daniel P. Berrangé 2018-06-14 11:52 ` Markus Armbruster 0 siblings, 2 replies; 32+ messages in thread From: Kevin Wolf @ 2018-06-14 8:40 UTC (permalink / raw) To: Markus Armbruster; +Cc: jcody, qemu-devel, qemu-block, mreitz Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > >> When you mix scalar and non-scalar keys, whether you get an "already > >> set as scalar" or an "already set as dict" error depends on qdict > >> iteration order. Neither message makes much sense. Replace by > >> ""Cannot mix scalar and non-scalar keys". This is similar to the > >> message we get for mixing list and non-list keys. > >> > >> I find qdict_crumple()'s first loop hard to understand. Rearrange it > >> and add a comment. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > To be honest, I found the old version of the loop more obvious. > > > >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- > >> 1 file changed, 21 insertions(+), 21 deletions(-) > >> > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c > >> index a4e1c8d08f..35e9052816 100644 > >> --- a/qobject/block-qdict.c > >> +++ b/qobject/block-qdict.c > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) > >> QObject *qdict_crumple(const QDict *src, Error **errp) > >> { > >> const QDictEntry *ent; > >> - QDict *two_level, *multi_level = NULL; > >> + QDict *two_level, *multi_level = NULL, *child_dict; > >> QObject *dst = NULL, *child; > >> size_t i; > >> char *prefix = NULL; > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) > >> } > >> > >> qdict_split_flat_key(ent->key, &prefix, &suffix); > >> - > >> child = qdict_get(two_level, prefix); > >> + child_dict = qobject_to(QDict, child); > >> + > >> + if (child) { > >> + /* > >> + * An existing child must be a dict and @ent must be a > >> + * dict member (i.e. suffix not null), or else @ent > >> + * clashes. > >> + */ > >> + if (!child_dict || !suffix) { > >> + error_setg(errp, > >> + "Cannot mix scalar and non-scalar keys"); > >> + goto error; > >> + } > >> + } else if (suffix) { > >> + child_dict = qdict_new(); > >> + qdict_put(two_level, prefix, child_dict); > >> + } else { > >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > >> + } > > > > At least, can you please move the else branch to the if below so that > > the addition of the new entry is symmetrical for both scalars and dicts? > > As the code is, it mixes the conflict check, creation of the child dict > > and addition of scalars (but not to the child dict) in a weird way in a > > single if block. > > > > Or maybe organise it like this: > > > > if (child && !(child_dict && suffix)) { > > error > > } > > > > if (suffix) { > > if (!child_dict) { > > create it > > add it to two_level > > } > > add entry to child_dict > > } else { > > add entry to two_level > > } > > Fleshing out... > > if (child && !child_dict) { > /* > * @prefix already exists and it's a non-dictionary, > * i.e. we've seen a scalar with key @prefix. The same > * key can't occur twice, therefore suffix must be > * non-null. > */ > assert(suffix); > /* > * @ent has key @prefix.@suffix, but we've already seen > * key @prefix: clash. > */ > error_setg(errp, "Cannot mix scalar and non-scalar keys"); > goto error; > } This catches "foo.bar" after "foo", but not "foo" after "foo.bar". > if (suffix) { > if (!child_dict) { > child_dict = qdict_new(); > qdict_put(two_level, prefix, child_dict); > } > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); > } else { > assert(!child); So "foo" after "foo.bar" would fail this assertion. > qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > } > > Okay? Kevin ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit 2018-06-14 8:40 ` Kevin Wolf @ 2018-06-14 8:46 ` Daniel P. Berrangé 2018-06-14 13:11 ` Markus Armbruster 2018-06-14 11:52 ` Markus Armbruster 1 sibling, 1 reply; 32+ messages in thread From: Daniel P. Berrangé @ 2018-06-14 8:46 UTC (permalink / raw) To: Kevin Wolf; +Cc: Markus Armbruster, jcody, qemu-devel, qemu-block, mreitz On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote: > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: > > Kevin Wolf <kwolf@redhat.com> writes: > > > > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > > >> When you mix scalar and non-scalar keys, whether you get an "already > > >> set as scalar" or an "already set as dict" error depends on qdict > > >> iteration order. Neither message makes much sense. Replace by > > >> ""Cannot mix scalar and non-scalar keys". This is similar to the > > >> message we get for mixing list and non-list keys. > > >> > > >> I find qdict_crumple()'s first loop hard to understand. Rearrange it > > >> and add a comment. > > >> > > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > > > To be honest, I found the old version of the loop more obvious. > > > > > >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- > > >> 1 file changed, 21 insertions(+), 21 deletions(-) > > >> > > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c > > >> index a4e1c8d08f..35e9052816 100644 > > >> --- a/qobject/block-qdict.c > > >> +++ b/qobject/block-qdict.c > > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) > > >> QObject *qdict_crumple(const QDict *src, Error **errp) > > >> { > > >> const QDictEntry *ent; > > >> - QDict *two_level, *multi_level = NULL; > > >> + QDict *two_level, *multi_level = NULL, *child_dict; > > >> QObject *dst = NULL, *child; > > >> size_t i; > > >> char *prefix = NULL; > > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) > > >> } > > >> > > >> qdict_split_flat_key(ent->key, &prefix, &suffix); > > >> - > > >> child = qdict_get(two_level, prefix); > > >> + child_dict = qobject_to(QDict, child); > > >> + > > >> + if (child) { > > >> + /* > > >> + * An existing child must be a dict and @ent must be a > > >> + * dict member (i.e. suffix not null), or else @ent > > >> + * clashes. > > >> + */ > > >> + if (!child_dict || !suffix) { > > >> + error_setg(errp, > > >> + "Cannot mix scalar and non-scalar keys"); > > >> + goto error; > > >> + } > > >> + } else if (suffix) { > > >> + child_dict = qdict_new(); > > >> + qdict_put(two_level, prefix, child_dict); > > >> + } else { > > >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > > >> + } > > > > > > At least, can you please move the else branch to the if below so that > > > the addition of the new entry is symmetrical for both scalars and dicts? > > > As the code is, it mixes the conflict check, creation of the child dict > > > and addition of scalars (but not to the child dict) in a weird way in a > > > single if block. > > > > > > Or maybe organise it like this: > > > > > > if (child && !(child_dict && suffix)) { > > > error > > > } > > > > > > if (suffix) { > > > if (!child_dict) { > > > create it > > > add it to two_level > > > } > > > add entry to child_dict > > > } else { > > > add entry to two_level > > > } > > > > Fleshing out... > > > > if (child && !child_dict) { > > /* > > * @prefix already exists and it's a non-dictionary, > > * i.e. we've seen a scalar with key @prefix. The same > > * key can't occur twice, therefore suffix must be > > * non-null. > > */ > > assert(suffix); > > /* > > * @ent has key @prefix.@suffix, but we've already seen > > * key @prefix: clash. > > */ > > error_setg(errp, "Cannot mix scalar and non-scalar keys"); > > goto error; > > } > > This catches "foo.bar" after "foo", but not "foo" after "foo.bar". > > > if (suffix) { > > if (!child_dict) { > > child_dict = qdict_new(); > > qdict_put(two_level, prefix, child_dict); > > } > > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); > > } else { > > assert(!child); > > So "foo" after "foo.bar" would fail this assertion. We got coverage of bad inputs in tests/check-qdict.c If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios we should extend it to make sure we detect them during testing Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit 2018-06-14 8:46 ` Daniel P. Berrangé @ 2018-06-14 13:11 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-14 13:11 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Kevin Wolf, jcody, mreitz, Markus Armbruster, qemu-block, qemu-devel Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote: >> Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: >> > Kevin Wolf <kwolf@redhat.com> writes: >> > >> > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: >> > >> When you mix scalar and non-scalar keys, whether you get an "already >> > >> set as scalar" or an "already set as dict" error depends on qdict >> > >> iteration order. Neither message makes much sense. Replace by >> > >> ""Cannot mix scalar and non-scalar keys". This is similar to the >> > >> message we get for mixing list and non-list keys. >> > >> >> > >> I find qdict_crumple()'s first loop hard to understand. Rearrange it >> > >> and add a comment. >> > >> >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> > > >> > > To be honest, I found the old version of the loop more obvious. >> > > >> > >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- >> > >> 1 file changed, 21 insertions(+), 21 deletions(-) >> > >> >> > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c >> > >> index a4e1c8d08f..35e9052816 100644 >> > >> --- a/qobject/block-qdict.c >> > >> +++ b/qobject/block-qdict.c >> > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) >> > >> QObject *qdict_crumple(const QDict *src, Error **errp) >> > >> { >> > >> const QDictEntry *ent; >> > >> - QDict *two_level, *multi_level = NULL; >> > >> + QDict *two_level, *multi_level = NULL, *child_dict; >> > >> QObject *dst = NULL, *child; >> > >> size_t i; >> > >> char *prefix = NULL; >> > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) >> > >> } >> > >> >> > >> qdict_split_flat_key(ent->key, &prefix, &suffix); >> > >> - >> > >> child = qdict_get(two_level, prefix); >> > >> + child_dict = qobject_to(QDict, child); >> > >> + >> > >> + if (child) { >> > >> + /* >> > >> + * An existing child must be a dict and @ent must be a >> > >> + * dict member (i.e. suffix not null), or else @ent >> > >> + * clashes. >> > >> + */ >> > >> + if (!child_dict || !suffix) { >> > >> + error_setg(errp, >> > >> + "Cannot mix scalar and non-scalar keys"); >> > >> + goto error; >> > >> + } >> > >> + } else if (suffix) { >> > >> + child_dict = qdict_new(); >> > >> + qdict_put(two_level, prefix, child_dict); >> > >> + } else { >> > >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); >> > >> + } >> > > >> > > At least, can you please move the else branch to the if below so that >> > > the addition of the new entry is symmetrical for both scalars and dicts? >> > > As the code is, it mixes the conflict check, creation of the child dict >> > > and addition of scalars (but not to the child dict) in a weird way in a >> > > single if block. >> > > >> > > Or maybe organise it like this: >> > > >> > > if (child && !(child_dict && suffix)) { >> > > error >> > > } >> > > >> > > if (suffix) { >> > > if (!child_dict) { >> > > create it >> > > add it to two_level >> > > } >> > > add entry to child_dict >> > > } else { >> > > add entry to two_level >> > > } >> > >> > Fleshing out... >> > >> > if (child && !child_dict) { >> > /* >> > * @prefix already exists and it's a non-dictionary, >> > * i.e. we've seen a scalar with key @prefix. The same >> > * key can't occur twice, therefore suffix must be >> > * non-null. >> > */ >> > assert(suffix); >> > /* >> > * @ent has key @prefix.@suffix, but we've already seen >> > * key @prefix: clash. >> > */ >> > error_setg(errp, "Cannot mix scalar and non-scalar keys"); >> > goto error; >> > } >> >> This catches "foo.bar" after "foo", but not "foo" after "foo.bar". >> >> > if (suffix) { >> > if (!child_dict) { >> > child_dict = qdict_new(); >> > qdict_put(two_level, prefix, child_dict); >> > } >> > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); >> > } else { >> > assert(!child); >> >> So "foo" after "foo.bar" would fail this assertion. > > We got coverage of bad inputs in tests/check-qdict.c > > If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios > we should extend it to make sure we detect them during testing Depends on qdict_first() / qdict_next() traversal order, which makes it awkward. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit 2018-06-14 8:40 ` Kevin Wolf 2018-06-14 8:46 ` Daniel P. Berrangé @ 2018-06-14 11:52 ` Markus Armbruster 2018-06-14 13:26 ` Kevin Wolf 1 sibling, 1 reply; 32+ messages in thread From: Markus Armbruster @ 2018-06-14 11:52 UTC (permalink / raw) To: Kevin Wolf; +Cc: Markus Armbruster, jcody, qemu-devel, qemu-block, mreitz Kevin Wolf <kwolf@redhat.com> writes: > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: >> >> When you mix scalar and non-scalar keys, whether you get an "already >> >> set as scalar" or an "already set as dict" error depends on qdict >> >> iteration order. Neither message makes much sense. Replace by >> >> ""Cannot mix scalar and non-scalar keys". This is similar to the >> >> message we get for mixing list and non-list keys. >> >> >> >> I find qdict_crumple()'s first loop hard to understand. Rearrange it >> >> and add a comment. >> >> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> > >> > To be honest, I found the old version of the loop more obvious. >> > >> >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- >> >> 1 file changed, 21 insertions(+), 21 deletions(-) >> >> >> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c >> >> index a4e1c8d08f..35e9052816 100644 >> >> --- a/qobject/block-qdict.c >> >> +++ b/qobject/block-qdict.c >> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) >> >> QObject *qdict_crumple(const QDict *src, Error **errp) >> >> { >> >> const QDictEntry *ent; >> >> - QDict *two_level, *multi_level = NULL; >> >> + QDict *two_level, *multi_level = NULL, *child_dict; >> >> QObject *dst = NULL, *child; >> >> size_t i; >> >> char *prefix = NULL; >> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) >> >> } >> >> >> >> qdict_split_flat_key(ent->key, &prefix, &suffix); >> >> - >> >> child = qdict_get(two_level, prefix); >> >> + child_dict = qobject_to(QDict, child); >> >> + >> >> + if (child) { >> >> + /* >> >> + * An existing child must be a dict and @ent must be a >> >> + * dict member (i.e. suffix not null), or else @ent >> >> + * clashes. >> >> + */ >> >> + if (!child_dict || !suffix) { >> >> + error_setg(errp, >> >> + "Cannot mix scalar and non-scalar keys"); >> >> + goto error; >> >> + } >> >> + } else if (suffix) { >> >> + child_dict = qdict_new(); >> >> + qdict_put(two_level, prefix, child_dict); >> >> + } else { >> >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); >> >> + } >> > >> > At least, can you please move the else branch to the if below so that >> > the addition of the new entry is symmetrical for both scalars and dicts? >> > As the code is, it mixes the conflict check, creation of the child dict >> > and addition of scalars (but not to the child dict) in a weird way in a >> > single if block. >> > >> > Or maybe organise it like this: >> > >> > if (child && !(child_dict && suffix)) { >> > error >> > } >> > >> > if (suffix) { >> > if (!child_dict) { >> > create it >> > add it to two_level >> > } >> > add entry to child_dict >> > } else { >> > add entry to two_level >> > } >> >> Fleshing out... >> >> if (child && !child_dict) { >> /* >> * @prefix already exists and it's a non-dictionary, >> * i.e. we've seen a scalar with key @prefix. The same >> * key can't occur twice, therefore suffix must be >> * non-null. >> */ >> assert(suffix); >> /* >> * @ent has key @prefix.@suffix, but we've already seen >> * key @prefix: clash. >> */ >> error_setg(errp, "Cannot mix scalar and non-scalar keys"); >> goto error; >> } > > This catches "foo.bar" after "foo", but not "foo" after "foo.bar". > >> if (suffix) { >> if (!child_dict) { >> child_dict = qdict_new(); >> qdict_put(two_level, prefix, child_dict); >> } >> qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); >> } else { >> assert(!child); > > So "foo" after "foo.bar" would fail this assertion. > >> qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); >> } >> >> Okay? > > Kevin I feel dumb. Next try: if (child) { /* * If @child_dict, then all previous keys with this prefix * had a suffix. If @suffix, this one has one as well, * and we're good, else there's a clash. */ if (!child_dict || !suffix) { error_setg(errp, "Cannot mix scalar and non-scalar keys"); goto error; } } if (suffix) { if (!child_dict) { child_dict = qdict_new(); qdict_put(two_level, prefix, child_dict); } qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); } else { qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); } ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit 2018-06-14 11:52 ` Markus Armbruster @ 2018-06-14 13:26 ` Kevin Wolf 0 siblings, 0 replies; 32+ messages in thread From: Kevin Wolf @ 2018-06-14 13:26 UTC (permalink / raw) To: Markus Armbruster; +Cc: jcody, qemu-devel, qemu-block, mreitz Am 14.06.2018 um 13:52 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> > >> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > >> >> When you mix scalar and non-scalar keys, whether you get an "already > >> >> set as scalar" or an "already set as dict" error depends on qdict > >> >> iteration order. Neither message makes much sense. Replace by > >> >> ""Cannot mix scalar and non-scalar keys". This is similar to the > >> >> message we get for mixing list and non-list keys. > >> >> > >> >> I find qdict_crumple()'s first loop hard to understand. Rearrange it > >> >> and add a comment. > >> >> > >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> > > >> > To be honest, I found the old version of the loop more obvious. > >> > > >> >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- > >> >> 1 file changed, 21 insertions(+), 21 deletions(-) > >> >> > >> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c > >> >> index a4e1c8d08f..35e9052816 100644 > >> >> --- a/qobject/block-qdict.c > >> >> +++ b/qobject/block-qdict.c > >> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) > >> >> QObject *qdict_crumple(const QDict *src, Error **errp) > >> >> { > >> >> const QDictEntry *ent; > >> >> - QDict *two_level, *multi_level = NULL; > >> >> + QDict *two_level, *multi_level = NULL, *child_dict; > >> >> QObject *dst = NULL, *child; > >> >> size_t i; > >> >> char *prefix = NULL; > >> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) > >> >> } > >> >> > >> >> qdict_split_flat_key(ent->key, &prefix, &suffix); > >> >> - > >> >> child = qdict_get(two_level, prefix); > >> >> + child_dict = qobject_to(QDict, child); > >> >> + > >> >> + if (child) { > >> >> + /* > >> >> + * An existing child must be a dict and @ent must be a > >> >> + * dict member (i.e. suffix not null), or else @ent > >> >> + * clashes. > >> >> + */ > >> >> + if (!child_dict || !suffix) { > >> >> + error_setg(errp, > >> >> + "Cannot mix scalar and non-scalar keys"); > >> >> + goto error; > >> >> + } > >> >> + } else if (suffix) { > >> >> + child_dict = qdict_new(); > >> >> + qdict_put(two_level, prefix, child_dict); > >> >> + } else { > >> >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > >> >> + } > >> > > >> > At least, can you please move the else branch to the if below so that > >> > the addition of the new entry is symmetrical for both scalars and dicts? > >> > As the code is, it mixes the conflict check, creation of the child dict > >> > and addition of scalars (but not to the child dict) in a weird way in a > >> > single if block. > >> > > >> > Or maybe organise it like this: > >> > > >> > if (child && !(child_dict && suffix)) { > >> > error > >> > } > >> > > >> > if (suffix) { > >> > if (!child_dict) { > >> > create it > >> > add it to two_level > >> > } > >> > add entry to child_dict > >> > } else { > >> > add entry to two_level > >> > } > >> > >> Fleshing out... > >> > >> if (child && !child_dict) { > >> /* > >> * @prefix already exists and it's a non-dictionary, > >> * i.e. we've seen a scalar with key @prefix. The same > >> * key can't occur twice, therefore suffix must be > >> * non-null. > >> */ > >> assert(suffix); > >> /* > >> * @ent has key @prefix.@suffix, but we've already seen > >> * key @prefix: clash. > >> */ > >> error_setg(errp, "Cannot mix scalar and non-scalar keys"); > >> goto error; > >> } > > > > This catches "foo.bar" after "foo", but not "foo" after "foo.bar". > > > >> if (suffix) { > >> if (!child_dict) { > >> child_dict = qdict_new(); > >> qdict_put(two_level, prefix, child_dict); > >> } > >> qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); > >> } else { > >> assert(!child); > > > > So "foo" after "foo.bar" would fail this assertion. > > > >> qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > >> } > >> > >> Okay? > > > > Kevin > > I feel dumb. Next try: > > if (child) { > /* > * If @child_dict, then all previous keys with this prefix > * had a suffix. If @suffix, this one has one as well, > * and we're good, else there's a clash. > */ > if (!child_dict || !suffix) { > error_setg(errp, "Cannot mix scalar and non-scalar keys"); > goto error; > } > } > > if (suffix) { > if (!child_dict) { > child_dict = qdict_new(); > qdict_put(two_level, prefix, child_dict); > } > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); > } else { > qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > } This one looks good to me. (Though I feel your comments explains the !(child_dict && suffix) form that I chose rather than what you actually have in the code. But I don't mind.) Kevin ^ permalink raw reply [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 13/18] block-qdict: Simplify qdict_is_list() some 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (11 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 14/18] check-block-qdict: Rename qdict_flatten()'s variables for clarity Markus Armbruster ` (6 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 14/18] check-block-qdict: Rename qdict_flatten()'s variables for clarity 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (12 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 13/18] block-qdict: Simplify qdict_is_list() some Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 15/18] check-block-qdict: Cover flattening of empty lists and dictionaries Markus Armbruster ` (5 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 15/18] check-block-qdict: Cover flattening of empty lists and dictionaries 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (13 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 14/18] check-block-qdict: Rename qdict_flatten()'s variables for clarity Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 16/18] block: Fix -blockdev / blockdev-add for empty objects and arrays Markus Armbruster ` (4 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 16/18] block: Fix -blockdev / blockdev-add for empty objects and arrays 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (14 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 15/18] check-block-qdict: Cover flattening of empty lists and dictionaries Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 17/18] rbd: New parameter auth-client-required Markus Armbruster ` (3 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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] 32+ messages in thread
* [Qemu-devel] [PATCH 17/18] rbd: New parameter auth-client-required 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (15 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 16/18] block: Fix -blockdev / blockdev-add for empty objects and arrays Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret Markus Armbruster ` (2 subsequent siblings) 19 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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 | 13 +++++++++++++ 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index 82346a2a5e..ea0575d068 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -240,20 +240,42 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp) static int qemu_rbd_set_auth(rados_t cluster, const char *secretid, + BlockdevOptionsRbd *opts, Error **errp) { - if (secretid == 0) { - return 0; - } + char *acr; + int r; + GString *accu; + RbdAuthModeList *auth; + + if (secretid) { + gchar *secret = qcrypto_secret_lookup_as_base64(secretid, + errp); + if (!secret) { + return -1; + } - gchar *secret = qcrypto_secret_lookup_as_base64(secretid, - errp); - if (!secret) { - return -1; + rados_conf_set(cluster, "key", secret); + g_free(secret); } - rados_conf_set(cluster, "key", secret); - g_free(secret); + if (opts->has_auth_client_required) { + accu = g_string_new(""); + for (auth = opts->auth_client_required; auth; auth = auth->next) { + if (accu->str[0]) { + g_string_append_c(accu, ';'); + } + g_string_append(accu, RbdAuthMode_str(auth->value)); + } + acr = g_string_free(accu, FALSE); + r = rados_conf_set(cluster, "auth_client_required", acr); + g_free(acr); + if (r < 0) { + error_setg_errno(errp, -r, + "Could not set 'auth_client_required'"); + return r; + } + } return 0; } @@ -585,7 +607,7 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, } } - if (qemu_rbd_set_auth(*cluster, secretid, errp) < 0) { + if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) { r = -EIO; goto failed_shutdown; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 4b1de474a9..841d196a21 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3093,6 +3093,14 @@ '*timeout': 'int' } } +## +# @RbdAuthMode: +# +# Since: 3.0 +## +{ 'enum': 'RbdAuthMode', + 'data': [ 'cephx', 'none' ] } + ## # @BlockdevOptionsRbd: # @@ -3108,6 +3116,10 @@ # # @user: Ceph id name. # +# @auth-client-required: Acceptable authentication modes. +# This maps to Ceph configuration option +# "auth_client_required". (Since 3.0) +# # @server: Monitor host address and port. This maps # to the "mon_host" Ceph option. # @@ -3119,6 +3131,7 @@ '*conf': 'str', '*snapshot': 'str', '*user': 'str', + '*auth-client-required': ['RbdAuthMode'], '*server': ['InetSocketAddressBase'] } } ## -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (16 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 17/18] rbd: New parameter auth-client-required Markus Armbruster @ 2018-06-12 12:58 ` Markus Armbruster 2018-06-12 13:20 ` Daniel P. Berrangé 2018-06-12 15:04 ` [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Kevin Wolf 2018-06-12 16:41 ` Kevin Wolf 19 siblings, 1 reply; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 12:58 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 | 6 ++++++ 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index ea0575d068..f2c6965418 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -239,24 +239,25 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp) } -static int qemu_rbd_set_auth(rados_t cluster, const char *secretid, - BlockdevOptionsRbd *opts, +static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts, Error **errp) { - char *acr; + char *key, *acr; int r; GString *accu; RbdAuthModeList *auth; - if (secretid) { - gchar *secret = qcrypto_secret_lookup_as_base64(secretid, - errp); - if (!secret) { - return -1; + if (opts->key_secret) { + key = qcrypto_secret_lookup_as_base64(opts->key_secret, errp); + if (!key) { + return -EIO; + } + r = rados_conf_set(cluster, "key", key); + g_free(key); + if (r < 0) { + error_setg_errno(errp, -r, "Could not set 'key'"); + return r; } - - rados_conf_set(cluster, "key", secret); - g_free(secret); } if (opts->has_auth_client_required) { @@ -367,9 +368,7 @@ static QemuOptsList runtime_opts = { }, }; -/* FIXME Deprecate and remove keypairs or make it available in QMP. - * password_secret should eventually be configurable in opts->location. Support - * for it in .bdrv_open will make it work here as well. */ +/* FIXME Deprecate and remove keypairs or make it available in QMP. */ static int qemu_rbd_do_create(BlockdevCreateOptions *options, const char *keypairs, const char *password_secret, Error **errp) @@ -575,6 +574,16 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, Error *local_err = NULL; int r; + if (secretid) { + if (opts->key_secret) { + error_setg(errp, + "Legacy 'password-secret' clashes with 'key-secret'"); + return -EINVAL; + } + opts->key_secret = g_strdup(secretid); + opts->has_key_secret = true; + } + mon_host = qemu_rbd_mon_host(opts, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -607,8 +616,8 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, } } - if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) { - r = -EIO; + r = qemu_rbd_set_auth(*cluster, opts, errp); + if (r < 0) { goto failed_shutdown; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 841d196a21..3eb536de5b 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3120,6 +3120,11 @@ # This maps to Ceph configuration option # "auth_client_required". (Since 3.0) # +# @key-secret: ID of a QCryptoSecret object providing a key +# for cephx authentication. +# This maps to Ceph configuration option +# "key". (Since 3.0) +# # @server: Monitor host address and port. This maps # to the "mon_host" Ceph option. # @@ -3132,6 +3137,7 @@ '*snapshot': 'str', '*user': 'str', '*auth-client-required': ['RbdAuthMode'], + '*key-secret': 'str', '*server': ['InetSocketAddressBase'] } } ## -- 2.17.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret 2018-06-12 12:58 ` [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret Markus Armbruster @ 2018-06-12 13:20 ` Daniel P. Berrangé 2018-06-12 16:42 ` Markus Armbruster 0 siblings, 1 reply; 32+ messages in thread From: Daniel P. Berrangé @ 2018-06-12 13:20 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, kwolf, jcody, qemu-block, mreitz On Tue, Jun 12, 2018 at 02:58:21PM +0200, Markus Armbruster wrote: > Legacy -drive supports "password-secret" parameter that isn't > available with -blockdev / blockdev-add. That's because we backed out > our first try to provide it there due to interface design doubts, in > commit 577d8c9a811, v2.9.0. > > This is the second try. It brings back the parameter, except it's > named "key-secret" now. > > Let's review our reasons for backing out the first try, as stated in > the commit message: > > * BlockdevOptionsRbd member @password-secret isn't actually a > password, it's a key generated by Ceph. I thought about that when I first added password-secret, but felt that it is still effectively acting as a password to authenticate to the server, and calling it password-secret made it clearer that it was related to the authentication phase, and not for example, disk encryption. > > Addressed by the rename. > > * We're not sure where member @password-secret belongs (see the > previous commit). > > See previous commit. > > * How @password-secret interacts with settings from a configuration > file specified with @conf is undocumented. > > Not actually true, the documentation for @conf says "Values in the > configuration file will be overridden by options specified via QAPI", > and we've tested this. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/rbd.c | 41 +++++++++++++++++++++++++---------------- > qapi/block-core.json | 6 ++++++ > 2 files changed, 31 insertions(+), 16 deletions(-) > > diff --git a/block/rbd.c b/block/rbd.c > index ea0575d068..f2c6965418 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -239,24 +239,25 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp) > } > > > -static int qemu_rbd_set_auth(rados_t cluster, const char *secretid, > - BlockdevOptionsRbd *opts, > +static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts, > Error **errp) > { > - char *acr; > + char *key, *acr; > int r; > GString *accu; > RbdAuthModeList *auth; > > - if (secretid) { > - gchar *secret = qcrypto_secret_lookup_as_base64(secretid, > - errp); > - if (!secret) { > - return -1; > + if (opts->key_secret) { > + key = qcrypto_secret_lookup_as_base64(opts->key_secret, errp); > + if (!key) { > + return -EIO; > + } > + r = rados_conf_set(cluster, "key", key); > + g_free(key); > + if (r < 0) { > + error_setg_errno(errp, -r, "Could not set 'key'"); > + return r; > } > - > - rados_conf_set(cluster, "key", secret); > - g_free(secret); > } > > if (opts->has_auth_client_required) { > @@ -367,9 +368,7 @@ static QemuOptsList runtime_opts = { > }, > }; > > -/* FIXME Deprecate and remove keypairs or make it available in QMP. > - * password_secret should eventually be configurable in opts->location. Support > - * for it in .bdrv_open will make it work here as well. */ > +/* FIXME Deprecate and remove keypairs or make it available in QMP. */ > static int qemu_rbd_do_create(BlockdevCreateOptions *options, > const char *keypairs, const char *password_secret, > Error **errp) > @@ -575,6 +574,16 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, > Error *local_err = NULL; > int r; > > + if (secretid) { > + if (opts->key_secret) { > + error_setg(errp, > + "Legacy 'password-secret' clashes with 'key-secret'"); > + return -EINVAL; > + } > + opts->key_secret = g_strdup(secretid); > + opts->has_key_secret = true; > + } > + > mon_host = qemu_rbd_mon_host(opts, &local_err); > if (local_err) { > error_propagate(errp, local_err); > @@ -607,8 +616,8 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, > } > } > > - if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) { > - r = -EIO; > + r = qemu_rbd_set_auth(*cluster, opts, errp); > + if (r < 0) { > goto failed_shutdown; > } > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 841d196a21..3eb536de5b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3120,6 +3120,11 @@ > # This maps to Ceph configuration option > # "auth_client_required". (Since 3.0) > # > +# @key-secret: ID of a QCryptoSecret object providing a key > +# for cephx authentication. > +# This maps to Ceph configuration option > +# "key". (Since 3.0) > +# > # @server: Monitor host address and port. This maps > # to the "mon_host" Ceph option. > # > @@ -3132,6 +3137,7 @@ > '*snapshot': 'str', > '*user': 'str', > '*auth-client-required': ['RbdAuthMode'], > + '*key-secret': 'str', > '*server': ['InetSocketAddressBase'] } } > > ## > -- > 2.17.1 > > Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret 2018-06-12 13:20 ` Daniel P. Berrangé @ 2018-06-12 16:42 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2018-06-12 16:42 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Markus Armbruster, kwolf, jcody, qemu-devel, qemu-block, mreitz Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Jun 12, 2018 at 02:58:21PM +0200, Markus Armbruster wrote: >> Legacy -drive supports "password-secret" parameter that isn't >> available with -blockdev / blockdev-add. That's because we backed out >> our first try to provide it there due to interface design doubts, in >> commit 577d8c9a811, v2.9.0. >> >> This is the second try. It brings back the parameter, except it's >> named "key-secret" now. >> >> Let's review our reasons for backing out the first try, as stated in >> the commit message: >> >> * BlockdevOptionsRbd member @password-secret isn't actually a >> password, it's a key generated by Ceph. > > I thought about that when I first added password-secret, but felt > that it is still effectively acting as a password to authenticate > to the server, and calling it password-secret made it clearer that > it was related to the authentication phase, and not for example, > disk encryption. I feel it's best to stick to the names Ceph uses, and Ceph calls it "key". [...] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (17 preceding siblings ...) 2018-06-12 12:58 ` [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret Markus Armbruster @ 2018-06-12 15:04 ` Kevin Wolf 2018-06-12 16:41 ` Kevin Wolf 19 siblings, 0 replies; 32+ messages in thread From: Kevin Wolf @ 2018-06-12 15:04 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, qemu-block, mreitz, jcody, eblake Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > PATCH 01-17 are configuration fixes and cleanup, in particular > -blockdev driver=nfs,... and -drive driver=(nbd|sheepdog|ssh),... with > non-string scalars. > > PATCH 18-19 provide support for configuring rbd authentication. > > I'm happy to split the series if that helps. > > Since the RFC post, I amended the documentation of the rbd > authentication parameters, and squashed PATCH 04 into 03. I was still reviewing the RFC while you sent this, so I posted two comments that are still relevant for this version in the RFC thread. Please read them there, I'm not going to duplicate them here. Will continue with the review on this one now. Kevin ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster ` (18 preceding siblings ...) 2018-06-12 15:04 ` [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Kevin Wolf @ 2018-06-12 16:41 ` Kevin Wolf 19 siblings, 0 replies; 32+ messages in thread From: Kevin Wolf @ 2018-06-12 16:41 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, qemu-block, mreitz, jcody, eblake Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > PATCH 01-17 are configuration fixes and cleanup, in particular > -blockdev driver=nfs,... and -drive driver=(nbd|sheepdog|ssh),... with > non-string scalars. > > PATCH 18-19 provide support for configuring rbd authentication. > > I'm happy to split the series if that helps. > > Since the RFC post, I amended the documentation of the rbd > authentication parameters, and squashed PATCH 04 into 03. > > Jeff Cody tested the RFC on his Ceph rig. All results match > expectations. > > The series clashes with Max's "[PATCH 00/13] block: Try to create well > typed json:{} filenames", but Max has rebased his work on mine. I had minor, but still non-trivial comments for patches 9 and 12. The rest is: Reviewed-by: Kevin Wolf <kwolf@redhat.com> ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2018-06-14 13:26 UTC | newest] Thread overview: 32+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-12 12:58 [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename" Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 02/18] iscsi: " Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header Markus Armbruster 2018-06-12 15:02 ` Kevin Wolf 2018-06-12 16:40 ` Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 04/18] qobject: Move block-specific qdict code to block-qdict.c Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 05/18] block: Fix -blockdev for certain non-string scalars Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 06/18] block: Fix -drive " Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 07/18] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts() Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 08/18] block: Factor out qobject_input_visitor_new_flat_confused() Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 09/18] block: Make remaining uses of qobject input visitor more robust Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 10/18] block-qdict: Simplify qdict_flatten_qdict() Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 11/18] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist() Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit Markus Armbruster 2018-06-12 15:39 ` Kevin Wolf 2018-06-13 15:23 ` Markus Armbruster 2018-06-14 8:40 ` Kevin Wolf 2018-06-14 8:46 ` Daniel P. Berrangé 2018-06-14 13:11 ` Markus Armbruster 2018-06-14 11:52 ` Markus Armbruster 2018-06-14 13:26 ` Kevin Wolf 2018-06-12 12:58 ` [Qemu-devel] [PATCH 13/18] block-qdict: Simplify qdict_is_list() some Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 14/18] check-block-qdict: Rename qdict_flatten()'s variables for clarity Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 15/18] check-block-qdict: Cover flattening of empty lists and dictionaries Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 16/18] block: Fix -blockdev / blockdev-add for empty objects and arrays Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 17/18] rbd: New parameter auth-client-required Markus Armbruster 2018-06-12 12:58 ` [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret Markus Armbruster 2018-06-12 13:20 ` Daniel P. Berrangé 2018-06-12 16:42 ` Markus Armbruster 2018-06-12 15:04 ` [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication Kevin Wolf 2018-06-12 16:41 ` Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).