Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor

[Luiz Capitulino]

On Wed, 13 Jun 2012 10:22:36 +0200
Laszlo Ersek <address@hidden> wrote:

> This visitor supports parsing
> 
>   -option [type=]discriminator[,optarg1=val1][,optarg2=val2][,...]
> 
> style QemuOpts objects into "native" C structures. After defining the type
> tree in the qapi schema (see below), a root type traversal with this
> visitor linked to the underlying QemuOpts object will build the "native" C
> representation of the option.
> 
> The type tree in the schema, corresponding to an option with a
> discriminator, must have the following structure:
> 
>   struct
>     scalar member for non-discriminated optarg 1 [*]
>     list for repeating non-discriminated optarg 2 [*]
>       wrapper struct
>         single scalar member
>     union
>       struct for discriminator case 1
>         scalar member for optarg 3 [*]
>         list for repeating optarg 4 [*]
>           wrapper struct
>             single scalar member
>         scalar member for optarg 5 [*]
>       struct for discriminator case 2
>         ...
> 
> The "type" optarg name is fixed for the discriminator role. Its schema
> representation is "union of structures", and each discriminator value must
> correspond to a member name in the union.
> 
> If the option takes no "type" descriminator, then the type subtree rooted
> at the union must be absent from the schema (including the union itself).
> 
> Optarg values can be of scalar types str / bool / integers / size.
> 
> Members marked with [*] may be defined as optional in the schema,
> describing an optional optarg.
> 
> Repeating an optarg is supported; 

I see that the current code supports this too, but why? Something like this
should fail:

 -netdev type=tap,vhost=on,vhost=off,id=guest1,script=qemu-ifup-switch

More comments below.

> its schema representation must be "list
> of structure with single mandatory scalar member". If an optarg is not
> described as repeating in the schema (ie. it is defined as a scalar field
> instead of a list), its last occurrence will take effect. Ordering between
> differently named optargs is not preserved.
> 
> A mandatory list (or an optional one which is reported to be available),
> corresponding to a repeating optarg, has at least one element after
> successful parsing.
> 
> v1->v2:
> - Update opts_type_size() prototype to uint64_t.
> - Add opts_type_uint64() for options needing the full uint64_t range.
>   (Internals could be extracted to "cutils.c".)
> - Allow negative values in opts_type_int().
> - Rebase to nested Makefiles.
> 
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>  qapi/opts-visitor.h |   31 ++++
>  qapi/opts-visitor.c |  401 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/Makefile.objs  |    2 +-
>  3 files changed, 433 insertions(+), 1 deletions(-)
>  create mode 100644 qapi/opts-visitor.h
>  create mode 100644 qapi/opts-visitor.c
> 
> diff --git a/qapi/opts-visitor.h b/qapi/opts-visitor.h
> new file mode 100644
> index 0000000..ea1a395
> --- /dev/null
> +++ b/qapi/opts-visitor.h
> @@ -0,0 +1,31 @@
> +/*
> + * Options Visitor
> + *
> + * Copyright Red Hat, Inc. 2012
> + *
> + * Author: Laszlo Ersek <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#ifndef OPTS_VISITOR_H
> +#define OPTS_VISITOR_H
> +
> +#include "qapi-visit-core.h"
> +#include "qemu-option.h"
> +
> +typedef struct OptsVisitor OptsVisitor;
> +
> +/* Contrarily to qemu-option.c::parse_option_number(), OptsVisitor's "int"
> + * parser relies on strtoll() instead of strtoull(). Consequences:
> + * - string representations of negative numbers yield negative values,
> + * - values below INT64_MIN or LLONG_MIN are rejected,
> + * - values above INT64_MAX or LLONG_MAX are rejected.
> + */
> +OptsVisitor *opts_visitor_new(const QemuOpts *opts);
> +void opts_visitor_cleanup(OptsVisitor *nv);
> +Visitor *opts_get_visitor(OptsVisitor *nv);
> +
> +#endif
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> new file mode 100644
> index 0000000..9187c86
> --- /dev/null
> +++ b/qapi/opts-visitor.c
> @@ -0,0 +1,401 @@
> +/*
> + * Options Visitor
> + *
> + * Copyright Red Hat, Inc. 2012
> + *
> + * Author: Laszlo Ersek <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "opts-visitor.h"
> +#include "qemu-queue.h"
> +#include "qemu-option-internal.h"
> +#include "qapi-visit-impl.h"
> +
> +
> +struct OptsVisitor
> +{
> +    Visitor visitor;
> +
> +    /* Ownership remains with opts_visitor_new()'s caller. */
> +    const QemuOpts *opts_root;
> +
> +    unsigned depth;
> +
> +    /* Non-null iff depth is positive. Each key is a QemuOpt name. Each value
> +     * is a non-empty GQueue, enumerating all QemuOpt occurrences with that
> +     * name. */
> +    GHashTable *unprocessed_opts;
> +
> +    /* The list currently being traversed with opts_start_list() /
> +     * opts_next_list(). The list must have a struct element type in the
> +     * schema, with a single mandatory scalar member. */
> +    GQueue *repeated_opts;
> +    bool repeated_opts_first;
> +};
> +
> +
> +static void
> +destroy_list(gpointer list)
> +{
> +  g_queue_free(list);
> +}
> +
> +
> +static void
> +opts_start_struct(Visitor *v, void **obj, const char *kind,
> +                  const char *name, size_t size, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    const QemuOpt *opt;
> +
> +    *obj = g_malloc0(size);
> +    if (ov->depth++ > 0) {
> +        return;
> +    }
> +
> +    ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
> +                                                 NULL, &destroy_list);
> +    QTAILQ_FOREACH(opt, &ov->opts_root->head, next) {
> +        GQueue *list;
> +
> +        list = g_hash_table_lookup(ov->unprocessed_opts, opt->name);
> +        if (list == NULL) {
> +            list = g_queue_new();
> +
> +            /* GHashTable will never try to free the keys -- we supplied NULL
> +             * as "key_destroy_func" above. Thus cast away key const-ness in
> +             * order to suppress gcc's warning. */
> +            g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name,
> +                                list);
> +        }
> +
> +        /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
> +        g_queue_push_tail(list, (gpointer)opt);
> +    }
> +}

This doesn't insert the opts id into the hash, so opts_type_str() will fail
to find the id when the generated code visits it here:

void visit_type_Netdev(Visitor *m, Netdev ** obj, const char *name, Error 
**errp)
{       
    if (!error_is_set(errp)) {
        Error *err = NULL;
        visit_start_struct(m, (void **)obj, "Netdev", name, sizeof(Netdev), 
&err);  
        if (!err) {
            assert(!obj || *obj);
            visit_type_str(m, obj ? &(*obj)->id : NULL, "id", &err); <---------
[...]

Also, you're using a queue to support the repeating of optargs, right? I think
this could be simplified if we just don't support that.

> +
> +
> +static gboolean
> +ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
> +{
> +    return TRUE;
> +}
> +
> +
> +static void
> +opts_end_struct(Visitor *v, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    GQueue *any;
> +
> +    if (--ov->depth > 0) {
> +        return;
> +    }
> +
> +    /* we should have processed all (distinct) QemuOpt instances */
> +    any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
> +    if (any) {
> +        const QemuOpt *first;
> +
> +        first = g_queue_peek_head(any);
> +        error_set(errp, QERR_INVALID_PARAMETER, first->name);
> +    }
> +    g_hash_table_destroy(ov->unprocessed_opts);
> +    ov->unprocessed_opts = NULL;
> +}
> +
> +
> +static GQueue *
> +lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
> +{
> +    GQueue *list;
> +
> +    list = g_hash_table_lookup(ov->unprocessed_opts, name);
> +    if (!list) {
> +        error_set(errp, QERR_MISSING_PARAMETER, name);
> +    }
> +    return list;
> +}
> +
> +
> +static void
> +opts_start_list(Visitor *v, const char *name, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +
> +    /* we can't traverse a list in a list */
> +    assert(ov->repeated_opts == NULL);
> +    ov->repeated_opts = lookup_distinct(ov, name, errp);
> +    ov->repeated_opts_first = (ov->repeated_opts != NULL);
> +}
> +
> +
> +static GenericList *
> +opts_next_list(Visitor *v, GenericList **list, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    GenericList **link;
> +
> +    if (ov->repeated_opts_first) {
> +        ov->repeated_opts_first = false;
> +        link = list;
> +    } else {
> +        const QemuOpt *opt;
> +
> +        opt = g_queue_pop_head(ov->repeated_opts);
> +        if (g_queue_is_empty(ov->repeated_opts)) {
> +            g_hash_table_remove(ov->unprocessed_opts, opt->name);
> +            return NULL;
> +        }
> +        link = &(*list)->next;
> +    }
> +
> +    *link = g_malloc0(sizeof **link);
> +    return *link;
> +}
> +
> +
> +static void
> +opts_end_list(Visitor *v, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +
> +    ov->repeated_opts = NULL;
> +}
> +
> +
> +static const QemuOpt *
> +lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
> +{
> +    if (ov->repeated_opts == NULL) {
> +        GQueue *list;
> +
> +        /* the last occurrence of any QemuOpt takes effect when queried by 
> name
> +         */
> +        list = lookup_distinct(ov, name, errp);
> +        return list ? g_queue_peek_tail(list) : NULL;
> +    }
> +    return g_queue_peek_head(ov->repeated_opts);
> +}
> +
> +
> +static void
> +processed(OptsVisitor *ov, const char *name)
> +{
> +    if (ov->repeated_opts == NULL) {
> +        g_hash_table_remove(ov->unprocessed_opts, name);
> +    }
> +}
> +
> +
> +static void
> +opts_type_str(Visitor *v, char **obj, const char *name, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    const QemuOpt *opt;
> +
> +    opt = lookup_scalar(ov, name, errp);
> +    if (!opt) {
> +        return;
> +    }
> +    *obj = g_strdup(opt->str ? opt->str : "");
> +    processed(ov, name);
> +}
> +
> +
> +/* mimics qemu-option.c::parse_option_bool() */
> +static void
> +opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    const QemuOpt *opt;
> +
> +    opt = lookup_scalar(ov, name, errp);
> +    if (!opt) {
> +        return;
> +    }
> +
> +    if (opt->str) {
> +        if (strcmp(opt->str, "on") == 0 ||
> +            strcmp(opt->str, "yes") == 0 ||
> +            strcmp(opt->str, "y") == 0) {
> +            *obj = true;
> +        } else if (strcmp(opt->str, "off") == 0 ||
> +            strcmp(opt->str, "no") == 0 ||
> +            strcmp(opt->str, "n") == 0) {
> +            *obj = false;

The current code only accepts 'on' or 'off', no reason to change that.

> +        } else {
> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> +                "on|yes|y|off|no|n");
> +            return;
> +        }
> +    } else {
> +        *obj = true;
> +    }
> +
> +    processed(ov, name);
> +}
> +
> +
> +static void
> +opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    const QemuOpt *opt;
> +    const char *str;
> +    long long val;
> +    char *endptr;
> +
> +    opt = lookup_scalar(ov, name, errp);
> +    if (!opt) {
> +        return;
> +    }
> +    str = opt->str ? opt->str : "";
> +
> +    errno = 0;
> +    val = strtoll(str, &endptr, 0);
> +    if (*str != '\0' && *endptr == '\0' && errno == 0 && INT64_MIN <= val &&
> +        val <= INT64_MAX) {
> +        *obj = val;
> +        processed(ov, name);
> +        return;
> +    }
> +    error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> +              "a non-negative int64 value");
> +}
> +
> +
> +static void
> +opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    const QemuOpt *opt;
> +    const char *str;
> +
> +    opt = lookup_scalar(ov, name, errp);
> +    if (!opt) {
> +        return;
> +    }
> +
> +    str = opt->str;
> +    if (str != NULL) {
> +        while (isspace((unsigned char)*str)) {
> +            ++str;
> +        }
> +
> +        if (*str != '-' && *str != '\0') {
> +            unsigned long long val;
> +            char *endptr;
> +
> +            /* non-empty, non-negative subject sequence */
> +            errno = 0;
> +            val = strtoull(str, &endptr, 0);
> +            if (*endptr == '\0' && errno == 0 && val <= UINT64_MAX) {
> +                *obj = val;
> +                processed(ov, name);
> +                return;
> +            }
> +        }
> +    }
> +    error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> +              "an uint64 value");
> +}
> +
> +
> +static void
> +opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    const QemuOpt *opt;
> +    int64_t val;
> +    char *endptr;
> +
> +    opt = lookup_scalar(ov, name, errp);
> +    if (!opt) {
> +        return;
> +    }
> +
> +    val = strtosz_suffix(opt->str ? opt->str : "", &endptr,
> +                         STRTOSZ_DEFSUFFIX_B);
> +    if (val != -1 && *endptr == '\0') {
> +        *obj = val;
> +        processed(ov, name);
> +        return;
> +    }
> +    error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> +              "a size value representible as a non-negative int64");
> +}
> +
> +
> +static void
> +opts_start_optional(Visitor *v, bool *present, const char *name,
> +                       Error **errp)
> +{
> +    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +
> +    /* we only support a single mandatory scalar field in a list node */
> +    assert(ov->repeated_opts == NULL);
> +    *present = (lookup_distinct(ov, name, NULL) != NULL);
> +}
> +
> +
> +OptsVisitor *
> +opts_visitor_new(const QemuOpts *opts)
> +{
> +    OptsVisitor *ov;
> +
> +    ov = g_malloc0(sizeof *ov);
> +
> +    ov->visitor.start_struct = &opts_start_struct;
> +    ov->visitor.end_struct   = &opts_end_struct;
> +
> +    ov->visitor.start_list = &opts_start_list;
> +    ov->visitor.next_list  = &opts_next_list;
> +    ov->visitor.end_list   = &opts_end_list;
> +
> +    /* input_type_enum() covers both "normal" enums and union discriminators.
> +     * The union discriminator field is always generated as "type"; it should
> +     * match the "type" QemuOpt child of any QemuOpts.
> +     *
> +     * input_type_enum() will remove the looked-up key from the
> +     * "unprocessed_opts" hash even if the lookup fails, because the removal 
> is
> +     * done earlier in opts_type_str(). This should be harmless.
> +     */
> +    ov->visitor.type_enum = &input_type_enum;
> +
> +    ov->visitor.type_int    = &opts_type_int;
> +    ov->visitor.type_uint64 = &opts_type_uint64;
> +    ov->visitor.type_size   = &opts_type_size;
> +    ov->visitor.type_bool   = &opts_type_bool;
> +    ov->visitor.type_str    = &opts_type_str;
> +
> +    /* type_number() is not filled in, but this is not the first visitor to
> +     * skip some mandatory methods... */
> +
> +    ov->visitor.start_optional = &opts_start_optional;
> +
> +    ov->opts_root = opts;
> +
> +    return ov;
> +}
> +
> +
> +void
> +opts_visitor_cleanup(OptsVisitor *ov)
> +{
> +    if (ov->unprocessed_opts != NULL) {
> +        g_hash_table_destroy(ov->unprocessed_opts);
> +    }
> +    memset(ov, '\0', sizeof *ov);
> +}
> +
> +
> +Visitor *
> +opts_get_visitor(OptsVisitor *ov)
> +{
> +    return &ov->visitor;
> +}
> diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
> index d0b0c16..5f5846e 100644
> --- a/qapi/Makefile.objs
> +++ b/qapi/Makefile.objs
> @@ -1,3 +1,3 @@
>  qapi-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
>  qapi-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
> -qapi-obj-y += string-input-visitor.o string-output-visitor.o
> +qapi-obj-y += string-input-visitor.o string-output-visitor.o opts-visitor.o