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

[Laurent Vivier]

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