Re: [Qemu-devel] [PATCH 04/16] qapi: introduce OptsVisitor

[Paolo Bonzini]

Il 22/05/2012 12:45, Laszlo Ersek ha scritto:
> 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 / int / size.

Michael Roth recently added Visitor interfaces for uint*_t and int*_t,
they look like they would simplify the patches very nicely.  They do the
range checking automatically and only need a type_int callback in the
visitor.

You can get the patch from http://patchwork.ozlabs.org/patch/150427/,
feel free to include it at the beginning of your series.

> Members marked with [*] may be defined as optional in the schema,
> describing an optional optarg.
> 
> Repeating an optarg is supported; 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.

Awesome!

Reviewed-by: Paolo Bonzini <address@hidden>

Paolo

> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>  qapi/opts-visitor.h |   31 +++++
>  qapi/opts-visitor.c |  363 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  Makefile.objs       |    2 +-
>  3 files changed, 395 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..edd6d41
> --- /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 are rejected (parsed values
> + *   continue to be non-negative),
> + * - 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..a94c67e
> --- /dev/null
> +++ b/qapi/opts-visitor.c
> @@ -0,0 +1,363 @@
> +/*
> + * 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);
> +    }
> +}
> +
> +
> +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;
> +        } 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 && 0 <= 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_size(Visitor *v, int64_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_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/Makefile.objs b/Makefile.objs
> index 70c5c79..5988835 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -448,7 +448,7 @@ libcacard-y = cac.o event.o vcard.o vreader.o 
> vcard_emul_nss.o vcard_emul_type.o
>  
>  qapi-nested-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
>  qapi-nested-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
> -qapi-nested-y += string-input-visitor.o string-output-visitor.o
> +qapi-nested-y += string-input-visitor.o string-output-visitor.o 
> opts-visitor.o
>  qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>  
>  common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o