| Message ID | 1467514729-29366-15-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > We finally have all the required pieces for doing a type-safe > representation of netdev_add as a flat union, where the > discriminator 'type' now selects which additional members may > appear in the "arguments" JSON object sent over QMP, and exposes > those types through introspection, and without breaking command > line parsing. > > Inline the function netdev_add() into its lone remaining caller. > > 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 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"}} Needs to be covered in release notes. > If that turns out to be problematic, we could resolve the problem > by either adding a loose parsing mode to qmp-input-visitor (teaching > it to accept a string encoding of an integer in place of an actual > integer), or by using QAPI alternates. But that work should be > in followup patches, if at all. The sloppy typing is an accidental misfeature that is at odds with QMP's design. As far as I can tell, it's not documented anywhere. But lots of things aren't documented anywhere; the questions is whether they're used. If we find we must keep netdev_add misfeature-compatible, I want it deprecated in favor of a new command that adheres to the QMP design. Do we have to keep it misfeature-compatible proactively? How comfortable are we with "wait see whether anything breaks" here? The scenarios I want to avoid are "OMG, hold the release while we improvise a misfeature-compatibility hack", and "OMG, we need a stable release for misfeature compatibility a.s.a.p." I'm okay with breaking misfeature compatibility in this patch, and unbreak it separately if necessary. > 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. This use of QemuOpts as a network backend directory has always been somewhat questionable. More so now the QemuOpts is empty. But it's not this patch's business. > Signed-off-by: Eric Blake <eblake@redhat.com>
diff --git a/qapi-schema.json b/qapi-schema.json index e8a015f..3a7632b 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2362,26 +2362,17 @@ # # Add a network backend. # -# @type: the type of network backend. Current valid values are 'user', 'tap', -# 'vde', 'socket', 'dump' and 'bridge' +# @type: the type of network backend; determines which additional arguments +# are valid. See Netdev documentation for more details. # # @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 # If @type is not a valid network backend, DeviceNotFound ## -{ 'command': 'netdev_add', - 'data': {'type': 'str', 'id': 'str'}, - 'gen': false } # so we can get the additional arguments +{ 'command': 'netdev_add', 'data': 'Netdev', 'box': true } ## # @netdev_del: diff --git a/include/net/net.h b/include/net/net.h index e8d9e9e..820f880 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -210,8 +210,6 @@ void net_check_clients(void); 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/hmp.c b/hmp.c index 4819abc..3502abf 100644 --- a/hmp.c +++ b/hmp.c @@ -1672,7 +1672,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict) goto out; } - netdev_add(opts, &err); + net_client_init(opts, true, &err); if (err) { qemu_opts_del(opts); } diff --git a/net/net.c b/net/net.c index c124b11..1bd7936 100644 --- a/net/net.c +++ b/net/net.c @@ -1181,12 +1181,7 @@ void hmp_host_net_remove(Monitor *mon, const QDict *qdict) qemu_del_net_client(nc); } -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; @@ -1197,12 +1192,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/qmp-commands.hx b/qmp-commands.hx index 6937e83..cfc4086 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -976,7 +976,7 @@ EQMP { .name = "netdev_add", .args_type = "netdev:O", - .mhandler.cmd_new = qmp_netdev_add, + .mhandler.cmd_new = qmp_marshal_netdev_add, }, SQMP
We finally have all the required pieces for doing a type-safe representation of netdev_add as a flat union, where the discriminator 'type' now selects which additional members may appear in the "arguments" JSON object sent over QMP, and exposes those types through introspection, and without breaking command line parsing. Inline the function netdev_add() into its lone remaining caller. 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 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"}} If that turns out to be problematic, we could resolve the problem by either adding a loose parsing mode to qmp-input-visitor (teaching it to accept a string encoding of an integer in place of an actual integer), or by using QAPI alternates. But that work should be in followup patches, if at all. 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. Signed-off-by: Eric Blake <eblake@redhat.com> --- v8: improve commit message, call out a change in QMP behavior, inline netdev_add() v7: no change v6: don't lose qemu_opts handling --- qapi-schema.json | 15 +++------------ include/net/net.h | 2 -- hmp.c | 2 +- net/net.c | 11 +++-------- qmp-commands.hx | 2 +- 5 files changed, 8 insertions(+), 24 deletions(-)