Re: [Qemu-block] [PATCH v3 01/10] qdict: implement a qdict_crumple method for un-flattening a dict
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 :|
Re: [Qemu-block] [PATCH v3 01/10] qdict: implement a qdict_crumple method for un-flattening a dict
On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:
> 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.
>
>
> 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' => [
s/=>/:/
> { 'bar': 'one', 'wizz': '1' }
s/$/,/
> { 'bar': 'two', 'wizz': '2' }
>],
> }
>
> The intent of this function is that it allows a set of QemuOpts
> to be turned into a nested data structure that mirrors the nested
s/the nested/the nesting/
> used when the same object is defined over QMP.
>
> Signed-off-by: Daniel P. Berrange
> ---
> include/qapi/qmp/qdict.h | 1 +
> qobject/qdict.c | 267
> +++
> tests/check-qdict.c | 143 +
> 3 files changed, 411 insertions(+)
>
> +
> +/**
> + * qdict_split_flat_key:
> + *
> + * Given a flattened key such as 'foo.0.bar', split it
> + * into two parts at the first '.' separator. Allows
> + * double dot ('..') to escape the normal separator.
> + *
> + * eg
> + *'foo.0.bar' -> prefix='foo' and suffix='0.bar'
> + *'foo..0.bar' -> prefix='foo.0' and suffix='bar'
> + *
> + * The '..' sequence will be unescaped in the returned
> + * 'prefix' string. The 'suffix' string will be left
> + * in escaped format, so it can be fed back into the
> + * qdict_split_flat_key() key as the input later.
> + */
Might be worth mentioning that prefix and suffix must both be non-NULL,
and that the caller must g_free() the two resulting strings.
> +static void qdict_split_flat_key(const char *key, char **prefix, char
> **suffix)
> +{
> +const char *separator;
> +size_t i, j;
> +
> +/* Find first '.' separator, but if there is a pair '..'
> + * that acts as an escape, so skip over '..' */
> +separator = NULL;
> +do {
> +if (separator) {
> +separator += 2;
> +} else {
> +separator = key;
> +}
> +separator = strchr(separator, '.');
> +} while (separator && *(separator + 1) == '.');
I'd probably have written separator[1] == '.', but your approach is
synonymous.
> +
> +if (separator) {
> +*prefix = g_strndup(key,
> +separator - key);
> +*suffix = g_strdup(separator + 1);
> +} else {
> +*prefix = g_strdup(key);
> +*suffix = NULL;
> +}
> +
> +/* 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++;
> +}
> +(*prefix)[j] = (*prefix)[i];
> +}
> +(*prefix)[j] = '\0';
> +}
> +
> +
> +/**
> + * qdict_list_size:
> + * @maybe_List: dict that may be only list elements
s/List/list/
> + *
> + * Determine whether all keys in @maybe_list are
> + * valid list elements. They they are all valid,
s/They they/If they/
> + * then this returns the number of elements. If
> + * they all look like non-numeric keys, then returns
> + * zero. If there is a mix of numeric and non-numeric
> + * keys, then an error is set as it is both a list
> + * and a dict at once.
> + *
> + * Returns: number of list elemets, 0 if a dict, -1 on error
s/elemets/elements/
> + */
> +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...
> +retur
