Re: [RFC PATCH] qapi: net: fix -set parameter with modern style

[Markus Armbruster]

Laurent Vivier <lvivier@redhat.com> writes:

> With netdev modern style, parameters cannot be found using
> qemu_find_opts_err() and then qemu_set_option() cannot find
> them to update them with the new option.
>
> To fix that, update the code to manage the modern style by
> searching the parameter in nd_queue, and update the entry
> using visit_type_Netdev_members().
>
> Fixes: f3eedcddba36 ("qapi: net: introduce a way to bypass 
> qemu_opts_parse_noisily()")
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Let me add Some more background.

A long time ago, configuration was a bunch of global variables in
various places, and CLI options and monitor commands to set them, all
using ad hoc parsers.

QemuOpts was an attempt at proper configuration infrastructure: a single
configuration repository with a rigid structure.  It is a set of named
"groups" (fixed at compile time, C type QemuOptsList).  Each group is a
set of QemuOpts.  A QemuOpts has a user-defined ID and a list of
QemuOpt.  A QemuOpt is essentially a key-value pair.  Values can be
string, uint64_t, or bool.

This is a *flat* design: GROUP.ID.NAME=VALUE.  Good enough back when it
was created in 2009, but we've long outgrown "flat".  More on that in a
jiffie.

-set operates on this repository.  -set GROUP.ID.NAME=VALUE selects the
QemuOptsList GROUP, within that the QemuOpts ID, and within that sets
the key NAME to value VALUE.

For -set to work, the QemuOpts must be created with a CLI option, and
used only after CLI parsing is complete.  This is how QemuOpts wants to
be used (but we use (and abuse) it in other ways, too).

The other way to configure a VM is of course the monitor.  QMP uses its
own infrastructure: the QAPI schema (since 2011).  Considerably more
expressive than QemuOpts: tree vs. flat, more scalar types.

There's another fundamental difference between a QemuOpts-based CLI and
the monitor.

With QemuOpts (when using it the intended way), we separate parsing from
acting.  First we parse the CLI entirely, populating the configuration
repository.  Only then do we actually create the things demanded by the
configuration.  Some errors get detected during parsing, and some during
acting.

In the monitor, however, each command acts immediately.  There is no
configuration repository.  There is simply no space for something like
-set.

There's also -readconfig: it parses a configuration file in a .INI
syntax into the configuration repository.

Over time, we grew bits of configurations QemuOpts can't express.  In
particular, QMP object-add could do things -object could not.  This led
to

commit 155b5f8b8d3d5dedd7c57e5223e822dc1b5295c8
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Fri Mar 12 14:19:21 2021 +0100

    qom: Support JSON in HMP object_add and tools --object
    
    Support JSON for --object in all tools and in HMP object_add in the same
    way as it is supported in qobject_input_visitor_new_str().
    
    Signed-off-by: Kevin Wolf <kwolf@redhat.com>
    Message-Id: <20210312131921.421023-1-kwolf@redhat.com>
    Reviewed-by: Eric Blake <eblake@redhat.com>
    Reviewed-by: Markus Armbruster <armbru@redhat.com>
    Signed-off-by: Kevin Wolf <kwolf@redhat.com>

The commit message is arguably bad: it doesn't explain why.  The point
is to make -object to work like QMP object-add when the option argument
is JSON.  Full expressive power then, no QemuOpts, and therefore no
-set and no -readconfig.  This led to:

commit 49e987695a1873a769a823604f9065aa88e00c55
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Mon May 24 06:57:52 2021 -0400

    vl: plug -object back into -readconfig
    
    Commit bc2f4fcb1d ("qom: move user_creatable_add_opts logic to vl.c
    and QAPIfy it", 2021-03-19) switched the creation of objects from
    qemu_opts_foreach to a bespoke QTAILQ in preparation for supporting JSON
    syntax in -object.
    
    Unfortunately in doing so it lost support for [object] stanzas in
    configuration files and also for "-set object.ID.KEY=VAL".  The latter
    is hard to re-establish and probably best solved by deprecating -set.
    This patch uses the infrastructure introduced by the previous two
    patches in order to parse QOM objects correctly from configuration
    files.
    
    Cc: Markus Armbruster <armbru@redhat.com>
    Cc: qemu-stable@nongnu.org
    Reviewed-by: Kevin Wolf <kwolf@redhat.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Message-Id: <20210524105752.3318299-4-pbonzini@redhat.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
>  include/net/net.h |  2 ++
>  net/net.c         | 35 +++++++++++++++++++++++++++++++++++
>  softmmu/vl.c      |  8 ++++++++
>  3 files changed, 45 insertions(+)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index 1448d00afbc6..be42ba96ee3d 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -246,6 +246,8 @@ extern const char *host_net_devices[];
>  extern NetClientStateList net_clients;
>  bool netdev_is_modern(const char *optarg);
>  void netdev_parse_modern(const char *optarg);
> +Netdev *netdev_find_modern(const char *id);
> +void netdev_set_modern(Netdev *net, const char *arg, Error **errp);
>  void net_client_parse(QemuOptsList *opts_list, const char *str);
>  void show_netdevs(void);
>  void net_init_clients(void);
> diff --git a/net/net.c b/net/net.c
> index 6492ad530e21..9c384187255b 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1624,6 +1624,41 @@ out:
>      return ret;
>  }
>  
> +Netdev *netdev_find_modern(const char *id)
> +{
> +    NetdevQueueEntry *e;
> +
> +    QSIMPLEQ_FOREACH(e, &nd_queue, entry) {
> +        if (strcmp(id, e->nd->id) == 0) {
> +            return e->nd;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +void netdev_set_modern(Netdev *net, const char *arg, Error **errp)
> +{
> +    Visitor *v;
> +    char *id = net->id;
> +    char *opts = g_strdup_printf("type=%s,id=%s,%s",
> +                                 NetClientDriver_lookup.array[net->type],
> +                                 id, arg);
> +
> +    v = qobject_input_visitor_new_str(opts, NULL, errp);
> +    if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
> +        goto out;
> +    }
> +    if (visit_type_Netdev_members(v, net, errp)) {
> +        g_free(id); /* re-allocated in visit_type_Netdev_members() */
> +        visit_check_struct(v, errp);
> +        visit_end_struct(v, NULL);
> +    }

This exploits the fact that visit_type_FOO_members() leaves alone the
members that are not in @opts.  It overwrites net->type and net->id with
identical values, and member NAME with VALUE, where NAME and value come
from -set netdev.ID.NAME=VALUE.

> +
> +out:
> +    visit_free(v);
> +    g_free(opts);
> +}
> +
>  static void netdev_init_modern(void)
>  {
>      while (!QSIMPLEQ_EMPTY(&nd_queue)) {
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 3340f63c3764..c063857867e2 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2162,6 +2162,14 @@ static void qemu_set_option(const char *str, Error 
> **errp)
>      if (!is_qemuopts_group(group)) {
>          error_setg(errp, "-set is not supported with %s", group);
>      } else {
> +        if (strcmp(group, "netdev") == 0) {
> +            Netdev *net = netdev_find_modern(id);
> +            if (net) {
> +                netdev_set_modern(net, str + strlen(group) + strlen(id) + 2,
> +                                  errp);
> +                return;
> +            }
> +        }
>          list = qemu_find_opts_err(group, errp);
>          if (list) {
>              opts = qemu_opts_find(list, id);

Do we really want to special-case -set netdev... this way?