[libvirt] [PATCH] build: use gnulib passfd for simpler SCM_RIGHTS code
* bootstrap.conf (gnulib_modules): Add passfd. * src/util/util.c (virFileOpenAs): Simplify. --- It's always nice to shove the maintenance burden onto gnulib. bootstrap.conf | 1 + src/util/util.c | 38 ++++++++------------------------------ 2 files changed, 9 insertions(+), 30 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 293f86e..3b3a90f 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -52,6 +52,7 @@ mkstemps mktempd netdb nonblocking +passfd perror physmem pipe-posix diff --git a/src/util/util.c b/src/util/util.c index d4d2610..de4e3b3 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -78,6 +78,7 @@ #include "files.h" #include "command.h" #include "nonblocking.h" +#include "passfd.h" #ifndef NSIG # define NSIG 32 @@ -1480,11 +1481,6 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, int waitret, status, ret = 0; int fd = -1; int pair[2] = { -1, -1 }; - struct msghdr msg; - struct cmsghdr *cmsg; - char buf[CMSG_SPACE(sizeof(fd))]; - char dummy = 0; - struct iovec iov; int forkRet; if ((!(flags & VIR_FILE_OPEN_AS_UID)) @@ -1506,18 +1502,6 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, return ret; } - memset(&msg, 0, sizeof(msg)); - iov.iov_base = &dummy; - iov.iov_len = 1; - msg.msg_iov = &iov; - msg.msg_iovlen = 1; - msg.msg_control = buf; - msg.msg_controllen = sizeof(buf); - cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - cmsg->cmsg_len = CMSG_LEN(sizeof(fd)); - forkRet = virFork(&pid); if (pid < 0) { @@ -1529,26 +1513,20 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, VIR_FORCE_CLOSE(pair[1]); do { - ret = recvmsg(pair[0], &msg, 0); + ret = recvfd(pair[0], 0); } while (ret < 0 && errno == EINTR); - if (ret < 0) { + if (ret < 0 && errno != EACCES) { ret = -errno; VIR_FORCE_CLOSE(pair[0]); while ((waitret = waitpid(pid, NULL, 0) == -1) && (errno == EINTR)); goto parenterror; + } else { + fd = ret; } VIR_FORCE_CLOSE(pair[0]); - /* See if fd was transferred. */ - cmsg = CMSG_FIRSTHDR(&msg); - if (cmsg && cmsg->cmsg_len == CMSG_LEN(sizeof(fd)) && - cmsg->cmsg_level == SOL_SOCKET && - cmsg->cmsg_type == SCM_RIGHTS) { - memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd)); - } - /* wait for child to complete, and retrieve its exit code */ while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); @@ -1557,12 +1535,14 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, virReportSystemError(errno, _("failed to wait for child creating '%s'"), path); + VIR_FORCE_CLOSE(fd); goto parenterror; } if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ + VIR_FORCE_CLOSE(fd); return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); } if (!ret) @@ -1627,10 +1607,9 @@ parenterror: path, mode); goto childerror; } - memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd)); do { - ret = sendmsg(pair[1], &msg, 0); + ret = sendfd(pair[1], fd); } while (ret < 0 && errno == EINTR); if (ret < 0) { @@ -1638,7 +1617,6 @@ parenterror: goto childerror; } - ret = 0; childerror: /* ret tracks -errno on failure, but exit value must be positive. * If the child exits with EACCES, then the parent tries again. */ -- 1.7.1
On 04/20/2011 02:15 PM, Eric Blake wrote:
* bootstrap.conf (gnulib_modules): Add passfd. * src/util/util.c (virFileOpenAs): Simplify. ---
It's always nice to shove the maintenance burden onto gnulib.
bootstrap.conf | 1 + src/util/util.c | 38 ++++++++------------------------------ 2 files changed, 9 insertions(+), 30 deletions(-)
Self-NAK. Until gnulib provides a replacement <sys/uio.h> header, gnulib's passfd.c won't compile on mingw. At that point, I'll have to also update .gnulib, then the rest of this patch should work as-is. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* .gnulib: Update to latest for passfd fixes. * bootstrap.conf (gnulib_modules): Add passfd. * src/util/util.c (virFileOpenAs): Simplify. --- Now that the mingw side of passfd is fixed in gnulib, I can resubmit this patch. v2: update .gnulib, no other changes .gnulib | 2 +- bootstrap.conf | 1 + src/util/util.c | 38 ++++++++------------------------------ 3 files changed, 10 insertions(+), 31 deletions(-) diff --git a/.gnulib b/.gnulib index fb79969..5a9e46a 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit fb799692f5bb43310424977e0ca15599fc68d776 +Subproject commit 5a9e46ab46042f007426c1e06b836cf5608d8d4a diff --git a/bootstrap.conf b/bootstrap.conf index 293f86e..3b3a90f 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -52,6 +52,7 @@ mkstemps mktempd netdb nonblocking +passfd perror physmem pipe-posix diff --git a/src/util/util.c b/src/util/util.c index d4d2610..de4e3b3 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -78,6 +78,7 @@ #include "files.h" #include "command.h" #include "nonblocking.h" +#include "passfd.h" #ifndef NSIG # define NSIG 32 @@ -1480,11 +1481,6 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, int waitret, status, ret = 0; int fd = -1; int pair[2] = { -1, -1 }; - struct msghdr msg; - struct cmsghdr *cmsg; - char buf[CMSG_SPACE(sizeof(fd))]; - char dummy = 0; - struct iovec iov; int forkRet; if ((!(flags & VIR_FILE_OPEN_AS_UID)) @@ -1506,18 +1502,6 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, return ret; } - memset(&msg, 0, sizeof(msg)); - iov.iov_base = &dummy; - iov.iov_len = 1; - msg.msg_iov = &iov; - msg.msg_iovlen = 1; - msg.msg_control = buf; - msg.msg_controllen = sizeof(buf); - cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - cmsg->cmsg_len = CMSG_LEN(sizeof(fd)); - forkRet = virFork(&pid); if (pid < 0) { @@ -1529,26 +1513,20 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, VIR_FORCE_CLOSE(pair[1]); do { - ret = recvmsg(pair[0], &msg, 0); + ret = recvfd(pair[0], 0); } while (ret < 0 && errno == EINTR); - if (ret < 0) { + if (ret < 0 && errno != EACCES) { ret = -errno; VIR_FORCE_CLOSE(pair[0]); while ((waitret = waitpid(pid, NULL, 0) == -1) && (errno == EINTR)); goto parenterror; + } else { + fd = ret; } VIR_FORCE_CLOSE(pair[0]); - /* See if fd was transferred. */ - cmsg = CMSG_FIRSTHDR(&msg); - if (cmsg && cmsg->cmsg_len == CMSG_LEN(sizeof(fd)) && - cmsg->cmsg_level == SOL_SOCKET && - cmsg->cmsg_type == SCM_RIGHTS) { - memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd)); - } - /* wait for child to complete, and retrieve its exit code */ while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); @@ -1557,12 +1535,14 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, virReportSystemError(errno, _("failed to wait for child creating '%s'"), path); + VIR_FORCE_CLOSE(fd); goto parenterror; } if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ + VIR_FORCE_CLOSE(fd); return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); } if (!ret) @@ -1627,10 +1607,9 @@ parenterror: path, mode); goto childerror; } - memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd)); do { - ret = sendmsg(pair[1], &msg, 0); + ret = sendfd(pair[1], fd); } while (ret < 0 && errno == EINTR); if (ret < 0) { @@ -1638,7 +1617,6 @@ parenterror: goto childerror; } - ret = 0; childerror: /* ret tracks -errno on failure, but exit value must be positive. * If the child exits with EACCES, then the parent tries again. */ -- 1.7.4.4
On 04/21/2011 05:17 PM, Eric Blake wrote:
* .gnulib: Update to latest for passfd fixes. * bootstrap.conf (gnulib_modules): Add passfd. * src/util/util.c (virFileOpenAs): Simplify. ---
Now that the mingw side of passfd is fixed in gnulib, I can resubmit this patch.
v2: update .gnulib, no other changes
.gnulib | 2 +- bootstrap.conf | 1 + src/util/util.c | 38 ++++++++------------------------------ 3 files changed, 10 insertions(+), 31 deletions(-)
diff --git a/.gnulib b/.gnulib index fb79969..5a9e46a 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit fb799692f5bb43310424977e0ca15599fc68d776 +Subproject commit 5a9e46ab46042f007426c1e06b836cf5608d8d4a diff --git a/bootstrap.conf b/bootstrap.conf index 293f86e..3b3a90f 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -52,6 +52,7 @@ mkstemps mktempd netdb nonblocking +passfd perror physmem pipe-posix diff --git a/src/util/util.c b/src/util/util.c index d4d2610..de4e3b3 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -78,6 +78,7 @@ #include "files.h" #include "command.h" #include "nonblocking.h" +#include "passfd.h"
#ifndef NSIG # define NSIG 32 @@ -1480,11 +1481,6 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, int waitret, status, ret = 0; int fd = -1; int pair[2] = { -1, -1 }; - struct msghdr msg; - struct cmsghdr *cmsg; - char buf[CMSG_SPACE(sizeof(fd))]; - char dummy = 0; - struct iovec iov; int forkRet;
if ((!(flags& VIR_FILE_OPEN_AS_UID)) @@ -1506,18 +1502,6 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, return ret; }
- memset(&msg, 0, sizeof(msg)); - iov.iov_base =&dummy; - iov.iov_len = 1; - msg.msg_iov =&iov; - msg.msg_iovlen = 1; - msg.msg_control = buf; - msg.msg_controllen = sizeof(buf); - cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - cmsg->cmsg_len = CMSG_LEN(sizeof(fd)); - forkRet = virFork(&pid);
if (pid< 0) { @@ -1529,26 +1513,20 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, VIR_FORCE_CLOSE(pair[1]);
do { - ret = recvmsg(pair[0],&msg, 0); + ret = recvfd(pair[0], 0); } while (ret< 0&& errno == EINTR);
- if (ret< 0) { + if (ret< 0&& errno != EACCES) { ret = -errno; VIR_FORCE_CLOSE(pair[0]); while ((waitret = waitpid(pid, NULL, 0) == -1) && (errno == EINTR)); goto parenterror; + } else { + fd = ret; }
What if errno == EACCES? Will we be getting all the error recovery we need in the caller?
VIR_FORCE_CLOSE(pair[0]);
- /* See if fd was transferred. */ - cmsg = CMSG_FIRSTHDR(&msg); - if (cmsg&& cmsg->cmsg_len == CMSG_LEN(sizeof(fd))&& - cmsg->cmsg_level == SOL_SOCKET&& - cmsg->cmsg_type == SCM_RIGHTS) { - memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd)); - } - /* wait for child to complete, and retrieve its exit code */ while ((waitret = waitpid(pid,&status, 0) == -1) && (errno == EINTR)); @@ -1557,12 +1535,14 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, virReportSystemError(errno, _("failed to wait for child creating '%s'"), path); + VIR_FORCE_CLOSE(fd); goto parenterror; } if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ + VIR_FORCE_CLOSE(fd); return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); } if (!ret) @@ -1627,10 +1607,9 @@ parenterror: path, mode); goto childerror; } - memcpy(CMSG_DATA(cmsg),&fd, sizeof(fd));
do { - ret = sendmsg(pair[1],&msg, 0); + ret = sendfd(pair[1], fd); } while (ret< 0&& errno == EINTR);
if (ret< 0) { @@ -1638,7 +1617,6 @@ parenterror: goto childerror; }
- ret = 0; childerror: /* ret tracks -errno on failure, but exit value must be positive. * If the child exits with EACCES, then the parent tries again. */
On 04/25/2011 01:17 AM, Laine Stump wrote:
@@ -1529,26 +1513,20 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, VIR_FORCE_CLOSE(pair[1]);
do { - ret = recvmsg(pair[0],&msg, 0); + ret = recvfd(pair[0], 0); } while (ret< 0&& errno == EINTR);
ret == -1 and errno == EACCES on failure to transfer fd...
- if (ret< 0) { + if (ret< 0&& errno != EACCES) { ret = -errno; VIR_FORCE_CLOSE(pair[0]); while ((waitret = waitpid(pid, NULL, 0) == -1) && (errno == EINTR)); goto parenterror; + } else { + fd = ret; }
so now fd == -1...
if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); }
so this uses the fallback code, regardless of child exit status, and we also ensured that the child got reaped.
What if errno == EACCES? Will we be getting all the error recovery we need in the caller?
Yes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/25/2011 10:21 AM, Eric Blake wrote:
On 04/25/2011 01:17 AM, Laine Stump wrote:
@@ -1529,26 +1513,20 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, VIR_FORCE_CLOSE(pair[1]);
do { - ret = recvmsg(pair[0],&msg, 0); + ret = recvfd(pair[0], 0); } while (ret< 0&& errno == EINTR); ret == -1 and errno == EACCES on failure to transfer fd...
- if (ret< 0) { + if (ret< 0&& errno != EACCES) { ret = -errno; VIR_FORCE_CLOSE(pair[0]); while ((waitret = waitpid(pid, NULL, 0) == -1) && (errno == EINTR)); goto parenterror; + } else { + fd = ret; } so now fd == -1...
if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); }
so this uses the fallback code, regardless of child exit status, and we also ensured that the child got reaped.
What if errno == EACCES? Will we be getting all the error recovery we need in the caller? Yes.
Okay. In that case, ACK.
On 04/26/2011 09:19 AM, Laine Stump wrote:
do { - ret = recvmsg(pair[0],&msg, 0); + ret = recvfd(pair[0], 0); } while (ret< 0&& errno == EINTR);
ret == -1 and errno == EACCES on failure to transfer fd...
if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); }
so this uses the fallback code, regardless of child exit status, and we also ensured that the child got reaped.
What if errno == EACCES? Will we be getting all the error recovery we need in the caller? Yes.
Another way to look at it - for all failures except EACCES, the child is immediately reaped and the overall function failed; with EACCES, we still want to try the non-forking fallback.
Okay. In that case, ACK.
Thanks for the review; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump