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@xxxxxxxxxx>
> ---
> 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
