qemu-block
[Top][All Lists]
Advanced

[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!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]