1:30 p.m.
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(a)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(a)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
>
>