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. > qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > } > > Okay? Kevin