From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33434) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fT7d2-0000uz-2k for qemu-devel@nongnu.org; Wed, 13 Jun 2018 11:24:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fT7d0-0007o6-QK for qemu-devel@nongnu.org; Wed, 13 Jun 2018 11:24:08 -0400 From: Markus Armbruster References: <20180612125821.4229-1-armbru@redhat.com> <20180612125821.4229-13-armbru@redhat.com> <20180612153909.GG4355@localhost.localdomain> Date: Wed, 13 Jun 2018 17:23:59 +0200 In-Reply-To: <20180612153909.GG4355@localhost.localdomain> (Kevin Wolf's message of "Tue, 12 Jun 2018 17:39:09 +0200") Message-ID: <87efha6774.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Markus Armbruster , jcody@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com Kevin Wolf 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 > > 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; } 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?