Re: [RFC PATCH v2 3/8] qapi: net: add stream and dgram netdevs

[Markus Armbruster <armbru@redhat.com>]

Laurent Vivier <lvivier@redhat.com> writes:

> On 15/06/2022 13:46, Markus Armbruster wrote:
>> Laurent Vivier <lvivier@redhat.com> writes:
>> 
>>> On 13/05/2022 13:44, Markus Armbruster wrote:
>>>> Laurent Vivier <lvivier@redhat.com> writes:
>>>>
>>>>> Copied from socket netdev file and modified to use SocketAddress
>>>>> to be able to introduce new features like unix socket.
>>>>>
>>>>> "udp" and "mcast" are squashed into dgram netdev, multicast is detected
>>>>> according to the IP address type.
>>>>> "listen" and "connect" modes are managed by stream netdev. An optional
>>>>> parameter "server" defines the mode (server by default)
>>>>>
>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>> ---
>>>>>    hmp-commands.hx |   2 +-
>>>>>    net/clients.h   |   6 +
>>>>>    net/dgram.c     | 630 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    net/hub.c       |   2 +
>>>>>    net/meson.build |   2 +
>>>>>    net/net.c       |  24 +-
>>>>>    net/stream.c    | 425 ++++++++++++++++++++++++++++++++
>>>>>    qapi/net.json   |  38 ++-
>>>>>    8 files changed, 1125 insertions(+), 4 deletions(-)
>>>>>    create mode 100644 net/dgram.c
>>>>>    create mode 100644 net/stream.c
>>>>>
> ...
>>>>> diff --git a/net/net.c b/net/net.c
>>>>> index 2aab7167316c..fd6b30a10c57 100644
>>>>> --- a/net/net.c
>>>>> +++ b/net/net.c
>>>>> @@ -1015,6 +1015,8 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>>>>>    #endif
>>>>>            [NET_CLIENT_DRIVER_TAP]       = net_init_tap,
>>>>>            [NET_CLIENT_DRIVER_SOCKET]    = net_init_socket,
>>>>> +        [NET_CLIENT_DRIVER_STREAM]    = net_init_stream,
>>>>> +        [NET_CLIENT_DRIVER_DGRAM]     = net_init_dgram,
>>>>>    #ifdef CONFIG_VDE
>>>>>            [NET_CLIENT_DRIVER_VDE]       = net_init_vde,
>>>>>    #endif
>>>>> @@ -1097,6 +1099,8 @@ void show_netdevs(void)
>>>>>        int idx;
>>>>>        const char *available_netdevs[] = {
>>>>>            "socket",
>>>>> +        "stream",
>>>>> +        "dgram",
>>>>>            "hubport",
>>>>>            "tap",
>>>>>    #ifdef CONFIG_SLIRP
>>>>> @@ -1606,7 +1610,25 @@ int net_init_clients(Error **errp)
>>>>>     */
>>>>>    static bool netdev_is_modern(const char *optarg)
>>>>>    {
>>>>> -    return false;
>>>>> +    static QemuOptsList dummy_opts = {
>>>>> +        .name = "netdev",
>>>>> +        .implied_opt_name = "type",
>>>>> +        .head = QTAILQ_HEAD_INITIALIZER(dummy_opts.head),
>>>>> +        .desc = { { } },
>>>>> +    };
>>>>> +    const char *netdev;
>>>>> +    QemuOpts *opts;
>>>>> +    bool is_modern;
>>>>> +
>>>>> +    opts = qemu_opts_parse(&dummy_opts, optarg, true, &error_fatal);
>>>>> +    netdev = qemu_opt_get(opts, "type");
>>>>> +
>>>>> +    is_modern = strcmp(netdev, "stream") == 0 ||
>>>>> +                strcmp(netdev, "dgram") == 0;
>>>>
>>>> Crashes when user neglects to pass "type".
>>>
>>> I think "type" is always passed because of the '.implied_opt_name = "type"'. Am I wrong?
>> 
>> .implied_opt_name = "type" lets you shorten "type=T,..." to "T,...".  It
>> doesn't make key "type" mandatory.  "-netdev id=foo" is still permitted.
>> Even "-netdev ''" is.
>
>
> In fact type is checked before by QAPI definition:
>
> { 'union': 'Netdev',
>    'base': { 'id': 'str', 'type': 'NetClientDriver' },
>    'discriminator': 'type',
> ...
>
> As it's the discriminator it must be there.
>
>    $ qemu-system-x86_64 -netdev id=foo
>    qemu-system-x86_64: -netdev id=foo: Parameter 'type' is missing

It does crash for me:

    (gdb) bt
    #0  0x00007ffff4d25dcb in __strcmp_avx2 () at /lib64/libc.so.6
    #1  0x0000555555b4574b in netdev_is_modern (optarg=0x7fffffffe2ae "id=foo")
        at ../net/net.c:1626
    #2  0x0000555555b457ad in net_client_parse
        (opts_list=0x555556563780 <qemu_netdev_opts>, optarg=0x7fffffffe2ae "id=foo") at ../net/net.c:1636
    #3  0x0000555555ad98de in qemu_init (argc=3, argv=0x7fffffffdf08, envp=0x0)
        at ../softmmu/vl.c:2901
    #4  0x0000555555842c01 in qemu_main (argc=3, argv=0x7fffffffdf08, envp=0x0)
        at ../softmmu/main.c:35
    #5  0x0000555555842c37 in main (argc=3, argv=0x7fffffffdf08)
        at ../softmmu/main.c:45
    (gdb) up
    #1  0x0000555555b4574b in netdev_is_modern (optarg=0x7fffffffe2ae "id=foo")
        at ../net/net.c:1626
    1626	    is_modern = strcmp(netdev, "stream") == 0 ||
    (gdb) p netdev
    $1 = 0x0

This is

     https://github.com/patchew-project/qemu tags/patchew/20220512080932.735962-1-lvivier@redhat.com 

I suspect you tested with your v3, which doesn't crash for me, either.

[...]