Re: [RFC PATCH 2/6] qapi: net: add socket-ng netdev

[Stefano Brivio <sbrivio@redhat.com>]

On Mon,  9 May 2022 19:36:14 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> 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, multicast is detected
> according to the IP address type.
> "listen" and "connect" modes are changed to "server" and "client".
> 
> As qemu_opts_parse_noisily() flattens the QAPI structures ("type" field
> of Netdev structure collides with "type" field of SocketAddress),
> we detect socket-ng backend and use directly visit_type_Netdev() to
> parse the backend parameters.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> net: socket-ng: replace mode=unicast/multicast by mode=dgram
> 
> The multicast mode is detected according to the remote
> address type.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  hmp-commands.hx |   2 +-
>  net/clients.h   |   3 +
>  net/hub.c       |   1 +
>  net/meson.build |   1 +
>  net/net.c       |  61 ++++
>  net/socket-ng.c | 890 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/net.json   |  41 ++-
>  7 files changed, 996 insertions(+), 3 deletions(-)
>  create mode 100644 net/socket-ng.c
> 
> [...]
>
> +static int net_socketng_connect_init(NetClientState *peer,
> +                                   const char *model,
> +                                   const char *name,
> +                                   SocketAddress *addr,
> +                                   Error **errp)
> +{
> +    NetSocketNGState *s;
> +    int fd, connected, ret;
> +    gchar *info_str;
> +
> +    switch (addr->type) {
> +    case SOCKET_ADDRESS_TYPE_INET: {
> +        struct sockaddr_in saddr_in;
> +
> +        if (convert_host_port(&saddr_in, addr->u.inet.host, addr->u.inet.port,
> +                              errp) < 0) {
> +            return -1;
> +        }
> +
> +        fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> +        if (fd < 0) {
> +            error_setg_errno(errp, errno, "can't create stream socket");
> +            return -1;
> +        }
> +        qemu_socket_set_nonblock(fd);
> +
> +        connected = 0;
> +        for (;;) {
> +            ret = connect(fd, (struct sockaddr *)&saddr_in, sizeof(saddr_in));
> +            if (ret < 0) {
> +                if (errno == EINTR || errno == EWOULDBLOCK) {
> +                    /* continue */
> +                } else if (errno == EINPROGRESS ||
> +                           errno == EALREADY ||
> +                           errno == EINVAL) {

I guess we should report failure and close the socket on EINVAL, there
are no chances the connection will eventually succeed. I actually
proposed this change (after some debugging frustration) in my quick and
dirty series to get AF_UNIX sockets working:

	https://lore.kernel.org/all/a6d475147682de1fe3b14eb325f4247e013e8440.1619190878.git.sbrivio@redhat.com/

> +                    break;
> +                } else {
> +                    error_setg_errno(errp, errno, "can't connect socket");
> +                    closesocket(fd);
> +                    return -1;
> +                }
>
> [...]
>
> diff --git a/qapi/net.json b/qapi/net.json
> index b92f3f5fb444..2b31101e6641 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -7,6 +7,7 @@
>  ##
>  
>  { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>  
>  ##
>  # @set_link:
> @@ -452,6 +453,40 @@
>      '*vhostdev':     'str',
>      '*queues':       'int' } }
>  
> +##
> +# @NetdevSocketNGMode:
> +#
> +# @dgram: UDP mode
> +#
> +# @server: TCP server mode
> +#
> +# @client: TCP client mode
> +#
> +# Legacy NetdevSocketOptions only accepts one of:
> +# "fd", "udp", "mcast" and "udp".
> +# we map:
> +#   "udp", "mcast" -> "dgram"
> +#   "listen"       -> "server"
> +#   "connect"      -> "client"
> +#
> +# Since: 7.1
> +#
> +##
> +{ 'enum': 'NetdevSocketNGMode',
> +  'data':  [ 'dgram', 'server', 'client' ] }
> +
> +##
> +# @NetdevSocketNGOptions:
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'NetdevSocketNGOptions',
> +  'data': {
> +    'mode':    'NetdevSocketNGMode',
> +    '*addr':   'SocketAddress',
> +    '*remote': 'SocketAddress',
> +    '*local':  'SocketAddress' } }
> +
>  ##
>  # @NetClientDriver:
>  #
> @@ -463,7 +498,8 @@
>  ##
>  { 'enum': 'NetClientDriver',
>    'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
> -            'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa' ] }
> +            'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa',
> +            'socket-ng' ] }

I don't know if this is an issue, but I couldn't figure out the reason
for this difference either:

- with the old socket option, I can pass something like:

    -net socket,fd=5 -net nic,model=virtio

  and frames are sent to/received from socket number 5 (which I
  pre-opened)

- with the new option, and something like:

    -netdev socket-ng,id=socket0,mode=client,addr.type=unix,addr.path=/tmp/test.socket -net nic,model=virtio,id=hostnet0

  I get a connection on the socket, but the virtio-net device is not
  connected to it (no frames received/sent).

  However, if instead of "-net nic" I pass this:

    -device virtio-net-pci,netdev=socket0

  everything works.

-- 
Stefano