Re: [Qemu-devel] [PATCH] RFC: net/socket: learn to talk with a unix dgram socket

[Jason Wang <jasowang@redhat.com>]

On 2018/11/14 下午9:01, Marc-André Lureau wrote:
> Hi
>
> On Wed, Nov 14, 2018 at 7:46 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2018/11/10 上午3:56, Marc-André Lureau wrote:
>>> -net socket has a fd argument, and may be passed pre-opened sockets.
>>>
>>> TCP sockets use framing.
>>> UDP sockets have datagram boundaries.
>>>
>>> When given a unix dgram socket, it will be able to read from it, but
>>> will attempt to send on the dgram_dst, which is unset. The other end
>>> will not receive the data.
>>>
>>> Let's teach -net socket to recognize a UNIX DGRAM socket, and use the
>>> regular send() command (without dgram_dst).
>>>
>>> This makes running slirp out-of-process possible that
>>> way (python pseudo-code):
>>>
>>> a, b = socket.socketpair(socket.AF_UNIX, socket.SOCK_DGRAM)
>>>
>>> subprocess.Popen('qemu -net socket,fd=%d -net user' % a.fileno(), shell=True)
>>> subprocess.Popen('qemu ... -net nic -net socket,fd=%d' % b.fileno(), shell=True)
>>>
>>> (to make slirp a seperate project altogether, we would have to have
>>> some compatibility code and/or deprecate various options & HMP
>>> commands for dynamic port forwarding etc - but this looks like a
>>> reachable goal)
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> I believe instead of supporting unnamed sockets, we should also support
>> named one through cli?
> This could be a later patch, I have no need for it yet. Perhaps it
> should be a chardev then?


I mean something like: -socket id=ud0,path=/tmp/XXX


>
>>> ---
>>>    net/socket.c | 25 +++++++++++++++++++++----
>>>    1 file changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/socket.c b/net/socket.c
>>> index 7095eb749f..8a9c30892d 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -119,9 +119,13 @@ static ssize_t net_socket_receive_dgram(NetClientState *nc, const uint8_t *buf,
>>>        ssize_t ret;
>>>
>>>        do {
>>> -        ret = qemu_sendto(s->fd, buf, size, 0,
>>> -                          (struct sockaddr *)&s->dgram_dst,
>>> -                          sizeof(s->dgram_dst));
>>> +        if (s->dgram_dst.sin_family != AF_UNIX) {
>>> +            ret = qemu_sendto(s->fd, buf, size, 0,
>>> +                              (struct sockaddr *)&s->dgram_dst,
>>> +                              sizeof(s->dgram_dst));
>>> +        } else {
>>> +            ret = send(s->fd, buf, size, 0);
>>> +        }
>>
>> Any reason that send is a must here? send(2) said:
>>          call
>>
>>              send(sockfd, buf, len, flags);
>>
>>          is equivalent to
>>
>>              sendto(sockfd, buf, len, flags, NULL, 0);
> Yes they should be equivalent, but then we need to add ?: operators
> for the dest arguments. I preferred to have an if() instead.
>
> thanks


One possible issue here is I'm not sure there's a equivalent send() in 
e.g non POSIX system.

Thanks


>
>>>        } while (ret == -1 && errno == EINTR);
>>>
>>>        if (ret == -1 && errno == EAGAIN) {
>>> @@ -322,6 +326,15 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>>>        int newfd;
>>>        NetClientState *nc;
>>>        NetSocketState *s;
>>> +    SocketAddress *sa;
>>> +    SocketAddressType sa_type;
>>> +
>>> +    sa = socket_local_address(fd, errp);
>>> +    if (!sa) {
>>> +        return NULL;
>>> +    }
>>> +    sa_type = sa->type;
>>> +    qapi_free_SocketAddress(sa);
>>>
>>>        /* fd passed: multicast: "learn" dgram_dst address from bound address and save it
>>>         * Because this may be "shared" socket from a "master" process, datagrams would be recv()
>>> @@ -365,8 +378,12 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>>>                     "socket: fd=%d (cloned mcast=%s:%d)",
>>>                     fd, inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
>>>        } else {
>>> +        if (sa_type == SOCKET_ADDRESS_TYPE_UNIX) {
>>> +            s->dgram_dst.sin_family = AF_UNIX;
>>> +        }
>>> +
>>>            snprintf(nc->info_str, sizeof(nc->info_str),
>>> -                 "socket: fd=%d", fd);
>>> +                 "socket: fd=%d %s", fd, SocketAddressType_str(sa_type));
>>>        }
>>>
>>>        return s;