From f0ccc00be16e6f4c925f90305e2d4ed9dd11c8fd Mon Sep 17 00:00:00 2001 From: Marc-André Lureau Date: Mon, 16 Mar 2020 18:18:24 +0100 Subject: qmp: constify QmpCommand and list Since 0b69f6f72ce47a37a749b056b6d5ec64c61f11e8 "qapi: remove qmp_unregister_command()", the command list can be declared const. Signed-off-by: Marc-André Lureau Reviewed-by: Damien Hedde Message-Id: <20200316171824.2319695-1-marcandre.lureau@redhat.com> [Rebased] Signed-off-by: Markus Armbruster --- include/qapi/qmp/dispatch.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'include') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 9aa426a398..5a9cf82472 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -39,7 +39,8 @@ typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList; void qmp_register_command(QmpCommandList *cmds, const char *name, QmpCommandFunc *fn, QmpCommandOptions options); -QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name); +const QmpCommand *qmp_find_command(const QmpCommandList *cmds, + const char *name); void qmp_disable_command(QmpCommandList *cmds, const char *name); void qmp_enable_command(QmpCommandList *cmds, const char *name); @@ -47,13 +48,13 @@ bool qmp_command_is_enabled(const QmpCommand *cmd); const char *qmp_command_name(const QmpCommand *cmd); bool qmp_has_success_response(const QmpCommand *cmd); QDict *qmp_error_response(Error *err); -QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request, +QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, bool allow_oob); bool qmp_is_oob(const QDict *dict); -typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque); +typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque); -void qmp_for_each_command(QmpCommandList *cmds, qmp_cmd_callback_fn fn, +void qmp_for_each_command(const QmpCommandList *cmds, qmp_cmd_callback_fn fn, void *opaque); #endif -- cgit v1.2.3-55-g7522 From db2a380c84574d8c76d7193b8af8535234fe5156 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 17 Mar 2020 15:17:10 -0500 Subject: 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 Signed-off-by: Eric Blake Message-Id: <20200317201711.322764-2-eblake@redhat.com> Reviewed-by: Markus Armbruster [Commit message typo fixed] Signed-off-by: Markus Armbruster --- include/net/net.h | 1 - monitor/misc.c | 2 -- net/net.c | 6 +++--- qapi/net.json | 14 +------------- 4 files changed, 4 insertions(+), 19 deletions(-) (limited to 'include') diff --git a/include/net/net.h b/include/net/net.h index e175ba9677..96e6eae817 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 c3bc34c099..41a86e7012 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 9e93c3f8a1..a2065aabed 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); 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 1cb9a7d782..cebb1b52e3 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: -- cgit v1.2.3-55-g7522 From 08712fcb851034228b61f75bd922863a984a4f60 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 17 Mar 2020 15:17:11 -0500 Subject: net: Track netdevs in NetClientState rather than QemuOpt As mentioned in the previous patch, our use of QemuOpt group "netdev" has two purposes: collect the CLI arguments, and serve as a witness for monitor hotplug actions. As the latter didn't use anything but an id, it felt rather unclean to have to touch QemuOpts at all when going through QMP, so let's instead track things with a bool field in NetClientState. Suggested-by: Markus Armbruster Signed-off-by: Eric Blake Message-Id: <20200317201711.322764-3-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/net/net.h | 1 + monitor/misc.c | 4 +--- net/net.c | 37 +++++++++++-------------------------- 3 files changed, 13 insertions(+), 29 deletions(-) (limited to 'include') diff --git a/include/net/net.h b/include/net/net.h index 96e6eae817..094e966af9 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -98,6 +98,7 @@ struct NetClientState { unsigned rxfilter_notify_enabled:1; int vring_enable; int vnet_hdr_len; + bool is_netdev; QTAILQ_HEAD(, NetFilterState) filters; }; diff --git a/monitor/misc.c b/monitor/misc.c index 41a86e7012..6c45fa490f 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -2035,13 +2035,11 @@ void netdev_del_completion(ReadLineState *rs, int nb_args, const char *str) count = qemu_find_net_clients_except(NULL, ncs, NET_CLIENT_DRIVER_NIC, MAX_QUEUE_NUM); for (i = 0; i < MIN(count, MAX_QUEUE_NUM); i++) { - QemuOpts *opts; const char *name = ncs[i]->name; if (strncmp(str, name, len)) { continue; } - opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), name); - if (opts) { + if (ncs[i]->is_netdev) { readline_add_completion(rs, name); } } diff --git a/net/net.c b/net/net.c index a2065aabed..38778e831d 100644 --- a/net/net.c +++ b/net/net.c @@ -1060,6 +1060,15 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp) } return -1; } + + if (is_netdev) { + NetClientState *nc; + + nc = qemu_find_netdev(netdev->id); + assert(nc); + nc->is_netdev = true; + } + return 0; } @@ -1172,34 +1181,12 @@ void netdev_add(QemuOpts *opts, Error **errp) void qmp_netdev_add(Netdev *netdev, Error **errp) { - Error *local_err = NULL; - QemuOptsList *opts_list; - QemuOpts *opts; - - opts_list = qemu_find_opts_err("netdev", &local_err); - if (local_err) { - goto out; - } - - opts = qemu_opts_create(opts_list, netdev->id, 1, &local_err); - if (local_err) { - goto out; - } - - net_client_init1(netdev, true, &local_err); - if (local_err) { - qemu_opts_del(opts); - goto out; - } - -out: - error_propagate(errp, local_err); + net_client_init1(netdev, true, errp); } void qmp_netdev_del(const char *id, Error **errp) { NetClientState *nc; - QemuOpts *opts; nc = qemu_find_netdev(id); if (!nc) { @@ -1208,14 +1195,12 @@ void qmp_netdev_del(const char *id, Error **errp) return; } - opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id); - if (!opts) { + if (!nc->is_netdev) { error_setg(errp, "Device '%s' is not a netdev", id); return; } qemu_del_net_client(nc); - qemu_opts_del(opts); } static void netfilter_print_info(Monitor *mon, NetFilterState *nf) -- cgit v1.2.3-55-g7522