Kevin Wolf <kw...@redhat.com> writes: > 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
I feel dumb. Next try: if (child) { /* * If @child_dict, then all previous keys with this prefix * had a suffix. If @suffix, this one has one as well, * and we're good, else there's a clash. */ if (!child_dict || !suffix) { 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 { qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); }