Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Cam Macdonell <cam@cs.ualberta.ca>]

Mark McLoughlin wrote:
> If a file descriptor is passed via a message with SCM_RIGHTS ancillary
> data on a unix socket, store the file descriptor for use in the
> chr_read() handler. Close the file descriptor if it was not used.
> 
> The qemu_chr_get_msgfd() API provides access to the passed descriptor.
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  qemu-char.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qemu-char.h |    2 ++
>  2 files changed, 56 insertions(+), 1 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 9886228..6e2cd31 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -168,6 +168,11 @@ void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
>      s->chr_read(s->handler_opaque, buf, len);
>  }
>  
> +int qemu_chr_get_msgfd(CharDriverState *s)
> +{
> +    return s->get_msgfd ? s->get_msgfd(s) : -1;
> +}
> +
>  void qemu_chr_accept_input(CharDriverState *s)
>  {
>      if (s->chr_accept_input)
> @@ -1832,6 +1837,7 @@ typedef struct {
>      int do_telnetopt;
>      int do_nodelay;
>      int is_unix;
> +    int msgfd;
>  } TCPCharDriver;
>  
>  static void tcp_chr_accept(void *opaque);
> @@ -1907,20 +1913,61 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
>      *size = j;
>  }
>  
> +static int tcp_get_msgfd(CharDriverState *chr)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +
> +    return s->msgfd;
> +}
> +
>  #ifndef WIN32
> +static void unix_process_msgfd(CharDriverState *chr, struct msghdr *msg)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +    struct cmsghdr *cmsg;
> +
> +    for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
> +        int fd;
> +
> +        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
> +            cmsg->cmsg_level != SOL_SOCKET ||
> +            cmsg->cmsg_type != SCM_RIGHTS)
> +            continue;
> +
> +        fd = *((int *)CMSG_DATA(cmsg));
> +        if (fd < 0)
> +            continue;
> +
> +        if (s->msgfd != -1)
> +            close(s->msgfd);
> +        s->msgfd = fd;
> +    }
> +}
> +
>  static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>  {
>      TCPCharDriver *s = chr->opaque;
>      struct msghdr msg = { 0, };
>      struct iovec iov[1];
> +    union {
> +        struct cmsghdr cmsg;
> +        char control[CMSG_SPACE(sizeof(int))];
> +    } msg_control;
> +    ssize_t ret;
>  
>      iov[0].iov_base = buf;
>      iov[0].iov_len = len;
>  
>      msg.msg_iov = iov;
>      msg.msg_iovlen = 1;
> +    msg.msg_control = &msg_control;
> +    msg.msg_controllen = sizeof(msg_control);
> +
> +    ret = recvmsg(s->fd, &msg, 0);
> +    if (ret > 0 && s->is_unix)
> +        unix_process_msgfd(chr, &msg);
>  
> -    return recvmsg(s->fd, &msg, 0);
> +    return ret;
>  }
>  #else
>  static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
> @@ -1957,6 +2004,10 @@ static void tcp_chr_read(void *opaque)
>              tcp_chr_process_IAC_bytes(chr, s, buf, &size);
>          if (size > 0)
>              qemu_chr_read(chr, buf, size);
> +        if (s->msgfd != -1) {
> +            close(s->msgfd);
> +            s->msgfd = -1;
> +        }

Hi Mark,

I'm trying to understand the intended behaviour here so I can use your 
patch.  In your comments for the patch it reads "store the file 
descriptor for use in the chr_read() handler.  Close the file descriptor 
if it was not used."  It seems that upon returning from the chr_read() 
handler the descriptor is closed if s->msgfd is not set to -1.  Does 
this mean the handler should set the fd to -1 after retrieving it if the 
process wants to keep that fd open, otherwise it will be closed when 
chr_read() returns?  Should tcp_get_msgfd() perhaps set  s->msgfd to -1 
after it is retrieved?  Sort of a "read and clear" behaviour?

Thanks,
Cam

>      }
>  }
>  
> @@ -2118,12 +2169,14 @@ static CharDriverState *qemu_chr_open_tcp(const char *host_str,
>      s->connected = 0;
>      s->fd = -1;
>      s->listen_fd = -1;
> +    s->msgfd = -1;
>      s->is_unix = is_unix;
>      s->do_nodelay = do_nodelay && !is_unix;
>  
>      chr->opaque = s;
>      chr->chr_write = tcp_chr_write;
>      chr->chr_close = tcp_chr_close;
> +    chr->get_msgfd = tcp_get_msgfd;
>  
>      if (is_listen) {
>          s->listen_fd = fd;
> diff --git a/qemu-char.h b/qemu-char.h
> index e1aa8db..77d4eda 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -51,6 +51,7 @@ struct CharDriverState {
>      int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
>      void (*chr_update_read_handler)(struct CharDriverState *s);
>      int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
> +    int (*get_msgfd)(struct CharDriverState *s);
>      IOEventHandler *chr_event;
>      IOCanRWHandler *chr_can_read;
>      IOReadHandler *chr_read;
> @@ -81,6 +82,7 @@ void qemu_chr_reset(CharDriverState *s);
>  void qemu_chr_initial_reset(void);
>  int qemu_chr_can_read(CharDriverState *s);
>  void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
> +int qemu_chr_get_msgfd(CharDriverState *s);
>  void qemu_chr_accept_input(CharDriverState *s);
>  void qemu_chr_info(Monitor *mon);
>