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 <kw...@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 <arm...@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 :|