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

[Laszlo Ersek <lersek@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6752 bytes --]

On 07/13/12 18:51, Luiz Capitulino wrote:
> On Wed, 13 Jun 2012 10:22:36 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:

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

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

I hate repeated options with a passion, but SLIRP's hostfwd and guestfwd
depend on repetition.

When the outermost opts_start_struct() is invoked and I shovel the
optargs into the queues, I can't yet know what's going to be used in
repeated form and what not.

If you prefer I can change lookup_scalar() as follows. For reference:

>> +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_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);
>> +}

>> +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;

We're outside of list traversal in this branch, meaning the optarg is
allowed exactly once. (Optional optargs are first handled by
opts_start_optional().) If lookup_distinct() succeeds here, then rather
than returning the last occurrence, I could check the depth of the queue
(== 1 or > 1), and set an error for > 1.

However QemuOpts definitely supports repeated optargs now (otherwise
slirp hostfwd/guestfwd wouldn't work). qemu_opt_foreach() is used for
iteration (with QTAILQ_FOREACH()), while qemu_opt_find() -- and thus its
direct callers -- rely on QTAILQ_FOREACH_REVERSE() and the first match.
Optargs of an option are apparently chained like this:

  qemu_opts_parse() [qemu-option.c]
    opts_parse(..., defaults=false)
      opts_do_parse(..., prepend=false)
        opt_set(..., prepend=false, ...)
          QTAILQ_INSERT_TAIL()

"-option arg=val1,arg=val2,arg=val3" is therefore linked into the
corresponding QemuOpts instance in the same order, and qemu_opt_find()
will return "arg=val3". I also use g_queue_push_tail() and
g_queue_peek_tail(), so I think we're compatible.

>> +    }
>> +    return g_queue_peek_head(ov->repeated_opts);
>> +}


Continuing slightly out of order:

>> +/* 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);
>> +}

This function is used for "bool" generally. The following optargs were
all unified as "bool":

- slirp/restrict: originally QEMU_OPT_STRING, net_init_slirp() accepting
all of "on|yes|y|off|no|n"
- tap/vnet_hdr: originally QEMU_OPT_BOOL, parse_option_bool() accepting
"on|off".
- tap/vhost: ditto
- tap/vhostforce: ditto

So I took the union (nothing should break that used to work).

The leading comment rather means that the structure of
parse_option_bool() is followed:
- optarg values meaning "true": true
- optarg values meaning "false": false
- other optarg values: error
- no optarg value at all: true


>> +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); <---------
> [...]
>

*groan*

You're right. opts_do_parse() makes an exception with "id" and doesn't
call opt_set() for any occurrence of it. Would you accept the attached
fix, split up and squashed into previous parts appropriately?

Thanks!
Laszlo

[-- Attachment #2: 0001-support-NetLegacy-id-and-clean-up-QemuOpts-id-usage.patch --]
[-- Type: text/plain, Size: 4686 bytes --]

>From 6839ead5ac4a77ac82aee2fe1365e72e276aa89d Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 13 Jul 2012 23:49:09 +0200
Subject: [PATCH] support NetLegacy::id and clean up QemuOpts::id usage

NetLegacy::id is actually allowed and takes precedence over
NetLegacy::name.

Factor opts_visitor_insert() out of opts_start_struct() and call it
separately for opts_root->id if there's any.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 net.c               |    2 +-
 qapi-schema.json    |    5 ++++-
 qapi/opts-visitor.c |   49 +++++++++++++++++++++++++++++++++++++------------
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/net.c b/net.c
index 1612f64..dbca77b 100644
--- a/net.c
+++ b/net.c
@@ -869,7 +869,7 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
         u.net = object;
         opts = u.net->opts;
         /* missing optional values have been initialized to "all bits zero" */
-        name = u.net->name;
+        name = u.net->has_id ? u.net->id : u.net->name;
     }
 
     if (net_client_init_fun[opts->kind]) {
diff --git a/qapi-schema.json b/qapi-schema.json
index ed345ee..cc48127 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2116,7 +2116,9 @@
 #
 # @vlan: #optional vlan number
 #
-# @name: #optional identifier for monitor commands
+# @id: #optional identifier for monitor commands
+#
+# @name: #optional identifier for monitor commands, ignored if @id is present
 #
 # @traits: device type specific properties (legacy)
 #
@@ -2125,6 +2127,7 @@
 { 'type': 'NetLegacy',
   'data': {
     '*vlan': 'int32',
+    '*id':   'str',
     '*name': 'str',
     'opts':  'NetClientOptions' } }
 
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 9187c86..a261cf3 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -35,6 +35,12 @@ struct OptsVisitor
      * schema, with a single mandatory scalar member. */
     GQueue *repeated_opts;
     bool repeated_opts_first;
+
+    /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for
+     * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does
+     * not survive or escape the OptsVisitor object.
+     */
+    QemuOpt *fake_id_opt;
 };
 
 
@@ -46,6 +52,27 @@ destroy_list(gpointer list)
 
 
 static void
+opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt)
+{
+    GQueue *list;
+
+    list = g_hash_table_lookup(unprocessed_opts, opt->name);
+    if (list == NULL) {
+        list = g_queue_new();
+
+        /* GHashTable will never try to free the keys -- we supply NULL as
+         * "key_destroy_func" in opts_start_struct(). Thus cast away key
+         * const-ness in order to suppress gcc's warning.
+         */
+        g_hash_table_insert(unprocessed_opts, (gpointer)opt->name, list);
+    }
+
+    /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
+    g_queue_push_tail(list, (gpointer)opt);
+}
+
+
+static void
 opts_start_struct(Visitor *v, void **obj, const char *kind,
                   const char *name, size_t size, Error **errp)
 {
@@ -60,21 +87,18 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
     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;
+        /* ensured by qemu-option.c::opts_do_parse() */
+        assert(strcmp(opt->name, "id") != 0);
 
-        list = g_hash_table_lookup(ov->unprocessed_opts, opt->name);
-        if (list == NULL) {
-            list = g_queue_new();
+        opts_visitor_insert(ov->unprocessed_opts, opt);
+    }
 
-            /* 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);
-        }
+    if (ov->opts_root->id != NULL) {
+        ov->fake_id_opt = g_malloc0(sizeof *ov->fake_id_opt);
 
-        /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
-        g_queue_push_tail(list, (gpointer)opt);
+        ov->fake_id_opt->name = "id";
+        ov->fake_id_opt->str = ov->opts_root->id;
+        opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
     }
 }
 
@@ -390,6 +414,7 @@ opts_visitor_cleanup(OptsVisitor *ov)
     if (ov->unprocessed_opts != NULL) {
         g_hash_table_destroy(ov->unprocessed_opts);
     }
+    g_free(ov->fake_id_opt);
     memset(ov, '\0', sizeof *ov);
 }
 
-- 
1.7.1