[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a b
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit |
Date: |
Thu, 14 Jun 2018 15:11:18 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Daniel P. Berrangé <address@hidden> writes:
> On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote:
>> 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.
>
> We got coverage of bad inputs in tests/check-qdict.c
>
> If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios
> we should extend it to make sure we detect them during testing
Depends on qdict_first() / qdict_next() traversal order, which makes it
awkward.
- [Qemu-devel] [PATCH 00/18] block: Configuration fixes and rbd authentication, Markus Armbruster, 2018/06/12
- [Qemu-devel] [PATCH 10/18] block-qdict: Simplify qdict_flatten_qdict(), Markus Armbruster, 2018/06/12
- [Qemu-devel] [PATCH 01/18] rbd: Drop deprecated -drive parameter "filename", Markus Armbruster, 2018/06/12
- [Qemu-devel] [PATCH 15/18] check-block-qdict: Cover flattening of empty lists and dictionaries, Markus Armbruster, 2018/06/12
- [Qemu-devel] [PATCH 11/18] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist(), Markus Armbruster, 2018/06/12
- [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit, Markus Armbruster, 2018/06/12
[Qemu-devel] [PATCH 02/18] iscsi: Drop deprecated -drive parameter "filename", Markus Armbruster, 2018/06/12
[Qemu-devel] [PATCH 06/18] block: Fix -drive for certain non-string scalars, Markus Armbruster, 2018/06/12
[Qemu-devel] [PATCH 14/18] check-block-qdict: Rename qdict_flatten()'s variables for clarity, Markus Armbruster, 2018/06/12
[Qemu-devel] [PATCH 03/18] block: Add block-specific QDict header, Markus Armbruster, 2018/06/12
[Qemu-devel] [PATCH 05/18] block: Fix -blockdev for certain non-string scalars, Markus Armbruster, 2018/06/12
[Qemu-devel] [PATCH 13/18] block-qdict: Simplify qdict_is_list() some, Markus Armbruster, 2018/06/12