Eric Blake <ebl...@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 <lekir...@yandex-team.ru>
> Signed-off-by: Eric Blake <ebl...@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);
> 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;
netdev_add() is a trivial wrapper around net_client_init(), which is a
not-so-trivial wrapper around net_client_init1(). Calling it directly
skips what's done in net_client_init(). Let's have a look:
static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
{
gchar **substrings = NULL;
void *object = NULL;
Error *err = NULL;
int ret = -1;
Visitor *v = opts_visitor_new(opts);
const char *type = qemu_opt_get(opts, "type");
if (is_netdev && type && is_help_option(type)) {
show_netdevs();
exit(0);
Bypassing this makes QMP command
{"execute": "netdev_add", "arguments": {"type": "help"}}
fail instead of print help to stdout and exit. I consider this a bug
fix. Broken in 547203ead4 'net: List available netdevs with "-netdev
help"', v2.12.0.
Needs a mention in commit message and perhaps release notes.
} else {
/* Parse convenience option format ip6-net=fec0::0[/64] */
const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
if (ip6_net) {
char *prefix_addr;
unsigned long prefix_len = 64; /* Default 64bit prefix length.
*/
substrings = g_strsplit(ip6_net, "/", 2);
if (!substrings || !substrings[0]) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "ipv6-net",
"a valid IPv6 prefix");
goto out;
}
prefix_addr = substrings[0];
/* Handle user-specified prefix length. */
if (substrings[1] &&
qemu_strtoul(substrings[1], NULL, 10, &prefix_len))
{
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
"ipv6-prefixlen", "a number");
goto out;
}
qemu_opt_set(opts, "ipv6-prefix", prefix_addr, &error_abort);
qemu_opt_set_number(opts, "ipv6-prefixlen", prefix_len,
&error_abort);
qemu_opt_unset(opts, "ipv6-net");
}
}
Bypassing this loses argument "ip6-net", sugar for "ipv6-prefix" and
"ipv6-prefixlen". Introduced in commit 7aac531ef2 "qapi-schema,
qemu-options & slirp: Adding Qemu options for IPv6 addresses", v2.6.0.
It seems to be entirely undocumented, even for -netdev and HMP.
I'm leaning towards treating this as the culling of an accidental
misfeature.
Needs a mention in commit message and perhaps release notes.
I'm half tempted to cull it from HMP and -netdev, too.
if (is_netdev) {
visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
} else {
visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
}
Bypassing this is the point of this patch.
if (!err) {
ret = net_client_init1(object, is_netdev, &err);
}
if (is_netdev) {
qapi_free_Netdev(object);
} else {
qapi_free_NetLegacy(object);
}
out:
error_propagate(errp, err);
g_strfreev(substrings);
visit_free(v);
return ret;
}
