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 :|