QMP netdev_add multiple dnssearch values

QMP netdev_add multiple dnssearch values

[Alex Kirillov]

Hi,

I'm trying to create a user (slirp) interface with several `dnssearch` values using QMP.
But every variant I pass can't do that.

According to the QAPI schema it should be like:

{
    "execute": "netdev_add",
    "arguments": {
        "id": "netdev0",
        "type": "user",
        "dnssearch": [
            {
                "str": "8.8.8.8"
            },
            {
                "str": "8.8.4.4"
            }
        ]
    }
}

I looked through code and find out that `dnssearch` is passing to the `slirp_dnssearch` (net/slirp.c),
but the only way to execute this function correctly is to pass simply string (like "example.org") to `dnssearch` OR to use command line options.


What is the correct form of QMP command that I should use?


P.S. Looks like fields `hostfwd` and `guestfwd` has the same issue.

-- 
Alex Kirillov
Yandex.Cloud

Re: QMP netdev_add multiple dnssearch values

[Markus Armbruster]

Alex Kirillov <lekiravi@yandex-team.ru> writes:

> Hi,
>
> I'm trying to create a user (slirp) interface with several `dnssearch` values using QMP.
> But every variant I pass can't do that.

What exactly goes wrong?  Does the QMP command fail?  Does it succeed
but the network backend incorrectly?

> According to the QAPI schema it should be like:
>
> {
>     "execute": "netdev_add",
>     "arguments": {
>         "id": "netdev0",
>         "type": "user",
>         "dnssearch": [
>             {
>                 "str": "8.8.8.8"
>             },
>             {
>                 "str": "8.8.4.4"
>             }
>         ]
>     }
> }
>
> I looked through code and find out that `dnssearch` is passing to the `slirp_dnssearch` (net/slirp.c),
> but the only way to execute this function correctly is to pass simply string (like "example.org") to `dnssearch` OR to use command line options.
>
>
> What is the correct form of QMP command that I should use?
>
>
> P.S. Looks like fields `hostfwd` and `guestfwd` has the same issue.
>
> -- 
> Alex Kirillov
> Yandex.Cloud

Re: QMP netdev_add multiple dnssearch values

[Alex Kirillov]

> 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`

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"}].


-- 
Alex Kirillov
Yandex.Cloud

Re: QMP netdev_add multiple dnssearch values

[Markus Armbruster]

Alex Kirillov <lekiravi@yandex-team.ru> 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 <armbru@redhat.com>
    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?

Re: QMP netdev_add multiple dnssearch values

[Eric Blake]

On 11/27/19 7:30 AM, Markus Armbruster wrote:

> "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.

That was before we had a deprecation process.  Now we do.  If we are 
still worried about it, then we should start the deprecation clock 
(squeezing it into 4.2-rc3 is risky, more likely is starting it in 5.0, 
so that we get rid of string support in 5.2).  If we are not worried 
about it, then we can just kill the misfeature in 5.0.

> 
> 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?

Yes, it's time to revive that work (I have no idea if my patches from 
back then will still rebase nicely, though).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: QMP netdev_add multiple dnssearch values

[Eric Blake]

On 11/27/19 9:49 AM, Eric Blake wrote:
> On 11/27/19 7:30 AM, Markus Armbruster wrote:
> 
>> "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.
> 
> That was before we had a deprecation process.  Now we do.  If we are 
> still worried about it, then we should start the deprecation clock 
> (squeezing it into 4.2-rc3 is risky, more likely is starting it in 5.0, 
> so that we get rid of string support in 5.2).  If we are not worried 
> about it, then we can just kill the misfeature in 5.0.

I'm leaning towards just killing the misfeature (it's a lot of glue code 
to add to support the misfeature for 5.0 and 5.1, if we're just going to 
rip it back out for 5.2), especially since introspection is enough for 
any affected clients to learn about the stricter behavior.

> 
>>
>> 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?
> 
> Yes, it's time to revive that work (I have no idea if my patches from 
> back then will still rebase nicely, though).

Now posted:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg03842.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: QMP netdev_add multiple dnssearch values

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/27/19 9:49 AM, Eric Blake wrote:
>> On 11/27/19 7:30 AM, Markus Armbruster wrote:
>>
>>> "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.
>>
>> That was before we had a deprecation process.  Now we do.  If we are
>> still worried about it, then we should start the deprecation clock
>> (squeezing it into 4.2-rc3 is risky, more likely is starting it in
>> 5.0, so that we get rid of string support in 5.2).  If we are not
>> worried about it, then we can just kill the misfeature in 5.0.
>
> I'm leaning towards just killing the misfeature (it's a lot of glue
> code to add to support the misfeature for 5.0 and 5.1, if we're just
> going to rip it back out for 5.2), especially since introspection is
> enough for any affected clients to learn about the stricter behavior.

Here's my practical argument for simply killing it without further
delay.

We have a bug we know to affect users: the one reported by Alex.

We have a misfeature users may or may not rely on: the one that made us
chicken out.

To fix the bug, we have to kill the misfeature.  Fixing it without
killing the misfeature feels impractical.  For 5.0, it's flat-out
impossible.

Deprecating the misfeature for a grace period before we kill it means
letting the bug continue to bite users for the whole grace period.

Since we lack the means to communicate "you're using a deprecate
misfeature, stop it!" to QMP clients, the deprecation grace period is
unlikely to help much.

>>> 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?
>>
>> Yes, it's time to revive that work (I have no idea if my patches
>> from back then will still rebase nicely, though).
>
> Now posted:
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg03842.html

Thanks, I'll review.