Re: [Qemu-block] [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple method for un-flattening a dict

[Max Reitz]

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.

Max

signature.asc
Description: OpenPGP digital signature