[04/16] qapi: introduce OptsVisitor

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.

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.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 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

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 <pbonzini@redhat.com>

Paolo

> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  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 <lersek@redhat.com>
> + *
> + * 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 <lersek@redhat.com>
> + *
> + * 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

Thank you very much for the review! One question below (and maybe some
more later in response to other parts of the review):

On 06/05/12 23:12, Paolo Bonzini wrote:
> Il 22/05/2012 12:45, Laszlo Ersek ha scritto:

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

The unsigned functions don't check if the int64_t value to be assigned is
negative. What happens in such a case is fully defined, but did Michael
really intend the wraparound to unsigned?

I thought it was a silent requirement for the int parser to return a
non-negative int (which is something I also implemented in
opts_type_int() [*]), but the signed visit_type_XXX() functions do check
for the negative limit. I'm confused.


[*] see the comment in opts-visitor.h: 

/* 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.
 */

That's also why I dropped/omitted the "< 0" checks in net_init_nic() [v1
09/16], net_init_dump() [v1 10/16] and net_init_vde() [v1 13/16].

If you want I can go through all the integer fields I introduced in [v1
06/16] "qapi schema: add Netdev types" and classify each as strictly as
possible (and then remove the checks being obviated).

Thanks!
Laszlo

Il 06/06/2012 13:12, Laszlo Ersek ha scritto:
> The unsigned functions don't check if the int64_t value to be assigned is
> negative. What happens in such a case is fully defined, but did Michael
> really intend the wraparound to unsigned?

I think that's a bug.  Please coordinate with Michael and Andreas (since
the patch is currently lying in his tree) as to who fixes what.

> I thought it was a silent requirement for the int parser to return a
> non-negative int (which is something I also implemented in
> opts_type_int() [*]), but the signed visit_type_XXX() functions do check
> for the negative limit. I'm confused.

It's not a requirement for the int parser, but I remember now: it was a
requirement for the opts visitor, because qemu_opt_get_number returns
uint64_t rather than int64_t.

However, net_init_* shouldn't assume anything about their struct
argument, so the check needs to be added there as if they weren't in the
OptsVisitor.  Unless you make the types stricter, of course.

> If you want I can go through all the integer fields I introduced in [v1
> 06/16] "qapi schema: add Netdev types" and classify each as strictly as
> possible (and then remove the checks being obviated).

Yes, that would be very nice.

Paolo