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; } 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?