Re: [PATCH v5 07/12] net: stream: add unix socket

[Laurent Vivier]

On 30/06/2022 11:28, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lvivier@redhat.com) wrote:
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >   net/stream.c    | 106 +++++++++++++++++++++++++++++++++++++++++++++---
> >   qemu-options.hx |   1 +
> >   2 files changed, 102 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/stream.c b/net/stream.c
> > index e8afbaca50b6..1a2bc237707e 100644
> > --- a/net/stream.c
> > +++ b/net/stream.c
> > @@ -235,7 +235,7 @@ static NetStreamState 
> > *net_stream_fd_init_stream(NetClientState *peer,
> >   static void net_stream_accept(void *opaque)
> >   {
> >       NetStreamState *s = opaque;
> > -    struct sockaddr_in saddr;
> > +    struct sockaddr_storage saddr;
> >       socklen_t len;
> >       int fd;
> >   
> > @@ -253,9 +253,27 @@ static void net_stream_accept(void *opaque)
> >       s->fd = fd;
> >       s->nc.link_down = false;
> >       net_stream_connect(s);
> > -    snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> > -             "connection from %s:%d",
> > -             inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
> > +    switch (saddr.ss_family) {
> > +    case AF_INET: {
> > +        struct sockaddr_in *saddr_in = (struct sockaddr_in *)&saddr;
> > +
> > +        snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> > +                 "connection from %s:%d",
> > +                 inet_ntoa(saddr_in->sin_addr), ntohs(saddr_in->sin_port));
> > +        break;
> > +    }
> > +    case AF_UNIX: {
> > +        struct sockaddr_un saddr_un;
> > +
> > +        len = sizeof(saddr_un);
> > +        getsockname(s->listen_fd, (struct sockaddr *)&saddr_un, &len);
> > +        snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> > +                 "connect from %s", saddr_un.sun_path);
> > +        break;
> > +    }
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> >   }
> >   
> >   static int net_stream_server_init(NetClientState *peer,
> > @@ -295,6 +313,42 @@ static int net_stream_server_init(NetClientState *peer,
> >           }
> >           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;
> > +        }
> > +
> > +        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 here?
> > +
> > +        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) {
> > @@ -380,6 +434,48 @@ static int net_stream_client_init(NetClientState *peer,
> >                                      ntohs(saddr_in.sin_port));
> >           break;
> >       }
> > +    case SOCKET_ADDRESS_TYPE_UNIX: {
> > +        struct sockaddr_un saddr_un;
> > +
> > +        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));
>
> and here
>
> (Can you not do a lot of this just by calling util/qemu-sockets.c
> unix_connect?)

Thank you for your comments.

In fact, in the last patch, I replace all this code by QIO functions.

I keep all the intermediate patches to be sure I don't break anything from the original 
behaviour of -netdev socket and to ease debugging by allowing to bisect between the 
different steps in the change.

Thanks,
Laurent