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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4101 bytes --]

On Wed, Oct 05, 2022 at 12:08:27PM +0200, Laurent Vivier wrote:
> 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.

Ok, that makes sense to me.  Or at least, if it's not correct we
should fix it later for all the places at the same time in the qio
code.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]