[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple meth
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple method for un-flattening a dict |
Date: |
Tue, 25 Oct 2016 12:20:49 +0200 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|