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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> 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 <address@hidden>
>> >
>> > 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 @address@hidden, 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));
        }