[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict
|
From: |
Markus Armbruster |
|
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple method for un-flattening a dict |
|
Date: |
Mon, 24 Oct 2016 14:38:52 +0200 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
"Daniel P. Berrange" <address@hidden> writes:
> On Mon, Oct 24, 2016 at 11:18:29AM +0200, Markus Armbruster wrote:
>> Max Reitz <address@hidden> writes:
>>
>> > On 21.10.2016 11:58, Markus Armbruster wrote:
>> >> "Daniel P. Berrange" <address@hidden> writes:
>> >>
>> >>> On Tue, Oct 18, 2016 at 04:32:13PM +0200, Markus Armbruster wrote:
>> >>>> "Daniel P. Berrange" <address@hidden> 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 <address@hidden>
>> >>>>> Reviewed-by: Kevin Wolf <address@hidden>
>> >>>>> Reviewed-by: Marc-André Lureau <address@hidden>
>> >>>>> Signed-off-by: Daniel P. Berrange <address@hidden>
>> >>>>> ---
>> >>>>> 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
>> >>>>> *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);
>> >>>>>
>> >>>>> void qdict_join(QDict *dest, QDict *src, bool overwrite);
>> >>>>>
>> >>>>> 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=false 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.
>> >>
>> >> In general, I prefer features to be added right before they're used, and
>> >> I really dislike adding features "just in case". YAGNI.
>> >>
>> >> 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.
>>
>> In the long run, the block layer should use proper C types instead of
>> mucking around with QDict. That's what QAPI is for.
>>
>> > However, clearly there are no patches out yet that replace
>> > qdict_array_split() by qdict_crumple(recursive=false), and I certainly
>> > won't write any until qdict_crumple() is merged.
>>
>> All right, I'll go hunting for prior versions of qdict_crumple() to see
>> whether I find a suitable one without @recursive.
>
> Please don't do that - there have been a tonne of other fixes and
> improvements since the version that introduced @recursive. We need
> to just chop out that parameter from the v15 version instead.
I realized that once I looked at the interdiffs.
Chopping off @recursive is easy enough. So is dropping all tests that
pass false, but I'm not sure what that does to test coverage. Any
advice?
I'm sorry I didn't catch this in my prior reviews (v5+v8).
diff --git a/hmp.c b/hmp.c
index b32d8c8..3845d1b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1700,7 +1700,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
Object *obj = NULL;
QObject *pobj;
- pobj = qdict_crumple(qdict, true, &err);
+ pobj = qdict_crumple(qdict, &err);
if (err) {
goto cleanup;
}
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);
void qdict_join(QDict *dest, QDict *src, bool overwrite);
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f1030d5..c743a5b 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -769,7 +769,7 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts
*opts,
goto cleanup;
}
- pobj = qdict_crumple(pdict, true, errp);
+ pobj = qdict_crumple(pdict, errp);
if (!pobj) {
goto cleanup;
}
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 83384b2..bc970bf 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -815,15 +815,11 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
/**
* qdict_crumple:
* @src: the original dictionary to crumple
- * @recursive: true to recursively crumple nested dictionaries
*
* Takes a dictionary whose keys use '.' separator to indicate
* nesting, crumples it into a nested structure. The values in
- * @src are permitted to be scalars or compound typee. 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.
+ * @src are permitted to be scalars or compound typee. It will be
+ * recursively crumpled to expand all levels of structure in the keys.
*
* To include a literal '.' in a key name, it must be escaped as '..'
*
@@ -859,7 +855,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
* 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 = NULL;
@@ -917,29 +913,24 @@ QObject *qdict_crumple(const QDict *src, bool recursive,
Error **errp)
/* Step 2: optionally process the two level dict recursively
* into a multi-level dict */
- if (recursive) {
- multi_level = qdict_new();
- for (ent = qdict_first(two_level); ent != NULL;
- ent = qdict_next(two_level, ent)) {
+ multi_level = qdict_new();
+ for (ent = qdict_first(two_level); ent != NULL;
+ ent = qdict_next(two_level, ent)) {
- if (qobject_type(ent->value) == QTYPE_QDICT &&
- !qdict_haskey(compound, ent->key)) {
- child = 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) == QTYPE_QDICT &&
+ !qdict_haskey(compound, ent->key)) {
+ child = 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 = two_level;
}
+ QDECREF(two_level);
two_level = NULL;
/* Step 3: detect if we need to turn our dict into list */
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 88a1e88..0ea22fd 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -170,7 +170,7 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error
**errp)
QEMU_OPTS_REPEAT_POLICY_ALL,
&error_abort);
- pobj = qdict_crumple(pdict, true, errp);
+ pobj = qdict_crumple(pdict, errp);
if (!pobj) {
goto cleanup;
}
diff --git a/tests/check-qdict.c b/tests/check-qdict.c
index 29d7362..0d1f996 100644
--- a/tests/check-qdict.c
+++ b/tests/check-qdict.c
@@ -596,108 +596,6 @@ static void qdict_join_test(void)
QDECREF(dict2);
}
-
-static void qdict_crumple_test_nonrecursive(void)
-{
- QDict *src, *dst, *rules, *vnc, *acl;
- QObject *child, *res;
-
- src = 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 = qdict_crumple(src, false, &error_abort);
-
- g_assert_cmpint(qobject_type(res), ==, QTYPE_QDICT);
-
- dst = qobject_to_qdict(res);
-
- g_assert_cmpint(qdict_size(dst), ==, 4);
-
- child = qdict_get(dst, "vnc");
- g_assert_cmpint(qobject_type(child), ==, QTYPE_QDICT);
- vnc = qdict_get_qdict(dst, "vnc");
-
- g_assert_cmpint(qdict_size(vnc), ==, 2);
-
- g_assert_cmpstr("127.0.0.1", ==, qdict_get_str(vnc, "listen.addr"));
- g_assert_cmpstr("5901", ==, qdict_get_str(vnc, "listen.port"));
-
- child = qdict_get(dst, "rule");
- g_assert_cmpint(qobject_type(child), ==, QTYPE_QDICT);
- rules = qdict_get_qdict(dst, "rule");
-
- g_assert_cmpint(qdict_size(rules), ==, 4);
-
- g_assert_cmpstr("fred", ==, qdict_get_str(rules, "0.match"));
- g_assert_cmpstr("allow", ==, qdict_get_str(rules, "0.policy"));
- g_assert_cmpstr("bob", ==, qdict_get_str(rules, "1.match"));
- g_assert_cmpstr("deny", ==, qdict_get_str(rules, "1.policy"));
-
- /* With non-recursive crumpling, we should only see the first
- * level names unescaped */
- g_assert_cmpstr("acl0", ==, qdict_get_str(dst, "acl.name"));
- child = qdict_get(dst, "acl");
- g_assert_cmpint(qobject_type(child), ==, QTYPE_QDICT);
- acl = qdict_get_qdict(dst, "acl");
- g_assert_cmpstr("acl0", ==, 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 = qdict_new();
- qdict_put(src, "0.match.name", qstring_from_str("Fred"));
- qdict_put(src, "0.match.email", qstring_from_str("address@hidden"));
- 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("address@hidden"));
- qdict_put(src, "1.policy", qstring_from_str("deny"));
-
- res = qdict_crumple(src, false, &error_abort);
-
- g_assert_cmpint(qobject_type(res), ==, QTYPE_QLIST);
-
- rules = qobject_to_qlist(res);
-
- g_assert_cmpint(qlist_size(rules), ==, 2);
-
- g_assert_cmpint(qobject_type(qlist_peek(rules)), ==, QTYPE_QDICT);
- rule = qobject_to_qdict(qlist_pop(rules));
- g_assert_cmpint(qdict_size(rule), ==, 3);
-
- g_assert_cmpstr("Fred", ==, qdict_get_str(rule, "match.name"));
- g_assert_cmpstr("address@hidden", ==, qdict_get_str(rule, "match.email"));
- g_assert_cmpstr("allow", ==, qdict_get_str(rule, "policy"));
- QDECREF(rule);
-
- g_assert_cmpint(qobject_type(qlist_peek(rules)), ==, QTYPE_QDICT);
- rule = qobject_to_qdict(qlist_pop(rules));
- g_assert_cmpint(qdict_size(rule), ==, 3);
-
- g_assert_cmpstr("Bob", ==, qdict_get_str(rule, "match.name"));
- g_assert_cmpstr("address@hidden", ==, qdict_get_str(rule, "match.email"));
- g_assert_cmpstr("deny", ==, 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 +613,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"));
- res = qdict_crumple(src, true, &error_abort);
+ res = qdict_crumple(src, &error_abort);
g_assert_cmpint(qobject_type(res), ==, QTYPE_QDICT);
@@ -765,14 +663,13 @@ static void qdict_crumple_test_recursive(void)
QDECREF(dst);
}
-
static void qdict_crumple_test_empty(void)
{
QDict *src, *dst;
src = qdict_new();
- dst = (QDict *)qdict_crumple(src, true, &error_abort);
+ dst = (QDict *)qdict_crumple(src, &error_abort);
g_assert_cmpint(qdict_size(dst), ==, 0);
@@ -780,7 +677,6 @@ static void qdict_crumple_test_empty(void)
QDECREF(dst);
}
-
static void qdict_crumple_test_bad_inputs(void)
{
QDict *src;
@@ -791,7 +687,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"));
- g_assert(qdict_crumple(src, true, &error) == NULL);
+ g_assert(qdict_crumple(src, &error) == NULL);
g_assert(error != NULL);
error_free(error);
error = NULL;
@@ -802,7 +698,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"));
- g_assert(qdict_crumple(src, true, &error) == NULL);
+ g_assert(qdict_crumple(src, &error) == NULL);
g_assert(error != NULL);
error_free(error);
error = NULL;
@@ -813,7 +709,7 @@ static void qdict_crumple_test_bad_inputs(void)
qdict_put(src, "rule.a", qdict_new());
qdict_put(src, "rule.b", qstring_from_str("allow"));
- g_assert(qdict_crumple(src, true, &error) == NULL);
+ g_assert(qdict_crumple(src, &error) == NULL);
g_assert(error != NULL);
error_free(error);
error = NULL;
@@ -825,7 +721,7 @@ static void qdict_crumple_test_bad_inputs(void)
qdict_put(src, "rule.0", qstring_from_str("deny"));
qdict_put(src, "rule.3", qstring_from_str("allow"));
- g_assert(qdict_crumple(src, true, &error) == NULL);
+ g_assert(qdict_crumple(src, &error) == NULL);
g_assert(error != NULL);
error_free(error);
error = NULL;
@@ -837,7 +733,7 @@ static void qdict_crumple_test_bad_inputs(void)
qdict_put(src, "rule.0", qstring_from_str("deny"));
qdict_put(src, "rule.+1", qstring_from_str("allow"));
- g_assert(qdict_crumple(src, true, &error) == NULL);
+ g_assert(qdict_crumple(src, &error) == NULL);
g_assert(error != NULL);
error_free(error);
error = NULL;
@@ -860,7 +756,7 @@ static void qdict_crumple_test_non_flat_list_ok(void)
qdict_put(src, "vnc.listen.port", qstring_from_str("5901"));
qdict_put(src, "match", list);
- res = qdict_crumple(src, true, &error_abort);
+ res = qdict_crumple(src, &error_abort);
g_assert_cmpint(qobject_type(res), ==, QTYPE_QDICT);
@@ -903,32 +799,6 @@ static void qdict_crumple_test_non_flat_list_ok(void)
QDECREF(dst);
}
-
-static void qdict_crumple_test_non_flat_list_bad(void)
-{
- QDict *src;
- QObject *res;
- QList *list;
- Error *err = NULL;
-
- src = qdict_new();
- list = qlist_new();
- qlist_append(list, qstring_from_str("fred"));
- qlist_append(list, qstring_from_str("bob"));
-
- 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, "match.0", qstring_from_str("frank"));
- qdict_put(src, "match", list);
-
- res = qdict_crumple(src, false, &err);
- g_assert(!res);
- error_free_or_abort(&err);
-
- QDECREF(src);
-}
-
-
static void qdict_crumple_test_non_flat_dict_ok(void)
{
QDict *src, *dst, *rule, *vnc, *acl;
@@ -946,7 +816,7 @@ static void qdict_crumple_test_non_flat_dict_ok(void)
qdict_put(src, "acl.rules.1.match", qstring_from_str("bob"));
qdict_put(src, "acl.rules.1.policy", qstring_from_str("deny"));
- res = qdict_crumple(src, true, &error_abort);
+ res = qdict_crumple(src, &error_abort);
g_assert_cmpint(qobject_type(res), ==, QTYPE_QDICT);
@@ -986,7 +856,6 @@ static void qdict_crumple_test_non_flat_dict_ok(void)
QDECREF(dst);
}
-
static void qdict_crumple_test_non_flat_dict_bad(void)
{
QDict *src, *vnc;
@@ -1004,14 +873,13 @@ static void qdict_crumple_test_non_flat_dict_bad(void)
qdict_put(src, "acl.rules.1.match", qstring_from_str("bob"));
qdict_put(src, "acl.rules.1.policy", qstring_from_str("deny"));
- res = qdict_crumple(src, true, &err);
+ res = qdict_crumple(src, &err);
g_assert(!res);
error_free_or_abort(&err);
QDECREF(src);
}
-
/*
* Errors test-cases
*/
@@ -1159,20 +1027,14 @@ 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);
- 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",
qdict_crumple_test_bad_inputs);
g_test_add_func("/public/crumple/non-flat-list-ok",
qdict_crumple_test_non_flat_list_ok);
- g_test_add_func("/public/crumple/non-flat-list-bad",
- qdict_crumple_test_non_flat_list_bad);
g_test_add_func("/public/crumple/non-flat-dict-ok",
qdict_crumple_test_non_flat_dict_ok);
g_test_add_func("/public/crumple/non-flat-dict-bad",