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

[Andrew Melnichenko]

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 at 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 at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210825/66f2bd22/attachment-0001.htm>