Am 14.06.2018 um 13:52 hat Markus Armbruster geschrieben: > 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)); > }
This one looks good to me. (Though I feel your comments explains the !(child_dict && suffix) form that I chose rather than what you actually have in the code. But I don't mind.) Kevin