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

[Laurent Vivier]

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

...
> > > > diff --git a/qapi/net.json b/qapi/net.json
> > > > index b92f3f5fb444..eef288886e1b 100644
> > > > --- a/qapi/net.json
> > > > +++ b/qapi/net.json
> > > > @@ -7,6 +7,7 @@
> > > >    ##
> > > >    
> > > >    { 'include': 'common.json' }
> > > > +{ 'include': 'sockets.json' }
> > > >    
> > > >    ##
> > > >    # @set_link:
> > > > @@ -452,6 +453,37 @@
> > > >        '*vhostdev':     'str',
> > > >        '*queues':       'int' } }
> > > >    
> > > > +##
> > > > +# @NetdevStreamOptions:
> > > > +#
> > > > +# Configuration info for stream socket netdev
> > > > +#
> > > > +# @addr: socket address to listen on (server=true)
> > > > +#        or connect to (server=false)
> > > > +# @server: create server socket (default: true)
> > > > +#
> > > > +# Since: 7.1
> > > > +##
> > > > +{ 'struct': 'NetdevStreamOptions',
> > > > +  'data': {
> > > > +    'addr':   'SocketAddress',
> > > > +    '*server': 'bool' } }
> > > > +
> > > > +##
> > > > +# @NetdevDgramOptions:
> > > > +#
> > > > +# Configuration info for datagram socket netdev.
> > > > +#
> > > > +# @remote: remote address
> > > > +# @local: local address
> > >
> > > Defaults?
> >
> > We can't have a default because for multicast default is remoTe, and for 
> > unicast default
> > is local.
>
> Well, the members are optional, so there must be some default behavior,
> which may or may not correspond to a single default value.  Regardless,
> what happens when a member is absent ought to be documented.

The code checks there is at least one of these options and reports an error if 
not.

if remote address is present and it's a multicast address, local address is 
optional.

otherwise local address is required and remote address is optional.
I've updated qemu-options.hx with that syntax.

Thanks,
Laurent