Re: [Qemu-devel] [PATCH v3 01/10] qdict: implement a qdict_crumple method for un-flattening a dict

[Daniel P. Berrange]

On Mon, Mar 21, 2016 at 04:45:39PM -0600, Eric Blake wrote:
> On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:

> > +    /* Unescape the '..' sequence into '.' */
> > +    for (i = 0, j = 0; (*prefix)[i] != '\0'; i++, j++) {
> > +        if ((*prefix)[i] == '.' &&
> > +            (*prefix)[i + 1] == '.') {
> 
> Technically, if (*prefix)[i] == '.', we could assert((*prefix)[i + 1] ==
> '.'), since the only way to get a '.' in prefix is via escaping.  For
> that matter, you could short-circuit (part of) the loop by doing a
> strchr for '.' (if not found, the loop is not needed; if found, start
> the reduction at that point rather on the bytes leading up to that point).

I'm not seeing obvious benefit in trying to short-circuit the loop
using a strchr, as both ways you still end up iterating over all
chars in the string - its just that you're hiding the iteration
in strchr instead.

> > +static ssize_t qdict_list_size(QDict *maybe_list, Error **errp)
> > +{
> > +    const QDictEntry *entry, *next;
> > +    ssize_t len = 0;
> > +    ssize_t max = -1;
> > +    int is_list = -1;
> > +    int64_t val;
> > +
> > +    entry = qdict_first(maybe_list);
> > +    while (entry != NULL) {
> > +        next = qdict_next(maybe_list, entry);
> > +
> > +        if (qemu_strtoll(entry->key, NULL, 10, &val) == 0) {
> > +            if (is_list == -1) {
> > +                is_list = 1;
> > +            } else if (!is_list) {
> > +                error_setg(errp,
> > +                           "Key '%s' is for a list, but previous key is "
> > +                           "for a dict", entry->key);
> 
> Keys are unsorted, so it's a bit hard to call it "previous key".  Maybe
> a better error message would be along the lines of "cannot crumple
> dictionary because of a mix of list and non-list keys"?  I dunno...

Yeah, I'll use

  "Cannot crumple a dictionary with a mix of list and non-list keys"


> 
> > +                return -1;
> > +            }
> > +            len++;
> > +            if (val > max) {
> > +                max = val;
> > +            }
> > +        } else {
> > +            if (is_list == -1) {
> > +                is_list = 0;
> > +            } else if (is_list) {
> > +                error_setg(errp,
> > +                           "Key '%s' is for a dict, but previous key is "
> > +                           "for a list", entry->key);
> 
> ...same argument. If we can wordsmith something that makes sense, it
> might work for both places.  Otherwise, I can live with your messages.


> > +++ b/tests/check-qdict.c
> > @@ -596,6 +596,140 @@ static void qdict_join_test(void)
> >      QDECREF(dict2);
> >  }
> >  
> > +
> > +static void qdict_crumple_test_nonrecursive(void)
> > +{
> 
> This only covers a single layer of collapse, but not turning a dict into
> a list.  Is it also worth covering a case where no list indices are
> involved, such as the four keys "a.b.d", "a.b.e", "a.c.d", "a.d.e" being
> crumpled non-recursively into a single dict "a" with keys "b.d", "b.e",
> "c.d", and "d.e"?

I'll add an explicit rule to test dict -> list conversion, and some
extra dict items here to cover proper nested dicts.

> 
> > +
> > +static void qdict_crumple_test_recursive(void)
> > +{
> > +
> 
> This only covers a list of dict collapse, not a true multi-layer dict
> collapse.  Is it also worth covering the same four keys as above, but
> this time that dict "a" has keys "b" and "c", each of which is a dict in
> turn with keys "d" and "e"?

I'll add some more dict items to properly cover nested dicts

> > +static void qdict_crumple_test_bad_inputs(void)
> > +{
> > +    QDict *src;
> > +    Error *error = NULL;
> > +
> 
> > +
> > +    src = qdict_new();
> > +    /* The input should be flat, ie no dicts or lists */
> > +    qdict_put(src, "rule.0", qdict_new());
> > +    qdict_put(src, "rule.a", qstring_from_str("allow"));
> 
> I'd use "rule.a" and "rule.b" here, so that you aren't confusing this
> with the earlier test that you can't mix list and dict.

Good point.

> I'd also add a negative test for "rule.1" without "rule.0" being invalid
> (missing a list index).

Yep, I'll add that.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|