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

[Laurent Vivier <lvivier@redhat.com>]

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