Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Eric Blake]

On 02/21/2017 03:01 PM, Markus Armbruster wrote:
> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
> qemu_opts_parse(), except:
> 
> * Returns a QDict instead of a QemuOpts (d'oh).
> 
> * It supports nesting, unlike QemuOpts: a KEY is split into key
>   components at '.' (dotted key convention; the block layer does
>   something similar on top of QemuOpts).  The key components are QDict
>   keys, and the last one's value is updated to VALUE.
> 
> * Each key component may be up to 127 bytes long.  qemu_opts_parse()
>   limits the entire key to 127 bytes.
> 
> * Overlong key components are rejected.  qemu_opts_parse() silently
>   truncates them.
> 
> * Empty key components are rejected.  qemu_opts_parse() happily
>   accepts empty keys.
> 
> * It does not store the returned value.  qemu_opts_parse() stores it
>   in the QemuOptsList.
> 
> * It does not treat parameter "id" specially.  qemu_opts_parse()
>   ignores all but the first "id", and fails when its value isn't
>   id_wellformed(), or duplicate (a QemuOpts with the same ID is
>   already stored).  It also screws up when a value contains ",id=".
> 
> I intend to grow this into a saner replacement for QemuOpts.  It'll
> take time, though.
> 
> TODO Support lists
> 
> TODO Function comment is missing.

Hence still being RFC, and I won't give R-b, but will still review.

> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---

>  
> +QDict *keyval_parse(const char *params, const char *implied_key,
> +                    Error **errp);
> +

> +++ b/tests/test-keyval.c

> +
> +static void test_keyval_parse(void)
> +{
> +    Error *err = NULL;
> +    QDict *qdict, *sub_qdict;
> +    char long_key[129];
> +    char *params;
> +
> +    /* Nothing */
> +    qdict = keyval_parse("", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 0);
> +    QDECREF(qdict);
> +
> +    /* Empty key */
> +    qdict = keyval_parse("=val", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +
> +    /* Empty key component */
> +    qdict = keyval_parse(".", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +    qdict = keyval_parse("key.", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

Do you also want to test ".=" or "key.=" ?

> +
> +    /* Overlong key */
> +    memset(long_key, 'a', 127);
> +    long_key[127] = 'z';
> +    long_key[128] = 0;
> +    params = g_strdup_printf("k.%s=v", long_key);
> +    qdict = keyval_parse(params + 2, NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +
> +    /* Overlong key component */
> +    qdict = keyval_parse(params, NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +    g_free(params);
> +
> +    /* Long key */
> +    params = g_strdup_printf("k.%s=v", long_key + 1);
> +    qdict = keyval_parse(params + 2, NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, long_key + 1), ==, "v");
> +    QDECREF(qdict);
> +
> +    /* Long key component */
> +    qdict = keyval_parse(params, NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    sub_qdict = qdict_get_qdict(qdict, "k");
> +    g_assert(sub_qdict);
> +    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(sub_qdict, long_key + 1), ==, "v");
> +    QDECREF(qdict);
> +    g_free(params);

As mentioned in the commit message, this works when QemuOpts would have
rejected it as overlong.  Good, in my opinion.

> +
> +    /* Multiple keys, last one wins */
> +    qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 2);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "a"), ==, "3");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "b"), ==, "2,x");
> +    QDECREF(qdict);
> +
> +    /* Even when it doesn't in QemuOpts */
> +    qdict = keyval_parse("id=foo,id=bar", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "bar");
> +    QDECREF(qdict);
> +
> +    /* Dotted keys */
> +    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 2);
> +    sub_qdict = qdict_get_qdict(qdict, "a");
> +    g_assert(sub_qdict);
> +    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
> +    sub_qdict = qdict_get_qdict(sub_qdict, "b");
> +    g_assert(sub_qdict);
> +    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(sub_qdict, "c"), ==, "2");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "d"), ==, "3");
> +    QDECREF(qdict);
> +
> +    /* Inconsistent dotted keys */
> +    qdict = keyval_parse("a.b=1,a=2", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +    qdict = keyval_parse("a.b=1,a.b.c=2", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

Will probably need more tests for inconsistencies when you support arrays.

> +
> +    /* Implied value */
> +    qdict = keyval_parse("an,noaus,noaus=", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 3);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "an"), ==, "on");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "aus"), ==, "off");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "noaus"), ==, "");
> +    QDECREF(qdict);

Oh, so '$key' implies the same as '$key=true'; 'no$key' implies the same
as '$key=false', and the presence of = is what changes an implicit bool
(with magic 'no' prefix handling) into a full keyname.  Looks a bit
weird, but it matches what QemuOpts does so we do need to keep that
magic around.

> +
> +    /* Implied key */
> +    qdict = keyval_parse("an,noaus,noaus=", "implied", &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 3);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "an");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "aus"), ==, "off");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "noaus"), ==, "");
> +    QDECREF(qdict);
> +
> +    /* Trailing comma is ignored */
> +    qdict = keyval_parse("x=y,", NULL,  &error_abort);

Why two spaces here?

> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, "y");
> +    QDECREF(qdict);

Nice. Too bad JSON can't be as forgiving about trailing commas.

> +
> +    /* Except when it isn't */
> +    qdict = keyval_parse(",", NULL,  &err);

Again, why two spaces? (I'll quit pointing it out, if there's more)

> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

Question: what happens with:

keyval_parse(",", "implied", &err)

Should that be the same as:

keyval_parse("implied=,", NULL, &err)

which in turn is the same as:

keyval_parse("implied=", NULL, &err)

Should you update "test-qemu-opts: Cover qemu_opts_parse()" to do a
QemuOpts counterpart of this question?

> +
> +    /* Value containing ,id= not misinterpreted as QemuOpts does */
> +    qdict = keyval_parse("x=,,id=bar", NULL,  &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, ",id=bar");
> +    QDECREF(qdict);

Yes, definitely better than QemuOpts' wart.

> +
> +    /* Anti-social ID is left to caller */
> +    qdict = keyval_parse("id=666", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "666");
> +    QDECREF(qdict);

Nice to compare this to your earlier patch on enhancing QemuOpts tests
(your reminder of my chuckle on the "anti-social ID" on that patch was
worth it).  That test had:

+    /* TODO Cover .merge_lists = true */

which matches this RFC's TODO for adding list support; it also had:

+    /* Unknown key */

which is not needed here, since this focuses on just the parsing rather
than also the mapping into recognized QemuOpt key names (the counterpart
here would be that the caller would flag extra keys by using a strict
qobject-input parse on the resulting QObject).

> +++ b/util/keyval.c
> @@ -0,0 +1,150 @@
> +/*
> + * Parsing KEY=VALUE,... strings
> + *
> + * Copyright (C) 2017 Red Hat Inc.
> + *
> + * Authors:
> + *  Markus Armbruster <address@hidden>,
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qemu/option.h"
> +
> +/* TODO Support lists */
> +
> +static QObject *keyval_parse_put(QDict *qdict, const char *key, QString 
> *value,
> +                                 Error **errp)
> +{
> +    QObject *old, *new;
> +
> +    old = qdict_get(qdict, key);
> +    if (old) {
> +        if (qobject_type(old) != (value ? QTYPE_QSTRING : QTYPE_QDICT)) {
> +            error_setg(errp, "Option key '%s' used inconsistently", key);
> +            return NULL;
> +        }
> +        if (!value) {
> +            return old;
> +        }
> +        new = QOBJECT(value);

So if we've already seen key, it is either a subdict (and we return the
existing sub-dict to add more into it) or a string (and we replace the
old string with the new)...

> +    } else {
> +        new = QOBJECT(value) ?: QOBJECT(qdict_new());

...and if we haven't seen key, we either add the string or a new subdict...

> +    }
> +    qdict_put_obj(qdict, key, new);
> +    return new;

...either way, we are returning the current value of the key within
qdict to the caller.

Comments may indeed help, because it took me a couple of reads before I
saw what was going on, but the function looks sane.

> +}
> +
> +static const char *keyval_parse_one(QDict *qdict, const char *params,
> +                                    const char *implied_key,
> +                                    Error **errp)
> +{
> +    QDict *cur = qdict;
> +    QObject *next;
> +    const char *s, *key;
> +    size_t len;
> +    char key_buf[128];
> +    QString *val;
> +
> +    s = params;
> +    len = strcspn(s, ".=,");
> +    if (implied_key && (s[len] == ',' || !s[len])) {
> +        /* Desugar implied key */
> +        key = implied_key;

Should keyval_parse("=foo", "implied", &err) also behave the same as
keyval_parse("implied==foo", NULL, &err) (resulting in "=foo" as the value)?

> +    } else {
> +        key_buf[0] = 0;
> +        for (;;) {
> +            if (!len) {
> +                error_setg(errp, "Invalid option key");
> +                return NULL;
> +            }
> +            if (len >= sizeof(key_buf)) {
> +                error_setg(errp, "Option key component '%.*s' is too long",
> +                           (int)len, s);
> +                return NULL;
> +            }
> +
> +            if (key_buf[0]) {
> +                next = keyval_parse_put(cur, key_buf, NULL, errp);
> +                if (!next) {
> +                    return NULL;
> +                }
> +                cur = qobject_to_qdict(next);
> +                assert(cur);
> +            }
> +
> +            memcpy(key_buf, s, len);
> +            key_buf[len] = 0;
> +            s += len;
> +            if (*s != '.') {
> +                break;
> +            }
> +            s++;
> +            len = strcspn(s, ".=,");
> +        }
> +        key = key_buf;
> +
> +        if (*s == '=') {
> +            s++;

I'm not sure this condition is correct, if there is an implied key where
we want the value string to start with "=".  Or are we requiring that
starting a value with = requires that we can't use implicit key?
Missing testsuite coverage, I think?

> +        } else {
> +            /*
> +             * Desugar implied value: it's "on", except when @key
> +             * starts with "no", it's "off".  Thus, key "novocaine"
> +             * gets desugard to "vocaine=off", not to "novocaine=on".
> +             * If sugar isn't bad enough for you, make it ambiguous...

So if I get this right,

keyval_parse(",", "implied", &err) => "implied" = "on"
keyval_parse(",", "noimplied", &err) => "implied" = "off"
keyval_parse("on", "noimplied", &err) => "noimplied" = "on"

Eww. Not necessarily your fault.  But maybe this is our chance to tweak
it to be slightly different?

> +             */
> +            if (*s == ',')
> +                s++;

Should this account for leading ",," with implied key (meaning a value
that starts with a comma)?  Or is that another thing where a leading
comma in a value cannot be mixed with an implied key?

> +            if (!strncmp(key, "no", 2)) {
> +                key += 2;
> +                val = qstring_from_str("off");
> +            } else {
> +                val = qstring_from_str("on");
> +            }
> +            goto got_val;
> +        }
> +    }
> +
> +    val = qstring_new();
> +    for (;;) {
> +        if (!*s) {
> +            break;
> +        } else if (*s == ',') {
> +            s++;
> +            if (*s != ',') {
> +                break;
> +            }
> +        }
> +        qstring_append_chr(val, *s++);
> +    }
> +
> +got_val:
> +    if (!keyval_parse_put(cur, key, val, errp)) {
> +        return NULL;
> +    }
> +    return s;
> +}

You managed to make this fairly compact for how hairy it is!  I don't
know how lists will impact it (well, maybe you'll have integral keys on
this pass, and then a second pass that turns integral keys into list
members...)

> +
> +/* TODO function comment */
> +QDict *keyval_parse(const char *params, const char *implied_key,
> +                    Error **errp)
> +{
> +    QDict *qdict = qdict_new();
> +    const char *s;

Should we do any sort of validation on implied_key, such as making sure
it is non-NULL and non-empty, does not contain ".=,", looks like a
well-formed id?  Or is that merely documentation that a caller that
passes a garbage implied_key gets a garbage QDict result?

> +
> +    s = params;
> +    while (*s) {
> +        s = keyval_parse_one(qdict, s, implied_key, errp);
> +        if (!s) {
> +            QDECREF(qdict);
> +            return NULL;
> +        }
> +        implied_key = NULL;
> +    }
> +
> +    return qdict;
> +}
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature