[Spice-devel] [PATCH spice 5/9] reds-stream: add send_msgfd()

[Marc-André Lureau]

----- Original Message -----
> > 
> > Hi
> > 
> > ----- Original Message -----
> > > > 
> > > > From: Marc-André Lureau <mlureau at redhat.com>
> > > > 
> > > > A new function to send fd with unix socket anciliary data.
> > > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > > ---
> > > >  server/reds-stream.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > >  server/reds-stream.h |  1 +
> > > >  2 files changed, 43 insertions(+)
> > > > 
> > > > diff --git a/server/reds-stream.c b/server/reds-stream.c
> > > > index d87cb23..4829b43 100644
> > > > --- a/server/reds-stream.c
> > > > +++ b/server/reds-stream.c
> > > > @@ -28,6 +28,7 @@
> > > >  #include <netdb.h>
> > > >  #include <unistd.h>
> > > >  #include <sys/socket.h>
> > > > +#include <fcntl.h>
> > > >  
> > > >  #include <glib.h>
> > > >  
> > > > @@ -254,6 +255,47 @@ int reds_stream_is_plain_unix(const RedsStream *s)
> > > >  
> > > >  }
> > > >  
> > > > +int reds_stream_send_msgfd(RedsStream *stream, int fd)
> > > > +{
> > > > +    struct msghdr msgh;
> > > > +    struct iovec iov;
> > > > +    int r;
> > > > +
> > > > +    size_t fd_size = 1 * sizeof(int);
> > > 
> > > const here?
> > 
> > Yeah, I am not that strict about internal variables. Sure why not?
> > 
> > > 
> > > > +    char control[CMSG_SPACE(fd_size)];
> > > 
> > > This would be better to be an union of data and struct cmsghdr, something
> > > like
> > > 
> > > union {
> > >    struct cmsghdr hdr;
> > >    char data[CMSG_SPACE(fd_size)];
> > > } control;
> > > 
> > > (of course this needs some other changes in the code).
> > 
> > Ok
> > 
> > > 
> > > > +    struct cmsghdr *cmsg;
> > > > +
> > > > +    spice_return_val_if_fail(reds_stream_is_plain_unix(stream), -1);
> > > > +
> > > > +    memset(&msgh, 0, sizeof(msgh));
> > > > +    memset(control, 0, sizeof(control));
> > > > +    /* set the payload */
> > > > +    iov.iov_base = (char*)"@";
> > > 
> > > is the cast required?
> > 
> > yes, "assignment discards 'const' qualifier from pointer target type"
> > 
> > > 
> > > > +    iov.iov_len = 1;
> > > > +
> > > > +    if (fd != -1) {
> > > > +        msgh.msg_iovlen = 1;
> > > > +        msgh.msg_iov = &iov;
> > > > +        msgh.msg_control = control;
> > > > +        msgh.msg_controllen = sizeof(control);
> > > > +
> > > > +        cmsg = CMSG_FIRSTHDR(&msgh);
> > > > +
> > > > +        cmsg->cmsg_len = CMSG_LEN(fd_size);
> > > > +        cmsg->cmsg_level = SOL_SOCKET;
> > > > +        cmsg->cmsg_type = SCM_RIGHTS;
> > > > +        memcpy(CMSG_DATA(cmsg), &fd, fd_size);
> > > > +    } else {
> > > > +        msgh.msg_iovlen = 0;
> > > 
> > > So you don't send nothing if no file descriptor is passes?
> > > How do you handle the read on the other end?
> > 
> > The read of one byte takes place, but no fd get through. The client should
> > not always expect a fd with the msg, depending on the msg. For
> > SPICE_MSG_DISPLAY_GL_SCANOUT_UNIX, it's possible to send no fd to "unset"
> > the current scanout.
> > 
> 
> My doubt is: don't you lose sync if client read one byte while server does
> not send any data? Did you try appending an integer after the file descriptor
> and see what's happen?

The in-band byte is still sent, it's the ancillary data that is empty. I will run further tests to confirm this work. I'll try to write a unit test for it too.

> 
> > > > +    }
> > > > +
> > > > +    do {
> > > > +        r = sendmsg(stream->socket, &msgh, 0);
> > > 
> > > MSG_NOSIGNAL as flag?
> > 
> > Why not? does that really change something here?
> > 
> 
> I think if you try to send data over a closed socket system will
> send a SIGPIPE to the process. If SIGPIPE is not blocked/ignored
> your program will exit. Actually Qemu and our software ignore the
> SIGPIPE so it's not a problem... but adding the flag make sure
> signal is not raised (an error is returned which is much easier
> to handle in a library).
> 
> > > 
> > > > +    } while (r < 0 && (errno == EINTR || errno == EAGAIN));
> > > > +
> > > > +    return r;
> > > > +}
> > > > +
> > > >  ssize_t reds_stream_writev(RedsStream *s, const struct iovec *iov, int
> > > >  iovcnt)
> > > >  {
> > > >      int i;
> > > > diff --git a/server/reds-stream.h b/server/reds-stream.h
> > > > index 9e53b22..72e5dd1 100644
> > > > --- a/server/reds-stream.h
> > > > +++ b/server/reds-stream.h
> > > > @@ -74,6 +74,7 @@ int reds_stream_enable_ssl(RedsStream *stream,
> > > > SSL_CTX
> > > > *ctx);
> > > >  void reds_stream_set_info_flag(RedsStream *stream, unsigned int flag);
> > > >  int reds_stream_get_family(const RedsStream *stream);
> > > >  int reds_stream_is_plain_unix(const RedsStream *stream);
> > > > +int reds_stream_send_msgfd(RedsStream *stream, int fd);
> > > >  
> > > >  typedef enum {
> > > >      REDS_SASL_ERROR_OK,
> > 
> > 
> > thanks for the review
> > 
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>