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

[Laurent Vivier <lvivier@redhat.com>]

On 20/06/2022 13:22, Markus Armbruster wrote:
> 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.
> 

Yes, thank you. So this is fixed now :)

Laurent