Re: [PATCH v9 05/16] qapi: net: add stream and dgram netdevs

[Laurent Vivier]

On 9/28/22 07:55, David Gibson wrote:
> > +static int net_stream_server_init(NetClientState *peer,
> > +                                  const char *model,
> > +                                  const char *name,
> > +                                  SocketAddress *addr,
> > +                                  Error **errp)
> > +{
> > +    NetClientState *nc;
> > +    NetStreamState *s;
> > +    int fd, ret;
> > +
> > +    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);
> > +
> > +        socket_set_fast_reuse(fd);
> > +
> > +        ret = bind(fd, (struct sockaddr *)&saddr_in, sizeof(saddr_in));
> > +        if (ret < 0) {
> > +            error_setg_errno(errp, errno, "can't bind ip=%s to socket",
> > +                             inet_ntoa(saddr_in.sin_addr));
> > +            closesocket(fd);
> > +            return -1;
> > +        }
> > +        break;
> > +    }
> > +    case SOCKET_ADDRESS_TYPE_FD:
> > +        fd = monitor_fd_param(monitor_cur(), addr->u.fd.str, errp);
> > +        if (fd == -1) {
> > +            return -1;
> > +        }
> > +        ret = qemu_socket_try_set_nonblock(fd);
> > +        if (ret < 0) {
> > +            error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
> > +                             name, fd);
> > +            return -1;
> > +        }
> > +        break;
> > +    default:
> > +        error_setg(errp, "only support inet or fd type");
> > +        return -1;
> > +    }
> > +
> > +    ret = listen(fd, 0);
> Does this make sense for a passed in fd?  If someone passes a "server"
> fd, are they likely to be passing a socket on which bind() but not
> listen() has been called?  Or one on which both bind() and listen()
> have been called?

Original code in net/socket.c doesn't manage server case with fd.

So I have checked what is done for QIO (all this code is overwritten by patch introducing 
QIO anyway):

At the end of the series, we use qio_channel_socket_listen_async() in 
net_stream_server_init(), that in the end calls socket_listen().

With SOCKET_ADDRESS_TYPE_FD we does the listen() (without bind()) with the 
following comment:

    case SOCKET_ADDRESS_TYPE_FD:
        fd = socket_get_fd(addr->u.fd.str, errp);
        if (fd < 0) {
            return -1;
        }

        /*
         * If the socket is not yet in the listen state, then transition it to
         * the listen state now.
         *
         * If it's already listening then this updates the backlog value as
         * requested.
         *
         * If this socket cannot listen because it's already in another state
         * (e.g. unbound or connected) then we'll catch the error here.
         */
        if (listen(fd, num) != 0) {
            error_setg_errno(errp, errno, "Failed to listen on fd socket");
            closesocket(fd);
            return -1;
        }
        break;

So I think we should keep the listen() in our case too.

Thanks,
Laurent