[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple method for un-flattening a dict |
Date: |
Tue, 25 Oct 2016 14:29:33 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
"Daniel P. Berrange" <address@hidden> writes:
> On Tue, Oct 25, 2016 at 12:03:33PM +0200, Markus Armbruster wrote:
>> Max Reitz <address@hidden> writes:
>>
>> > On 21.10.2016 11:58, Markus Armbruster wrote:
>> >> "Daniel P. Berrange" <address@hidden> writes:
>> >>
>> >>> On Tue, Oct 18, 2016 at 04:32:13PM +0200, Markus Armbruster wrote:
>> >>>> "Daniel P. Berrange" <address@hidden> 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 <address@hidden>
>> >>>>> Reviewed-by: Kevin Wolf <address@hidden>
>> >>>>> Reviewed-by: Marc-André Lureau <address@hidden>
>> >>>>> Signed-off-by: Daniel P. Berrange <address@hidden>
>> >>>>> ---
>> >>>>> 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);
>> >>>>>
>> >>>>> void qdict_join(QDict *dest, QDict *src, bool overwrite);
>> >>>>>
>> >>>>> 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 crumple
>> >>>>> + * @recursive: true to recursively crumple nested dictionaries
>> >>>>
>> >>>> Is recursive=false 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.
>> >>
>> >> In general, I prefer features to be added right before they're used, and
>> >> I really dislike adding features "just in case". YAGNI.
>> >>
>> >> 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 topic
>> > in this version here.
>> >
>> > However, clearly there are no patches out yet that replace
>> > qdict_array_split() by qdict_crumple(recursive=false), and I certainly
>> > won't write any until qdict_crumple() is merged.
>>
>> 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:
>>
>> * Drop @recursive, in the most straightforward way possible.
>>
>> * Drop all tests that pass false to @recursive. This might reduce
>> coverage, but we can fix that on top.
>>
>> * While there, collapse some vertical whitespace, for consistency with
>> spacing elsewhere in the same files.
>>
>> 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!