From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35108) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byyaH-0000tJ-IR for qemu-devel@nongnu.org; Tue, 25 Oct 2016 06:03:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byyaC-00008s-CL for qemu-devel@nongnu.org; Tue, 25 Oct 2016 06:03:53 -0400 From: Markus Armbruster References: <1475246744-29302-1-git-send-email-berrange@redhat.com> <1475246744-29302-3-git-send-email-berrange@redhat.com> <87inspk9si.fsf@dusky.pond.sub.org> <20161020141134.GA27909@redhat.com> <871sza3twf.fsf@dusky.pond.sub.org> Date: Tue, 25 Oct 2016 12:03:33 +0200 In-Reply-To: (Max Reitz's message of "Fri, 21 Oct 2016 20:31:52 +0200") Message-ID: <871sz44ufe.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple method for un-flattening a dict List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: "Daniel P. Berrange" , Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= , qemu-block@nongnu.org, qemu-devel@nongnu.org Max Reitz writes: > On 21.10.2016 11:58, Markus Armbruster wrote: >> "Daniel P. Berrange" writes: >>=20 >>> On Tue, Oct 18, 2016 at 04:32:13PM +0200, Markus Armbruster wrote: >>>> "Daniel P. Berrange" writes: >>>> >>>>> The qdict_flatten() method will take a dict whose elements are >>>>> further nested dicts/lists and flatten them by concatenating >>>>> keys. >>>>> >>>>> The qdict_crumple() method aims to do the reverse, taking a flat >>>>> qdict, and turning it into a set of nested dicts/lists. It will >>>>> apply nesting based on the key name, with a '.' indicating a >>>>> new level in the hierarchy. If the keys in the nested structure >>>>> are all numeric, it will create a list, otherwise it will create >>>>> a dict. >>>>> >>>>> If the keys are a mixture of numeric and non-numeric, or the >>>>> numeric keys are not in strictly ascending order, an error will >>>>> be reported. >>>>> >>>>> As an example, a flat dict containing >>>>> >>>>> { >>>>> 'foo.0.bar': 'one', >>>>> 'foo.0.wizz': '1', >>>>> 'foo.1.bar': 'two', >>>>> 'foo.1.wizz': '2' >>>>> } >>>>> >>>>> will get turned into a dict with one element 'foo' whose >>>>> value is a list. The list elements will each in turn be >>>>> dicts. >>>>> >>>>> { >>>>> 'foo': [ >>>>> { 'bar': 'one', 'wizz': '1' }, >>>>> { 'bar': 'two', 'wizz': '2' } >>>>> ], >>>>> } >>>>> >>>>> If the key is intended to contain a literal '.', then it must >>>>> be escaped as '..'. ie a flat dict >>>>> >>>>> { >>>>> 'foo..bar': 'wizz', >>>>> 'bar.foo..bar': 'eek', >>>>> 'bar.hello': 'world' >>>>> } >>>>> >>>>> Will end up as >>>>> >>>>> { >>>>> 'foo.bar': 'wizz', >>>>> 'bar': { >>>>> 'foo.bar': 'eek', >>>>> 'hello': 'world' >>>>> } >>>>> } >>>>> >>>>> The intent of this function is that it allows a set of QemuOpts >>>>> to be turned into a nested data structure that mirrors the nesting >>>>> used when the same object is defined over QMP. >>>>> >>>>> Reviewed-by: Eric Blake >>>>> Reviewed-by: Kevin Wolf >>>>> Reviewed-by: Marc-Andr=C3=A9 Lureau >>>>> Signed-off-by: Daniel P. Berrange >>>>> --- >>>>> include/qapi/qmp/qdict.h | 1 + >>>>> qobject/qdict.c | 289 +++++++++++++++++++++++++++++++++++++= ++++++++++ >>>>> tests/check-qdict.c | 261 +++++++++++++++++++++++++++++++++++++= +++++ >>>>> 3 files changed, 551 insertions(+) >>>>> >>>>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h >>>>> index 71b8eb0..e0d24e1 100644 >>>>> --- a/include/qapi/qmp/qdict.h >>>>> +++ b/include/qapi/qmp/qdict.h >>>>> @@ -73,6 +73,7 @@ void qdict_flatten(QDict *qdict); >>>>> void qdict_extract_subqdict(QDict *src, QDict **dst, const char *sta= rt); >>>>> void qdict_array_split(QDict *src, QList **dst); >>>>> int qdict_array_entries(QDict *src, const char *subqdict); >>>>> +QObject *qdict_crumple(const QDict *src, bool recursive, Error **err= p); >>>>>=20=20 >>>>> void qdict_join(QDict *dest, QDict *src, bool overwrite); >>>>>=20=20 >>>>> diff --git a/qobject/qdict.c b/qobject/qdict.c >>>>> index 60f158c..c38e90e 100644 >>>>> --- a/qobject/qdict.c >>>>> +++ b/qobject/qdict.c >>>> [...] >>>>> +/** >>>>> + * qdict_crumple: >>>>> + * @src: the original flat dictionary (only scalar values) to crumple >>>>> + * @recursive: true to recursively crumple nested dictionaries >>>> >>>> Is recursive=3Dfalse used outside tests in this series? >>> >>> No, its not used. >>> >>> It was suggested in a way earlier version by Max, but not sure if his >>> code uses it or not. >>=20 >> In general, I prefer features to be added right before they're used, and >> I really dislike adding features "just in case". YAGNI. >>=20 >> Max, do you actually need this one? If yes, please explain your use >> case. > > As far as I can tell from a quick glance, I made the point for v1 that > qdict_crumple() could be simplified by using qdict_array_split() and > qdict_array_entries(). > > Dan then (correctly) said that using these functions would worsen > runtime performance of qdict_crumple() and that instead we can replace > qdict_array_split() by qdict_crumple(). However, for that to work, we > need to be able to make qdict_crumple() non-recursive (because > qdict_array_split() is non-recursive). > > Dan also said that in the long run we want to keep the tree structure in > the block layer instead of flattening everything down which would rid us > of several other QDict functions (and would make non-recursive behavior > obsolete again). I believe that this is an idea for the (far?) future, > as can be seen from the discussion you and others had on this very topic > in this version here. > > However, clearly there are no patches out yet that replace > qdict_array_split() by qdict_crumple(recursive=3Dfalse), and I certainly > won't write any until qdict_crumple() is merged. I prefer not to maintain code that isn't actually used. Since Dan is enjoying a few days off, and the soft freeze is closing in, I'm proposing to squash in the following: * Drop @recursive, in the most straightforward way possible. * Drop all tests that pass false to @recursive. This might reduce coverage, but we can fix that on top. * While there, collapse some vertical whitespace, for consistency with spacing elsewhere in the same files. Resulting fixup patch appended. I'd appreciate a quick look-over before I do a pull request. Current state at http://repo.or.cz/w/qemu/armbru.git branch qapi-not-next. >>From df87de10a12bd65d2ddc47514e951712de7c06f0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 25 Oct 2016 10:09:46 +0200 Subject: [PATCH] fixup! qdict: implement a qdict_crumple method for un-flattening a dict --- include/qapi/qmp/qdict.h | 2 +- qobject/qdict.c | 42 ++++++--------- tests/check-qdict.c | 133 +++----------------------------------------= ---- 3 files changed, 23 insertions(+), 154 deletions(-) diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h index e0d24e1..fe9a4c5 100644 --- a/include/qapi/qmp/qdict.h +++ b/include/qapi/qmp/qdict.h @@ -73,7 +73,7 @@ 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, bool recursive, Error **errp); +QObject *qdict_crumple(const QDict *src, Error **errp); =20 void qdict_join(QDict *dest, QDict *src, bool overwrite); =20 diff --git a/qobject/qdict.c b/qobject/qdict.c index c38e90e..197b0fb 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -684,7 +684,6 @@ void qdict_array_split(QDict *src, QList **dst) } } =20 - /** * qdict_split_flat_key: * @key: the key string to split @@ -744,7 +743,6 @@ static void qdict_split_flat_key(const char *key, char = **prefix, (*prefix)[j] =3D '\0'; } =20 - /** * qdict_is_list: * @maybe_list: dict to check if keys represent list elements. @@ -815,14 +813,10 @@ static int qdict_is_list(QDict *maybe_list, Error **e= rrp) /** * qdict_crumple: * @src: the original flat dictionary (only scalar values) to crumple - * @recursive: true to recursively crumple nested dictionaries * * Takes a flat dictionary whose keys use '.' separator to indicate * nesting, and values are scalars, and crumples it into a nested - * structure. If the @recursive parameter is false, then only the - * first level of structure implied by the keys will be crumpled. If - * @recursive is true, then the input will be recursively crumpled to - * expand all levels of structure in the keys. + * structure. * * To include a literal '.' in a key name, it must be escaped as '..' * @@ -855,7 +849,7 @@ static int qdict_is_list(QDict *maybe_list, Error **err= p) * Returns: either a QDict or QList for the nested data structure, or NULL * on error */ -QObject *qdict_crumple(const QDict *src, bool recursive, Error **errp) +QObject *qdict_crumple(const QDict *src, Error **errp) { const QDictEntry *ent; QDict *two_level, *multi_level =3D NULL; @@ -908,28 +902,23 @@ QObject *qdict_crumple(const QDict *src, bool recursi= ve, Error **errp) =20 /* Step 2: optionally process the two level dict recursively * into a multi-level dict */ - if (recursive) { - multi_level =3D qdict_new(); - for (ent =3D qdict_first(two_level); ent !=3D NULL; - ent =3D qdict_next(two_level, ent)) { + multi_level =3D qdict_new(); + for (ent =3D qdict_first(two_level); ent !=3D NULL; + ent =3D qdict_next(two_level, ent)) { =20 - if (qobject_type(ent->value) =3D=3D QTYPE_QDICT) { - child =3D qdict_crumple(qobject_to_qdict(ent->value), - recursive, errp); - if (!child) { - goto error; - } - - qdict_put_obj(multi_level, ent->key, child); - } else { - qobject_incref(ent->value); - qdict_put_obj(multi_level, ent->key, ent->value); + if (qobject_type(ent->value) =3D=3D QTYPE_QDICT) { + child =3D qdict_crumple(qobject_to_qdict(ent->value), errp); + if (!child) { + goto error; } + + qdict_put_obj(multi_level, ent->key, child); + } else { + qobject_incref(ent->value); + qdict_put_obj(multi_level, ent->key, ent->value); } - QDECREF(two_level); - } else { - multi_level =3D two_level; } + QDECREF(two_level); two_level =3D NULL; =20 /* Step 3: detect if we need to turn our dict into list */ @@ -971,7 +960,6 @@ QObject *qdict_crumple(const QDict *src, bool recursive= , Error **errp) return NULL; } =20 - /** * 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 diff --git a/tests/check-qdict.c b/tests/check-qdict.c index 64c33ab..07b1c79 100644 --- a/tests/check-qdict.c +++ b/tests/check-qdict.c @@ -413,7 +413,6 @@ static void qdict_array_split_test(void) =20 QDECREF(test_dict); =20 - /* * Test the split of * @@ -519,7 +518,6 @@ static void qdict_join_test(void) dict1 =3D qdict_new(); dict2 =3D qdict_new(); =20 - /* Test everything once without overwrite and once with */ do { @@ -529,7 +527,6 @@ static void qdict_join_test(void) g_assert(qdict_size(dict1) =3D=3D 0); g_assert(qdict_size(dict2) =3D=3D 0); =20 - /* First iteration: Test movement */ /* Second iteration: Test empty source and non-empty destination */ qdict_put(dict2, "foo", qint_from_int(42)); @@ -543,7 +540,6 @@ static void qdict_join_test(void) g_assert(qdict_get_int(dict1, "foo") =3D=3D 42); } =20 - /* Test non-empty source and destination without conflict */ qdict_put(dict2, "bar", qint_from_int(23)); =20 @@ -555,7 +551,6 @@ static void qdict_join_test(void) g_assert(qdict_get_int(dict1, "foo") =3D=3D 42); g_assert(qdict_get_int(dict1, "bar") =3D=3D 23); =20 - /* Test conflict */ qdict_put(dict2, "foo", qint_from_int(84)); =20 @@ -571,7 +566,6 @@ static void qdict_join_test(void) g_assert(qdict_get_int(dict2, "foo") =3D=3D 84); } =20 - /* Check the references */ g_assert(qdict_get(dict1, "foo")->refcnt =3D=3D 1); g_assert(qdict_get(dict1, "bar")->refcnt =3D=3D 1); @@ -580,7 +574,6 @@ static void qdict_join_test(void) g_assert(qdict_get(dict2, "foo")->refcnt =3D=3D 1); } =20 - /* Clean up */ qdict_del(dict1, "foo"); qdict_del(dict1, "bar"); @@ -591,113 +584,10 @@ static void qdict_join_test(void) } while (overwrite ^=3D true); =20 - QDECREF(dict1); QDECREF(dict2); } =20 - -static void qdict_crumple_test_nonrecursive(void) -{ - QDict *src, *dst, *rules, *vnc, *acl; - QObject *child, *res; - - src =3D qdict_new(); - qdict_put(src, "vnc.listen.addr", qstring_from_str("127.0.0.1")); - qdict_put(src, "vnc.listen.port", qstring_from_str("5901")); - qdict_put(src, "rule.0.match", qstring_from_str("fred")); - qdict_put(src, "rule.0.policy", qstring_from_str("allow")); - qdict_put(src, "rule.1.match", qstring_from_str("bob")); - qdict_put(src, "rule.1.policy", qstring_from_str("deny")); - qdict_put(src, "acl..name", qstring_from_str("acl0")); - qdict_put(src, "acl.rule..name", qstring_from_str("acl0")); - - res =3D qdict_crumple(src, false, &error_abort); - - g_assert_cmpint(qobject_type(res), =3D=3D, QTYPE_QDICT); - - dst =3D qobject_to_qdict(res); - - g_assert_cmpint(qdict_size(dst), =3D=3D, 4); - - child =3D qdict_get(dst, "vnc"); - g_assert_cmpint(qobject_type(child), =3D=3D, QTYPE_QDICT); - vnc =3D qdict_get_qdict(dst, "vnc"); - - g_assert_cmpint(qdict_size(vnc), =3D=3D, 2); - - g_assert_cmpstr("127.0.0.1", =3D=3D, qdict_get_str(vnc, "listen.addr")= ); - g_assert_cmpstr("5901", =3D=3D, qdict_get_str(vnc, "listen.port")); - - child =3D qdict_get(dst, "rule"); - g_assert_cmpint(qobject_type(child), =3D=3D, QTYPE_QDICT); - rules =3D qdict_get_qdict(dst, "rule"); - - g_assert_cmpint(qdict_size(rules), =3D=3D, 4); - - g_assert_cmpstr("fred", =3D=3D, qdict_get_str(rules, "0.match")); - g_assert_cmpstr("allow", =3D=3D, qdict_get_str(rules, "0.policy")); - g_assert_cmpstr("bob", =3D=3D, qdict_get_str(rules, "1.match")); - g_assert_cmpstr("deny", =3D=3D, qdict_get_str(rules, "1.policy")); - - /* With non-recursive crumpling, we should only see the first - * level names unescaped */ - g_assert_cmpstr("acl0", =3D=3D, qdict_get_str(dst, "acl.name")); - child =3D qdict_get(dst, "acl"); - g_assert_cmpint(qobject_type(child), =3D=3D, QTYPE_QDICT); - acl =3D qdict_get_qdict(dst, "acl"); - g_assert_cmpstr("acl0", =3D=3D, qdict_get_str(acl, "rule..name")); - - QDECREF(src); - QDECREF(dst); -} - - -static void qdict_crumple_test_listtop(void) -{ - QDict *src, *rule; - QList *rules; - QObject *res; - - src =3D qdict_new(); - qdict_put(src, "0.match.name", qstring_from_str("Fred")); - qdict_put(src, "0.match.email", qstring_from_str("fred@example.com")); - qdict_put(src, "0.policy", qstring_from_str("allow")); - qdict_put(src, "1.match.name", qstring_from_str("Bob")); - qdict_put(src, "1.match.email", qstring_from_str("bob@example.com")); - qdict_put(src, "1.policy", qstring_from_str("deny")); - - res =3D qdict_crumple(src, false, &error_abort); - - g_assert_cmpint(qobject_type(res), =3D=3D, QTYPE_QLIST); - - rules =3D qobject_to_qlist(res); - - g_assert_cmpint(qlist_size(rules), =3D=3D, 2); - - g_assert_cmpint(qobject_type(qlist_peek(rules)), =3D=3D, QTYPE_QDICT); - rule =3D qobject_to_qdict(qlist_pop(rules)); - g_assert_cmpint(qdict_size(rule), =3D=3D, 3); - - g_assert_cmpstr("Fred", =3D=3D, qdict_get_str(rule, "match.name")); - g_assert_cmpstr("fred@example.com", =3D=3D, qdict_get_str(rule, "match= .email")); - g_assert_cmpstr("allow", =3D=3D, qdict_get_str(rule, "policy")); - QDECREF(rule); - - g_assert_cmpint(qobject_type(qlist_peek(rules)), =3D=3D, QTYPE_QDICT); - rule =3D qobject_to_qdict(qlist_pop(rules)); - g_assert_cmpint(qdict_size(rule), =3D=3D, 3); - - g_assert_cmpstr("Bob", =3D=3D, qdict_get_str(rule, "match.name")); - g_assert_cmpstr("bob@example.com", =3D=3D, qdict_get_str(rule, "match.= email")); - g_assert_cmpstr("deny", =3D=3D, qdict_get_str(rule, "policy")); - QDECREF(rule); - - QDECREF(src); - qobject_decref(res); -} - - static void qdict_crumple_test_recursive(void) { QDict *src, *dst, *rule, *vnc, *acl, *listen; @@ -715,7 +605,7 @@ static void qdict_crumple_test_recursive(void) qdict_put(src, "vnc.acl..name", qstring_from_str("acl0")); qdict_put(src, "vnc.acl.rule..name", qstring_from_str("acl0")); =20 - res =3D qdict_crumple(src, true, &error_abort); + res =3D qdict_crumple(src, &error_abort); =20 g_assert_cmpint(qobject_type(res), =3D=3D, QTYPE_QDICT); =20 @@ -765,14 +655,13 @@ static void qdict_crumple_test_recursive(void) QDECREF(dst); } =20 - static void qdict_crumple_test_empty(void) { QDict *src, *dst; =20 src =3D qdict_new(); =20 - dst =3D (QDict *)qdict_crumple(src, true, &error_abort); + dst =3D (QDict *)qdict_crumple(src, &error_abort); =20 g_assert_cmpint(qdict_size(dst), =3D=3D, 0); =20 @@ -780,7 +669,6 @@ static void qdict_crumple_test_empty(void) QDECREF(dst); } =20 - static void qdict_crumple_test_bad_inputs(void) { QDict *src; @@ -791,7 +679,7 @@ static void qdict_crumple_test_bad_inputs(void) qdict_put(src, "rule.0", qstring_from_str("fred")); qdict_put(src, "rule.0.policy", qstring_from_str("allow")); =20 - g_assert(qdict_crumple(src, true, &error) =3D=3D NULL); + g_assert(qdict_crumple(src, &error) =3D=3D NULL); g_assert(error !=3D NULL); error_free(error); error =3D NULL; @@ -802,7 +690,7 @@ static void qdict_crumple_test_bad_inputs(void) qdict_put(src, "rule.0", qstring_from_str("fred")); qdict_put(src, "rule.a", qstring_from_str("allow")); =20 - g_assert(qdict_crumple(src, true, &error) =3D=3D NULL); + g_assert(qdict_crumple(src, &error) =3D=3D NULL); g_assert(error !=3D NULL); error_free(error); error =3D NULL; @@ -813,38 +701,35 @@ static void qdict_crumple_test_bad_inputs(void) qdict_put(src, "rule.a", qdict_new()); qdict_put(src, "rule.b", qstring_from_str("allow")); =20 - g_assert(qdict_crumple(src, true, &error) =3D=3D NULL); + g_assert(qdict_crumple(src, &error) =3D=3D NULL); g_assert(error !=3D NULL); error_free(error); error =3D NULL; QDECREF(src); =20 - src =3D qdict_new(); /* List indexes must not have gaps */ qdict_put(src, "rule.0", qstring_from_str("deny")); qdict_put(src, "rule.3", qstring_from_str("allow")); =20 - g_assert(qdict_crumple(src, true, &error) =3D=3D NULL); + g_assert(qdict_crumple(src, &error) =3D=3D NULL); g_assert(error !=3D NULL); error_free(error); error =3D NULL; QDECREF(src); =20 - src =3D qdict_new(); /* List indexes must be in %zu format */ qdict_put(src, "rule.0", qstring_from_str("deny")); qdict_put(src, "rule.+1", qstring_from_str("allow")); =20 - g_assert(qdict_crumple(src, true, &error) =3D=3D NULL); + g_assert(qdict_crumple(src, &error) =3D=3D NULL); g_assert(error !=3D NULL); error_free(error); error =3D NULL; QDECREF(src); } =20 - /* * Errors test-cases */ @@ -992,12 +877,8 @@ int main(int argc, char **argv) g_test_add_func("/errors/put_exists", qdict_put_exists_test); g_test_add_func("/errors/get_not_exists", qdict_get_not_exists_test); =20 - g_test_add_func("/public/crumple/nonrecursive", - qdict_crumple_test_nonrecursive); g_test_add_func("/public/crumple/recursive", qdict_crumple_test_recursive); - g_test_add_func("/public/crumple/listtop", - qdict_crumple_test_listtop); g_test_add_func("/public/crumple/empty", qdict_crumple_test_empty); g_test_add_func("/public/crumple/bad_inputs", --=20 2.5.5