net: Complete qapi-fication of netdev_add

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.  netdev_add() was a
trivial wrapper around net_client_init(), which did a few steps prior
to calling net_client_init1(); with this patch, we now skip directly
to net_client_init1().  In addition to fixing array parameters, the
following additional differences occur:

-  {"execute": "netdev_add", "arguments": {"type": "help"}}
no longer attempts to print help to stdout and exit.  Bug fix, broken
in 547203ead4 'net: List available netdevs with "-netdev help"',
v2.12.0.

-  {"execute": "netdev_add", "arguments': {... "ipv6-net": "..." }}
no longer attempts to desugar the undocumented ipv6-net magic string
into the proper "ipv6-prefix" and "ipv6-prefixlen".  Undocumented
misfeature, introduced in commit 7aac531ef2 "qapi-schema, qemu-options
& slirp: Adding Qemu options for IPv6 addresses", v2.6.0.

-  {'execute':'netdev_add',
     'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}}
   {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'hubid', expected: integer"}}
Used to succeed: since our command line treats everything as strings,
our not-so-round-trip conversion from QAPI -> QemuOpts -> QAPI lost
the original typing and turned everything into a string; now that we
skip the QemuOpts, the JSON input has to match the exact QAPI type.
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; a separate patch will
address the abuse of QemuOpts as a witness for whether a
NetClientState is a netdev.  In the meantime, our argument that we are
okay requires auditing all uses of option group "netdev":

- qemu_netdev_opts: option group definition, empty .desc[]
- CLI (CLI netdev parsing ends before monitors start, so while
  monitors can mess with CLI netdevs, CLI cannot mess with
  monitor netdevs):
  - main() case QEMU_OPTION_netdev: store CLI definition
  - main() case QEMU_OPTION_readconfig, case QEMU_OPTION_writeconfig:
  similar, dealing only with CLI
  - net_init_clients(): Pass CLI to net_client_init()
- Monitor:
  - hmp_netdev_add(): straightforward parse into net_client_init()
  - qmp_netdev_add(): subject of this patch, used to add full
  object to option group, now just adds bare-bones id
  - qmp_netdev_del(), netdev_del_completion(): check the option group
  solely for id, as a 'is this a netdev' predicate

Reported-by: Alex Kirillov <lekiravi@yandex-team.ru>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200317201711.322764-2-eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Commit message typo fixed]
Signed-off-by: Markus Armbruster <armbru@redhat.com>

diff --git a/include/net/net.h b/include/net/net.h
index e175ba9677dc09402bdc99f90fa2b5edfc4942fa..96e6eae8176ed9d497fa19c3964cda8477fe491a 100644 (file)
--- 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 c3bc34c099dd95f60b8d9d3f6a3186f6763cf7c5..41a86e7012a185f620f8eaeb0fe48e0be7d729a3 100644 (file)
--- 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 9e93c3f8a1e24a7471c815578ffdd44692b79754..a2065aabede2af169b98d4f46be3557e9d629fbd 100644 (file)
--- 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);
     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;

diff --git a/qapi/net.json b/qapi/net.json
index 1cb9a7d782b4ea89fbe0afe339d34125c06c6d96..cebb1b52e3b1824326ecc0d3cd70a3afa0d632c8 100644 (file)
--- 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: