Re: [PATCH v12 15/17] net: stream: move to QIO to enable additional parameters

[Laurent Vivier <lvivier@xxxxxxxxxx>]

On 10/20/22 15:09, Philippe Mathieu-Daudé wrote:
> On 20/10/22 11:16, Laurent Vivier wrote:
> > Use QIOChannel, QIOChannelSocket and QIONetListener.
> > This allows net/stream to use all the available parameters provided by
> > SocketAddress.
> >
> > Signed-off-by: Laurent Vivier <lvivier@xxxxxxxxxx>
> > Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > ---
> >   meson           |   2 +-
> >   net/stream.c    | 493 +++++++++++++++++-------------------------------
> >   qemu-options.hx |   4 +-
> >   3 files changed, 180 insertions(+), 319 deletions(-)
> >   static int net_stream_server_init(NetClientState *peer,
> > @@ -283,105 +287,61 @@ static int net_stream_server_init(NetClientState *peer,
> >   {
> >       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;
> > -        }
> > +    QIOChannelSocket *listen_sioc = qio_channel_socket_new();
> > -        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);
> > +    nc = qemu_new_net_client(&net_stream_info, peer, model, name);
> > +    s = DO_UPCAST(NetStreamState, nc, nc);
> > -        socket_set_fast_reuse(fd);
> > +    s->listen_ioc = QIO_CHANNEL(listen_sioc);
> > +    qio_channel_socket_listen_async(listen_sioc, addr, 0,
> > +                                    net_stream_server_listening, s,
> > +                                    NULL, NULL);
> > -        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_UNIX: {
> > -        struct sockaddr_un saddr_un;
> > -
> > -        ret = unlink(addr->u.q_unix.path);
> > -        if (ret < 0 && errno != ENOENT) {
> > -            error_setg_errno(errp, errno, "failed to unlink socket %s",
> > -                             addr->u.q_unix.path);
> > -            return -1;
> > -        }
> > +    return 0;
> > +}
> > -        saddr_un.sun_family = PF_UNIX;
> > -        ret = snprintf(saddr_un.sun_path, sizeof(saddr_un.sun_path), "%s",
> > -                       addr->u.q_unix.path);
> > -        if (ret < 0 || ret >= sizeof(saddr_un.sun_path)) {
> > -            error_setg(errp, "UNIX socket path '%s' is too long",
> > -                       addr->u.q_unix.path);
> > -            error_append_hint(errp, "Path must be less than %zu bytes\n",
> > -                              sizeof(saddr_un.sun_path));
> > -            return -1;
> > -        }
> > +static void net_stream_client_connected(QIOTask *task, gpointer opaque)
> > +{
> > +    NetStreamState *s = opaque;
> > +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(s->ioc);
> > +    SocketAddress *addr;
> > +    gchar *uri;
> > +    int ret;
> > -        fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
> > -        if (fd < 0) {
> > -            error_setg_errno(errp, errno, "can't create stream socket");
> > -            return -1;
> > -        }
> > -        qemu_socket_set_nonblock(fd);
> > -
> > -        ret = bind(fd, (struct sockaddr *)&saddr_un, sizeof(saddr_un));
> > -        if (ret < 0) {
> > -            error_setg_errno(errp, errno, "can't create socket with path: %s",
> > -                             saddr_un.sun_path);
> > -            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;
> > +    if (sioc->fd < 0) {
> > +        qemu_set_info_str(&s->nc, "connection error");
> > +        goto error;
> >       }
> > -    ret = listen(fd, 0);
> > -    if (ret < 0) {
> > -        error_setg_errno(errp, errno, "can't listen on socket");
> > -        closesocket(fd);
> > -        return -1;
> > +    addr = qio_channel_socket_get_remote_address(sioc, NULL);
> > +    g_assert(addr != NULL);
> Please use:
>
>         addr = qio_channel_socket_get_remote_address(sioc, &error_fatal);
>
> Which is more verbose in case of error, i.e.:
>
>    qemu-system-x86_64: socket family 0 unsupported
>
> Instead of:
>
>    ERROR:../../net/stream.c:321:net_stream_client_connected: assertion failed: 
> (addr != NULL)
I put an assert() here because I consider to have NULL here is a programming error, not an 
user error.
"It should not happen".

Thanks,
Laurent