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. 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': {... "ip6-net": "..." }}
> no longer attempts to desugar the undocumented ip6-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 <lekir...@yandex-team.ru>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Thanks for the nice commit message.
Reviewed-by: Markus Armbruster <arm...@redhat.com>
