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

[Kevin Wolf <kwolf@redhat.com>]

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