Re: [PATCH] net: Complete qapi-fication of netdev_add

[Markus Armbruster <armbru@redhat.com>]

It's too late in my day for a full review.  Just one observation for
now.

Eric Blake <eblake@redhat.com> writes:

> We've had all the required pieces for doing a type-safe representation
> of netdev_add as a flat union for quite some time now (since
> 0e55c381f6 in v2.7.0, released in 2016), but did not make the final
> switch to using it because of concern about whether a command-line
> regression in accepting "1" in place of 1 for integer arguments would
> be problematic.  Back then, we did not have the deprecation cycle to
> allow us to make progress.  But now that we have waited so long, other
> problems have crept in: for example, our desire to add
> qemu-storage-daemon is hampered by the inability to express net
> objects, and we are unable to introspect what we actually accept.
> Additionally, our round-trip through QemuOpts silently eats any
> argument that expands to an array, rendering dnssearch, hostfwd, and
> guestfwd useless through QMP:
>
> {"execute": "netdev_add", "arguments": { "id": "netdev0",
>   "type": "user", "dnssearch": [
>     { "str": "8.8.8.8" }, { "str": "8.8.4.4" }
>   ]}}
>
> So without further ado, let's turn on proper QAPI.
>
> There are a few places where the QMP 'netdev_add' command is now
> more strict: anywhere that the QAPI lists an integer member, we
> now require a strict JSON integer (previously, we allowed both
> integers and strings, because the conversion from QMP to QemuOpts
> back to QObject collapsed them into integers).  For example,
> pre-patch, both of these examples succeed, but post-patch, the
> second example now fails:
>
> {'execute':'netdev_add',
>   'arguments':{'id':'net1', 'type':'hubport', 'hubid':1}}
> {"return": {}}
> {'execute':'netdev_add',
>   'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}}
> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'hubid', expected: integer"}}
>
> But this stricter QMP is desirable, and introspection is sufficient
> for any affected applications to make sure they use it correctly.
>
> In qmp_netdev_add(), we still have to create a QemuOpts object
> so that qmp_netdev_del() will be able to remove a hotplugged
> network device; but the opts->head remains empty since we now
> manage all parsing through the QAPI object rather than QemuOpts.
>
> Reported-by: Alex Kirillov <lekiravi@yandex-team.ru>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/net.json     | 14 +-------------
>  include/net/net.h |  1 -
>  monitor/misc.c    |  2 --
>  net/net.c         |  6 +++---
>  4 files changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 1cb9a7d782b4..cebb1b52e3b1 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -39,18 +39,8 @@
>  #
>  # Add a network backend.
>  #
> -# @type: the type of network backend. Possible values are listed in
> -#        NetClientDriver (excluding 'none' and 'nic')
> -#
> -# @id: the name of the new network backend
> -#
>  # Additional arguments depend on the type.
>  #
> -# TODO: This command effectively bypasses QAPI completely due to its
> -#       "additional arguments" business.  It shouldn't have been added to
> -#       the schema in this form.  It should be qapified properly, or
> -#       replaced by a properly qapified command.
> -#
>  # Since: 0.14.0
>  #
>  # Returns: Nothing on success
> @@ -64,9 +54,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'netdev_add',
> -  'data': {'type': 'str', 'id': 'str'},
> -  'gen': false }                # so we can get the additional arguments
> +{ 'command': 'netdev_add', 'data': 'Netdev', 'boxed': true }
>
>  ##
>  # @netdev_del:
> diff --git a/include/net/net.h b/include/net/net.h
> index e175ba9677dc..96e6eae8176e 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -203,7 +203,6 @@ void net_cleanup(void);
>  void hmp_host_net_add(Monitor *mon, const QDict *qdict);
>  void hmp_host_net_remove(Monitor *mon, const QDict *qdict);
>  void netdev_add(QemuOpts *opts, Error **errp);
> -void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp);
>
>  int net_hub_id_for_client(NetClientState *nc, int *id);
>  NetClientState *net_hub_port_find(int hub_id);
> diff --git a/monitor/misc.c b/monitor/misc.c
> index c3bc34c099dd..41a86e7012a1 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -247,8 +247,6 @@ static void monitor_init_qmp_commands(void)
>                           qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
>      qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
>                           QCO_NO_OPTIONS);
> -    qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
> -                         QCO_NO_OPTIONS);
>      qmp_register_command(&qmp_commands, "object-add", qmp_object_add,
>                           QCO_NO_OPTIONS);
>
> diff --git a/net/net.c b/net/net.c
> index 9e93c3f8a1e2..a2065aabede2 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1170,7 +1170,7 @@ void netdev_add(QemuOpts *opts, Error **errp)
>      net_client_init(opts, true, errp);
>  }
>
> -void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp)
> +void qmp_netdev_add(Netdev *netdev, Error **errp)
>  {
>      Error *local_err = NULL;
>      QemuOptsList *opts_list;
> @@ -1181,12 +1181,12 @@ void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp)
>          goto out;
>      }
>
> -    opts = qemu_opts_from_qdict(opts_list, qdict, &local_err);
> +    opts = qemu_opts_create(opts_list, netdev->id, 1, &local_err);

As oyu explain in your commit message, we still create a QemuOpts with
the same ID, but no other parameters, because qmp_netdev_del() needs
it.  I show it below.

>      if (local_err) {
>          goto out;
>      }
>
> -    netdev_add(opts, &local_err);
> +    net_client_init1(netdev, true, &local_err);
>      if (local_err) {
>          qemu_opts_del(opts);
>          goto out;
       }

   out:
       error_propagate(errp, local_err);
   }

   void qmp_netdev_del(const char *id, Error **errp)
   {
       NetClientState *nc;
       QemuOpts *opts;

       nc = qemu_find_netdev(id);
       if (!nc) {
           error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                     "Device '%s' not found", id);
           return;
       }

       opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id);
--->   if (!opts) {
           error_setg(errp, "Device '%s' is not a netdev", id);
           return;
       }

       qemu_del_net_client(nc);
       qemu_opts_del(opts);
   }

We better check all other uses of option group "netdev".