[RFC PATCH 04/10] virsocket: Added receive for multiple fds.

Andrew Melnychenko posted 10 patches 4 years, 6 months ago
There is a newer version of this series
[RFC PATCH 04/10] virsocket: Added receive for multiple fds.
Posted by Andrew Melnychenko 4 years, 6 months ago
Similar to virSocketRecvFD() added virSocketRecvMultipleFDs().
This function returns multiple fds through unix socket.
New function is required for "qemu-ebpf-rss-helper" program.
The helper may pass few file descriptors - eBPF program and maps.

Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virsocket.c     | 83 ++++++++++++++++++++++++++++++++++++++++
 src/util/virsocket.h     |  2 +
 3 files changed, 86 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 43493ea76e..6987ff00c2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3226,6 +3226,7 @@ virSecureEraseString;
 # util/virsocket.h
 virSocketRecvFD;
 virSocketSendFD;
+virSocketRecvMultipleFDs;
 
 
 # util/virsocketaddr.h
diff --git a/src/util/virsocket.c b/src/util/virsocket.c
index b971da16e3..da8af42e72 100644
--- a/src/util/virsocket.c
+++ b/src/util/virsocket.c
@@ -486,6 +486,82 @@ virSocketRecvFD(int sock, int fdflags)
 
     return fd;
 }
+
+
+/* virSocketRecvMultipleFDs receives few file descriptors through the socket.
+   The flags are a bitmask, possibly including O_CLOEXEC (defined in <fcntl.h>).
+
+   Return the number of recived file descriptors on success,
+   or -1 with errno set in case of error.
+*/
+int
+virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags)
+{
+    char byte = 0;
+    struct iovec iov;
+    struct msghdr msg;
+    int ret = -1;
+    ssize_t len;
+    struct cmsghdr *cmsg;
+    char buf[CMSG_SPACE(sizeof(int) * nfds)];
+    int fdflags_recvmsg = fdflags & O_CLOEXEC ? MSG_CMSG_CLOEXEC : 0;
+    int fdSize = -1;
+    int i = 0;
+    int saved_errno = 0;
+
+    if ((fdflags & ~O_CLOEXEC) != 0) {
+        errno = EINVAL;
+        return -1;
+    }
+
+    /* send at least one char */
+    memset(&msg, 0, sizeof(msg));
+    iov.iov_base = &byte;
+    iov.iov_len = 1;
+    msg.msg_iov = &iov;
+    msg.msg_iovlen = 1;
+    msg.msg_name = NULL;
+    msg.msg_namelen = 0;
+
+    msg.msg_control = buf;
+    msg.msg_controllen = sizeof(buf);
+
+    len = recvmsg(sock, &msg, fdflags_recvmsg);
+    if (len < 0) {
+        return -1;
+    }
+
+    cmsg = CMSG_FIRSTHDR(&msg);
+    /* be paranoiac */
+    if (len == 0 || cmsg == NULL || cmsg->cmsg_len < CMSG_LEN(sizeof(int))
+        || cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
+        /* fake errno: at end the file is not available */
+        errno = len ? EACCES : ENOTCONN;
+        return -1;
+    }
+
+    fdSize = cmsg->cmsg_len - CMSG_LEN(0);
+    memcpy(fds, CMSG_DATA(cmsg), fdSize);
+    ret = fdSize/sizeof(int);
+
+    /* set close-on-exec flag */
+    if (!MSG_CMSG_CLOEXEC && (fdflags & O_CLOEXEC)) {
+        for (i = 0; i < ret; ++i) {
+            if (virSetCloseExec(fds[i]) < 0) {
+                saved_errno = errno;
+                goto error;
+            }
+        }
+    }
+
+    return ret;
+error:
+    for (i = 0; i < ret; ++i) {
+        VIR_FORCE_CLOSE(fds[i]);
+    }
+    errno = saved_errno;
+    return -1;
+}
 #else /* WIN32 */
 int
 virSocketSendFD(int sock G_GNUC_UNUSED, int fd G_GNUC_UNUSED)
@@ -500,4 +576,11 @@ virSocketRecvFD(int sock G_GNUC_UNUSED, int fdflags G_GNUC_UNUSED)
     errno = ENOSYS;
     return -1;
 }
+
+int
+virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags)
+{
+    errno = ENOSYS;
+    return -1;
+}
 #endif  /* WIN32 */
diff --git a/src/util/virsocket.h b/src/util/virsocket.h
index 419da8b3ae..c926effbc3 100644
--- a/src/util/virsocket.h
+++ b/src/util/virsocket.h
@@ -22,6 +22,8 @@
 
 int virSocketSendFD(int sock, int fd);
 int virSocketRecvFD(int sock, int fdflags);
+int
+virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags);
 
 #ifdef WIN32
 
-- 
2.31.1

Re: [RFC PATCH 04/10] virsocket: Added receive for multiple fds.
Posted by Michal Prívozník 4 years, 5 months ago
On 7/28/21 10:17 AM, Andrew Melnychenko wrote:
> Similar to virSocketRecvFD() added virSocketRecvMultipleFDs().
> This function returns multiple fds through unix socket.
> New function is required for "qemu-ebpf-rss-helper" program.
> The helper may pass few file descriptors - eBPF program and maps.
> 
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virsocket.c     | 83 ++++++++++++++++++++++++++++++++++++++++
>  src/util/virsocket.h     |  2 +
>  3 files changed, 86 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 43493ea76e..6987ff00c2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3226,6 +3226,7 @@ virSecureEraseString;
>  # util/virsocket.h
>  virSocketRecvFD;
>  virSocketSendFD;
> +virSocketRecvMultipleFDs;

This needs to be ordered. The correct order is:

 # util/virsocket.h
 virSocketRecvFD;
 virSocketRecvMultipleFDs;
 virSocketSendFD;


>  
>  
>  # util/virsocketaddr.h
> diff --git a/src/util/virsocket.c b/src/util/virsocket.c
> index b971da16e3..da8af42e72 100644
> --- a/src/util/virsocket.c
> +++ b/src/util/virsocket.c
> @@ -486,6 +486,82 @@ virSocketRecvFD(int sock, int fdflags)
>  
>      return fd;
>  }
> +
> +
> +/* virSocketRecvMultipleFDs receives few file descriptors through the socket.
> +   The flags are a bitmask, possibly including O_CLOEXEC (defined in <fcntl.h>).
> +
> +   Return the number of recived file descriptors on success,
> +   or -1 with errno set in case of error.
> +*/
> +int
> +virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags)
> +{
> +    char byte = 0;
> +    struct iovec iov;
> +    struct msghdr msg;
> +    int ret = -1;
> +    ssize_t len;
> +    struct cmsghdr *cmsg;
> +    char buf[CMSG_SPACE(sizeof(int) * nfds)];
> +    int fdflags_recvmsg = fdflags & O_CLOEXEC ? MSG_CMSG_CLOEXEC : 0;
> +    int fdSize = -1;
> +    int i = 0;
> +    int saved_errno = 0;
> +
> +    if ((fdflags & ~O_CLOEXEC) != 0) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    /* send at least one char */
> +    memset(&msg, 0, sizeof(msg));
> +    iov.iov_base = &byte;
> +    iov.iov_len = 1;
> +    msg.msg_iov = &iov;
> +    msg.msg_iovlen = 1;
> +    msg.msg_name = NULL;
> +    msg.msg_namelen = 0;
> +
> +    msg.msg_control = buf;
> +    msg.msg_controllen = sizeof(buf);
> +
> +    len = recvmsg(sock, &msg, fdflags_recvmsg);
> +    if (len < 0) {
> +        return -1;
> +    }
> +
> +    cmsg = CMSG_FIRSTHDR(&msg);
> +    /* be paranoiac */
> +    if (len == 0 || cmsg == NULL || cmsg->cmsg_len < CMSG_LEN(sizeof(int))
> +        || cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
> +        /* fake errno: at end the file is not available */
> +        errno = len ? EACCES : ENOTCONN;
> +        return -1;
> +    }
> +
> +    fdSize = cmsg->cmsg_len - CMSG_LEN(0);
> +    memcpy(fds, CMSG_DATA(cmsg), fdSize);
> +    ret = fdSize/sizeof(int);

Please put a space before and after '/'. Like this:

ret = fdSize / sizeof(int);

> +
> +    /* set close-on-exec flag */
> +    if (!MSG_CMSG_CLOEXEC && (fdflags & O_CLOEXEC)) {
> +        for (i = 0; i < ret; ++i) {
> +            if (virSetCloseExec(fds[i]) < 0) {
> +                saved_errno = errno;

This isn't needed really, because ..

> +                goto error;
> +            }
> +        }
> +    }
> +
> +    return ret;
> +error:
> +    for (i = 0; i < ret; ++i) {
> +        VIR_FORCE_CLOSE(fds[i]);

.. VIR_FORCE_CLOSE() doesn't change errno.

> +    }
> +    errno = saved_errno;
> +    return -1;
> +}

But I wonder if this function is needed. I mean, we currently have
virSocketRecvFD() and this new function looks very much like it. Would
it be possible to turn virSocketRecvFD() into virSocketRecvMultipleFDs()
and fix all (current) callers of virSocketRecvFD()?

Alternatively, we can have virSocketRecvMultipleFDs() as you propose and
then virSocketRecvFD() be just a thin wrapper over
virSocketRecvMultipleFDs(), e.g. like this:

virSocketRecvFD()
{
  int fds[1];

  virSocketRecvMultipleFDs(sock, fds, 1, fdflags);
  return fds[0];
}

Or even better, have virSocketRecvFD() return FD via argument and its
retval be 0/-1 (success/fail).

My aim is to avoid having nearly the same code twice.

Michal

Re: [RFC PATCH 04/10] virsocket: Added receive for multiple fds.
Posted by Andrew Melnichenko 4 years, 5 months ago
Hi,

> virSocketRecvFD()
> {
>   int fds[1];
>
>   virSocketRecvMultipleFDs(sock, fds, 1, fdflags);
>   return fds[0];
> }
>
Yea, it's a good idea.

On Fri, Aug 20, 2021 at 3:57 PM Michal Prívozník <mprivozn@redhat.com>
wrote:

> On 7/28/21 10:17 AM, Andrew Melnychenko wrote:
> > Similar to virSocketRecvFD() added virSocketRecvMultipleFDs().
> > This function returns multiple fds through unix socket.
> > New function is required for "qemu-ebpf-rss-helper" program.
> > The helper may pass few file descriptors - eBPF program and maps.
> >
> > Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> > ---
> >  src/libvirt_private.syms |  1 +
> >  src/util/virsocket.c     | 83 ++++++++++++++++++++++++++++++++++++++++
> >  src/util/virsocket.h     |  2 +
> >  3 files changed, 86 insertions(+)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 43493ea76e..6987ff00c2 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -3226,6 +3226,7 @@ virSecureEraseString;
> >  # util/virsocket.h
> >  virSocketRecvFD;
> >  virSocketSendFD;
> > +virSocketRecvMultipleFDs;
>
> This needs to be ordered. The correct order is:
>
>  # util/virsocket.h
>  virSocketRecvFD;
>  virSocketRecvMultipleFDs;
>  virSocketSendFD;
>
>
> >
> >
> >  # util/virsocketaddr.h
> > diff --git a/src/util/virsocket.c b/src/util/virsocket.c
> > index b971da16e3..da8af42e72 100644
> > --- a/src/util/virsocket.c
> > +++ b/src/util/virsocket.c
> > @@ -486,6 +486,82 @@ virSocketRecvFD(int sock, int fdflags)
> >
> >      return fd;
> >  }
> > +
> > +
> > +/* virSocketRecvMultipleFDs receives few file descriptors through the
> socket.
> > +   The flags are a bitmask, possibly including O_CLOEXEC (defined in
> <fcntl.h>).
> > +
> > +   Return the number of recived file descriptors on success,
> > +   or -1 with errno set in case of error.
> > +*/
> > +int
> > +virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags)
> > +{
> > +    char byte = 0;
> > +    struct iovec iov;
> > +    struct msghdr msg;
> > +    int ret = -1;
> > +    ssize_t len;
> > +    struct cmsghdr *cmsg;
> > +    char buf[CMSG_SPACE(sizeof(int) * nfds)];
> > +    int fdflags_recvmsg = fdflags & O_CLOEXEC ? MSG_CMSG_CLOEXEC : 0;
> > +    int fdSize = -1;
> > +    int i = 0;
> > +    int saved_errno = 0;
> > +
> > +    if ((fdflags & ~O_CLOEXEC) != 0) {
> > +        errno = EINVAL;
> > +        return -1;
> > +    }
> > +
> > +    /* send at least one char */
> > +    memset(&msg, 0, sizeof(msg));
> > +    iov.iov_base = &byte;
> > +    iov.iov_len = 1;
> > +    msg.msg_iov = &iov;
> > +    msg.msg_iovlen = 1;
> > +    msg.msg_name = NULL;
> > +    msg.msg_namelen = 0;
> > +
> > +    msg.msg_control = buf;
> > +    msg.msg_controllen = sizeof(buf);
> > +
> > +    len = recvmsg(sock, &msg, fdflags_recvmsg);
> > +    if (len < 0) {
> > +        return -1;
> > +    }
> > +
> > +    cmsg = CMSG_FIRSTHDR(&msg);
> > +    /* be paranoiac */
> > +    if (len == 0 || cmsg == NULL || cmsg->cmsg_len <
> CMSG_LEN(sizeof(int))
> > +        || cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type !=
> SCM_RIGHTS) {
> > +        /* fake errno: at end the file is not available */
> > +        errno = len ? EACCES : ENOTCONN;
> > +        return -1;
> > +    }
> > +
> > +    fdSize = cmsg->cmsg_len - CMSG_LEN(0);
> > +    memcpy(fds, CMSG_DATA(cmsg), fdSize);
> > +    ret = fdSize/sizeof(int);
>
> Please put a space before and after '/'. Like this:
>
> ret = fdSize / sizeof(int);
>
> > +
> > +    /* set close-on-exec flag */
> > +    if (!MSG_CMSG_CLOEXEC && (fdflags & O_CLOEXEC)) {
> > +        for (i = 0; i < ret; ++i) {
> > +            if (virSetCloseExec(fds[i]) < 0) {
> > +                saved_errno = errno;
>
> This isn't needed really, because ..
>
> > +                goto error;
> > +            }
> > +        }
> > +    }
> > +
> > +    return ret;
> > +error:
> > +    for (i = 0; i < ret; ++i) {
> > +        VIR_FORCE_CLOSE(fds[i]);
>
> .. VIR_FORCE_CLOSE() doesn't change errno.
>
> > +    }
> > +    errno = saved_errno;
> > +    return -1;
> > +}
>
> But I wonder if this function is needed. I mean, we currently have
> virSocketRecvFD() and this new function looks very much like it. Would
> it be possible to turn virSocketRecvFD() into virSocketRecvMultipleFDs()
> and fix all (current) callers of virSocketRecvFD()?
>
> Alternatively, we can have virSocketRecvMultipleFDs() as you propose and
> then virSocketRecvFD() be just a thin wrapper over
> virSocketRecvMultipleFDs(), e.g. like this:
>
> virSocketRecvFD()
> {
>   int fds[1];
>
>   virSocketRecvMultipleFDs(sock, fds, 1, fdflags);
>   return fds[0];
> }
>
> Or even better, have virSocketRecvFD() return FD via argument and its
> retval be 0/-1 (success/fail).
>
> My aim is to avoid having nearly the same code twice.
>
> Michal
>
>