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

[Marc-André Lureau]

Hi

On Wed, Nov 14, 2018 at 7:46 AM Jason Wang <address@hidden> 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 <address@hidden>
>
>
> 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?

> > ---
> >   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

>
> >       } 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;