From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43966) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz0rQ-00059x-LE for qemu-devel@nongnu.org; Tue, 25 Oct 2016 08:29:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz0rP-00084f-53 for qemu-devel@nongnu.org; Tue, 25 Oct 2016 08:29:44 -0400 From: Markus Armbruster References: <1475246744-29302-1-git-send-email-berrange@redhat.com> <1475246744-29302-3-git-send-email-berrange@redhat.com> <87inspk9si.fsf@dusky.pond.sub.org> <20161020141134.GA27909@redhat.com> <871sza3twf.fsf@dusky.pond.sub.org> <871sz44ufe.fsf@dusky.pond.sub.org> <20161025102049.GB3449@redhat.com> Date: Tue, 25 Oct 2016 14:29:33 +0200 In-Reply-To: <20161025102049.GB3449@redhat.com> (Daniel P. Berrange's message of "Tue, 25 Oct 2016 12:20:49 +0200") Message-ID: <87h9801uj6.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple method for un-flattening a dict List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org, Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= , qemu-block@nongnu.org, Max Reitz "Daniel P. Berrange" writes: > On Tue, Oct 25, 2016 at 12:03:33PM +0200, Markus Armbruster wrote: >> Max Reitz writes: >>=20 >> > On 21.10.2016 11:58, Markus Armbruster wrote: >> >> "Daniel P. Berrange" writes: >> >>=20 >> >>> On Tue, Oct 18, 2016 at 04:32:13PM +0200, Markus Armbruster wrote: >> >>>> "Daniel P. Berrange" writes: >> >>>> >> >>>>> The qdict_flatten() method will take a dict whose elements are >> >>>>> further nested dicts/lists and flatten them by concatenating >> >>>>> keys. >> >>>>> >> >>>>> The qdict_crumple() method aims to do the reverse, taking a flat >> >>>>> qdict, and turning it into a set of nested dicts/lists. It will >> >>>>> apply nesting based on the key name, with a '.' indicating a >> >>>>> new level in the hierarchy. If the keys in the nested structure >> >>>>> are all numeric, it will create a list, otherwise it will create >> >>>>> a dict. >> >>>>> >> >>>>> If the keys are a mixture of numeric and non-numeric, or the >> >>>>> numeric keys are not in strictly ascending order, an error will >> >>>>> be reported. >> >>>>> >> >>>>> As an example, a flat dict containing >> >>>>> >> >>>>> { >> >>>>> 'foo.0.bar': 'one', >> >>>>> 'foo.0.wizz': '1', >> >>>>> 'foo.1.bar': 'two', >> >>>>> 'foo.1.wizz': '2' >> >>>>> } >> >>>>> >> >>>>> will get turned into a dict with one element 'foo' whose >> >>>>> value is a list. The list elements will each in turn be >> >>>>> dicts. >> >>>>> >> >>>>> { >> >>>>> 'foo': [ >> >>>>> { 'bar': 'one', 'wizz': '1' }, >> >>>>> { 'bar': 'two', 'wizz': '2' } >> >>>>> ], >> >>>>> } >> >>>>> >> >>>>> If the key is intended to contain a literal '.', then it must >> >>>>> be escaped as '..'. ie a flat dict >> >>>>> >> >>>>> { >> >>>>> 'foo..bar': 'wizz', >> >>>>> 'bar.foo..bar': 'eek', >> >>>>> 'bar.hello': 'world' >> >>>>> } >> >>>>> >> >>>>> Will end up as >> >>>>> >> >>>>> { >> >>>>> 'foo.bar': 'wizz', >> >>>>> 'bar': { >> >>>>> 'foo.bar': 'eek', >> >>>>> 'hello': 'world' >> >>>>> } >> >>>>> } >> >>>>> >> >>>>> The intent of this function is that it allows a set of QemuOpts >> >>>>> to be turned into a nested data structure that mirrors the nesting >> >>>>> used when the same object is defined over QMP. >> >>>>> >> >>>>> Reviewed-by: Eric Blake >> >>>>> Reviewed-by: Kevin Wolf >> >>>>> Reviewed-by: Marc-Andr=C3=A9 Lureau >> >>>>> Signed-off-by: Daniel P. Berrange >> >>>>> --- >> >>>>> include/qapi/qmp/qdict.h | 1 + >> >>>>> qobject/qdict.c | 289 ++++++++++++++++++++++++++++++++++= +++++++++++++ >> >>>>> tests/check-qdict.c | 261 ++++++++++++++++++++++++++++++++++= ++++++++ >> >>>>> 3 files changed, 551 insertions(+) >> >>>>> >> >>>>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h >> >>>>> index 71b8eb0..e0d24e1 100644 >> >>>>> --- a/include/qapi/qmp/qdict.h >> >>>>> +++ b/include/qapi/qmp/qdict.h >> >>>>> @@ -73,6 +73,7 @@ void qdict_flatten(QDict *qdict); >> >>>>> void qdict_extract_subqdict(QDict *src, QDict **dst, const char *= start); >> >>>>> void qdict_array_split(QDict *src, QList **dst); >> >>>>> int qdict_array_entries(QDict *src, const char *subqdict); >> >>>>> +QObject *qdict_crumple(const QDict *src, bool recursive, Error **= errp); >> >>>>>=20=20 >> >>>>> void qdict_join(QDict *dest, QDict *src, bool overwrite); >> >>>>>=20=20 >> >>>>> diff --git a/qobject/qdict.c b/qobject/qdict.c >> >>>>> index 60f158c..c38e90e 100644 >> >>>>> --- a/qobject/qdict.c >> >>>>> +++ b/qobject/qdict.c >> >>>> [...] >> >>>>> +/** >> >>>>> + * qdict_crumple: >> >>>>> + * @src: the original flat dictionary (only scalar values) to cru= mple >> >>>>> + * @recursive: true to recursively crumple nested dictionaries >> >>>> >> >>>> Is recursive=3Dfalse used outside tests in this series? >> >>> >> >>> No, its not used. >> >>> >> >>> It was suggested in a way earlier version by Max, but not sure if his >> >>> code uses it or not. >> >>=20 >> >> In general, I prefer features to be added right before they're used, = and >> >> I really dislike adding features "just in case". YAGNI. >> >>=20 >> >> Max, do you actually need this one? If yes, please explain your use >> >> case. >> > >> > As far as I can tell from a quick glance, I made the point for v1 that >> > qdict_crumple() could be simplified by using qdict_array_split() and >> > qdict_array_entries(). >> > >> > Dan then (correctly) said that using these functions would worsen >> > runtime performance of qdict_crumple() and that instead we can replace >> > qdict_array_split() by qdict_crumple(). However, for that to work, we >> > need to be able to make qdict_crumple() non-recursive (because >> > qdict_array_split() is non-recursive). >> > >> > Dan also said that in the long run we want to keep the tree structure = in >> > the block layer instead of flattening everything down which would rid = us >> > of several other QDict functions (and would make non-recursive behavior >> > obsolete again). I believe that this is an idea for the (far?) future, >> > as can be seen from the discussion you and others had on this very top= ic >> > in this version here. >> > >> > However, clearly there are no patches out yet that replace >> > qdict_array_split() by qdict_crumple(recursive=3Dfalse), and I certain= ly >> > won't write any until qdict_crumple() is merged. >>=20 >> I prefer not to maintain code that isn't actually used. Since Dan is >> enjoying a few days off, and the soft freeze is closing in, I'm >> proposing to squash in the following: >>=20 >> * Drop @recursive, in the most straightforward way possible. >>=20 >> * Drop all tests that pass false to @recursive. This might reduce >> coverage, but we can fix that on top. >>=20 >> * While there, collapse some vertical whitespace, for consistency with >> spacing elsewhere in the same files. >>=20 >> Resulting fixup patch appended. I'd appreciate a quick look-over before >> I do a pull request. Current state at >> http://repo.or.cz/w/qemu/armbru.git branch qapi-not-next. > > Had a quick look, and it seems fine to me. Applied to qapi-next, thanks!