It's too late in my day for a full review. Just one observation for
now.
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);
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".
