Re: QMP netdev_add multiple dnssearch values

[Markus Armbruster]

Alex Kirillov <address@hidden> writes:

>> What exactly goes wrong? Does the QMP command fail? Does it succeed
>> but the network backend incorrectly?
>
> QMP command succesfully creates Slirp backend, but ignore whole arguments:
> - `dnssearch`
> - `hostfwd`
> - `guestfwd`

You're right, QMP command netdev_add silently ignores arguments that
aren't string, number, or bool, i.e. exactly the three you quoted.  Has
always been that way, as far as I can tell.

> As example, `dnssearch` field of `NetdevUserOptions` goes straight to the 
> function `slirp_dnssearch` (net/slirp.c), where it converts to `char **`. But 
> at this moment, this parameter is simply NULL, when I pass something 
> differrent from simple string.
>
> This is very strange, because type of this parameters is `StringList` and 
> must require something like [{"str": "a"}, {"str": "b"}].

During our push to get QMP feature-complete, we took some shortcuts.
One of them is qmp_netdev_add().

Objective back then : provide a QMP command for the existing netdev
configuration machinery net_client_init().  Due to its roots in CLI,
net_client_init() takes a QemuOpts.

Proper solution: define a QAPI schema, rewrite net_client_init() to take
the resulting QAPI type instead of QemuOpts, make existing users convert
from QemuOpts to the QAPI type, have qmp_netdev_add() take the QAPI type
as argument, and pass it to net_client_init().  Too much work.

Shortcut: use 'gen': false to bypass generated marshaling, marshal by
hand into a QemuOpts, so we can call unmodified net_client_init().  That
became commit 928059a37b "qapi: convert netdev_add".

The "marshal by hand into a QemuOpts" uses qemu_opts_from_qdict(), which
goes back to similarly shortcut QMP command device_add:

    commit 01e7f18869c9ee4c84793f4a39ec1f5f4128a0aa
    Author: Markus Armbruster <address@hidden>
    Date:   Wed Feb 10 20:15:29 2010 +0100

        qemu-option: Functions to convert to/from QDict

        The functions are somewhat restricted.  Good enough for the job at
        hand.  We'll extend them when we need more.

"Good enough" was true back then.  It wasn't true when we reused it for
netdev_add: hostfwd and guestfwd are list-valued.

We did define a QAPI schema a few months later (14aa0c2de0 "qapi schema:
add Netdev types").  net_client_init() uses it to convert from QemuOpts
to QAPI type Netdev.  This took us to the crazy pipeline we still use
today:

                            CLI, HMP
                        (key=value,...)
                               |
                               v
    QMP (JSON) -> QDict -> QemuOpts -> Netdev

We should instead use:

                          CLI, HMP
                      (key=value,...)
                             |
                             v
                          QemuOpts
                             |
                             v
    QMP (JSON) -> QDict -> Netdev

Back in 2016, Eric (cc'ed) posted patches to get us to this pipeline.
They got stuck on backward compatibility worries: the old code accepts
all parameters as JSON strings in addition to their proper type, the new
code doesn't.  Undocumented misfeature, but we chickened out anyway.

Let's reconsider.  Eric's patches break interface misuse that may or may
not exist in the field.  They fix a correct use of interface people want
to use (or Alex wouldn't have reported this bug), and they make QMP
introspection work for netdev_add.

Eric, what do you think?