[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit |
Date: |
Wed, 13 Jun 2018 17:23:59 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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;
}
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);
qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
}
Okay?
- [Qemu-block] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename", (continued)
- [Qemu-block] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename", Markus Armbruster, 2018/06/12
- [Qemu-block] [PATCH 06/18] block: Fix -drive for certain non-string scalars, Markus Armbruster, 2018/06/12
- [Qemu-block] [PATCH 14/18] check-block-qdict: Rename qdict_flatten()'s variables for clarity, Markus Armbruster, 2018/06/12
- [Qemu-block] [PATCH 09/18] block: Make remaining uses of qobject input visitor more robust, Markus Armbruster, 2018/06/12
- [Qemu-block] [PATCH 13/18] block-qdict: Simplify qdict_is_list() some, Markus Armbruster, 2018/06/12
- [Qemu-block] [PATCH 15/18] check-block-qdict: Cover flattening of empty lists and dictionaries, Markus Armbruster, 2018/06/12
- [Qemu-block] [PATCH 17/18] rbd: New parameter auth-client-required, Markus Armbruster, 2018/06/12
- [Qemu-block] [PATCH 07/18] block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts(), Markus Armbruster, 2018/06/12
- [Qemu-block] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit, Markus Armbruster, 2018/06/12
[Qemu-block] [PATCH 10/18] block-qdict: Simplify qdict_flatten_qdict(), Markus Armbruster, 2018/06/12
[Qemu-block] [PATCH 03/18] block: Add block-specific QDict header, Markus Armbruster, 2018/06/12
[Qemu-block] [PATCH 18/18] rbd: New parameter key-secret, Markus Armbruster, 2018/06/12