Re: [PATCH v2 1/2] net: Complete qapi-fication of netdev_add

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>

Thanks for the nice commit message.

Reviewed-by: Markus Armbruster <address@hidden>