Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit

[Kevin Wolf <kwolf@redhat.com>]

Am 14.06.2018 um 13:52 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@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 <armbru@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