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

[Laurent Vivier <lvivier@redhat.com>]

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