Since not only systemd can do this (we'll be doing it as well few patches later), change 'systemd' to 'caller' and fix LISTEN_FDS to LISTEN_PID where applicable. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 45 +++------------------------------- src/util/virutil.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 4 files changed, 69 insertions(+), 41 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 08111d4..7516ed3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2107,6 +2107,7 @@ virGetGroupID; virGetGroupList; virGetGroupName; virGetHostname; +virGetListenFDs; virGetSelfLastChanged; virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 3379f29..e9219d5 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -600,50 +600,13 @@ static int virLockDaemonSetupNetworkingSystemD(virNetServerPtr srv) { virNetServerServicePtr svc; - const char *pidstr; - const char *fdstr; - unsigned long long procid; unsigned int nfds; - VIR_DEBUG("Setting up networking from systemd"); - - if (!(pidstr = virGetEnvAllowSUID("LISTEN_PID"))) { - VIR_DEBUG("No LISTEN_FDS from systemd"); - return 0; - } - - if (virStrToLong_ull(pidstr, NULL, 10, &procid) < 0) { - VIR_DEBUG("Malformed LISTEN_PID from systemd %s", pidstr); - return 0; - } - - if ((pid_t)procid != getpid()) { - VIR_DEBUG("LISTEN_PID %s is not for us %llu", - pidstr, (unsigned long long)getpid()); - return 0; - } - - if (!(fdstr = virGetEnvAllowSUID("LISTEN_FDS"))) { - VIR_DEBUG("No LISTEN_FDS from systemd"); - return 0; - } - - if (virStrToLong_ui(fdstr, NULL, 10, &nfds) < 0) { - VIR_DEBUG("Malformed LISTEN_FDS from systemd %s", fdstr); - return 0; - } - - if (nfds > 1) { - VIR_DEBUG("Too many (%d) file descriptors from systemd", - nfds); - nfds = 1; - } - - unsetenv("LISTEN_PID"); - unsetenv("LISTEN_FDS"); - - if (nfds == 0) + if ((nfds = virGetListenFDs()) == 0) return 0; + if (nfds > 1) + VIR_DEBUG("Too many (%d) file descriptors from systemd", nfds); + nfds = 1; /* Systemd passes FDs, starting immediately after stderr, * so the first FD we'll get is '3'. */ diff --git a/src/util/virutil.c b/src/util/virutil.c index 50faf2b..1d897d9 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2396,3 +2396,65 @@ void virUpdateSelfLastChanged(const char *path) selfLastChanged = sb.st_ctime; } } + +/** + * virGetListenFDs: + * + * Parse LISTEN_PID and LISTEN_FDS passed from caller. + * + * Returns number of passed FDs. + */ +unsigned int +virGetListenFDs(void) +{ + const char *pidstr; + const char *fdstr; + size_t i = 0; + unsigned long long procid; + unsigned int nfds; + + VIR_DEBUG("Setting up networking from caller"); + + if (!(pidstr = virGetEnvAllowSUID("LISTEN_PID"))) { + VIR_DEBUG("No LISTEN_PID from caller"); + return 0; + } + + if (virStrToLong_ull(pidstr, NULL, 10, &procid) < 0) { + VIR_DEBUG("Malformed LISTEN_PID from caller %s", pidstr); + return 0; + } + + if ((pid_t)procid != getpid()) { + VIR_DEBUG("LISTEN_PID %s is not for us %llu", + pidstr, (unsigned long long)getpid()); + return 0; + } + + if (!(fdstr = virGetEnvAllowSUID("LISTEN_FDS"))) { + VIR_DEBUG("No LISTEN_FDS from caller"); + return 0; + } + + if (virStrToLong_ui(fdstr, NULL, 10, &nfds) < 0) { + VIR_DEBUG("Malformed LISTEN_FDS from caller %s", fdstr); + return 0; + } + + unsetenv("LISTEN_PID"); + unsetenv("LISTEN_FDS"); + + VIR_DEBUG("Got %u file descriptors", nfds); + + for (i = 0; i < nfds; i++) { + int fd = STDERR_FILENO + i + 1; + + VIR_DEBUG("Disabling inheritance of passed FD %d", fd); + + if (virSetInherit(fd, false) < 0) { + VIR_WARN("Couldn't disable inheritance of passed FD %d", fd); + } + } + + return nfds; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index f93ea93..89b7923 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -232,4 +232,6 @@ typedef enum { VIR_ENUM_DECL(virTristateBool) VIR_ENUM_DECL(virTristateSwitch) +unsigned int virGetListenFDs(void); + #endif /* __VIR_UTIL_H__ */ -- 2.0.4

It's just a wrapper around NewFD and NewUNIX that selects the right option and increments the number of used FDs. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_remote.syms | 1 + src/rpc/virnetserverservice.c | 50 ++++++++++++++++++++++++++++++++++++++++++- src/rpc/virnetserverservice.h | 14 +++++++++++- 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index d482a55..6b520b5 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -159,6 +159,7 @@ virNetServerServiceGetMaxRequests; virNetServerServiceGetPort; virNetServerServiceIsReadonly; virNetServerServiceNewFD; +virNetServerServiceNewFDOrUNIX; virNetServerServiceNewPostExecRestart; virNetServerServiceNewTCP; virNetServerServiceNewUNIX; diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 320a02c..e85889b 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -1,7 +1,7 @@ /* * virnetserverservice.c: generic network RPC server service * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2012, 2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -25,6 +25,8 @@ #include "virnetserverservice.h" +#include <unistd.h> + #include "viralloc.h" #include "virerror.h" #include "virthread.h" @@ -90,6 +92,52 @@ static void virNetServerServiceAccept(virNetSocketPtr sock, } +virNetServerServicePtr +virNetServerServiceNewFDOrUNIX(const char *path, + mode_t mask, + gid_t grp, + int auth, +#if WITH_GNUTLS + virNetTLSContextPtr tls, +#endif + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max, + unsigned int nfds, + unsigned int *cur_fd) +{ + if (*cur_fd - STDERR_FILENO > nfds) { + /* + * There are no more file descriptors to use, so we have to + * fallback to UNIX socket. + */ + return virNetServerServiceNewUNIX(path, + mask, + grp, + auth, +#if WITH_GNUTLS + tls, +#endif + readonly, + max_queued_clients, + nrequests_client_max); + + } else { + /* + * There's still enough file descriptors. In this case we'll + * use the current one and increment it afterwards. + */ + return virNetServerServiceNewFD(*cur_fd++, + auth, +#if WITH_GNUTLS + tls, +#endif + readonly, + nrequests_client_max); + } +} + + virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, const char *service, int auth, diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index eb31abf..a1c8960 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -1,7 +1,7 @@ /* * virnetserverservice.h: generic network RPC server service * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2011, 2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -37,6 +37,18 @@ typedef int (*virNetServerServiceDispatchFunc)(virNetServerServicePtr svc, virNetSocketPtr sock, void *opaque); +virNetServerServicePtr virNetServerServiceNewFDOrUNIX(const char *path, + mode_t mask, + gid_t grp, + int auth, +# if WITH_GNUTLS + virNetTLSContextPtr tls, +# endif + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max, + unsigned int nfds, + unsigned int *cur_fd); virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, const char *service, int auth, -- 2.0.4

First FD is the RW unix socket to listen on, second one (if applicable) is the RO unix socket. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- daemon/libvirtd.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index a1f64ad..2c18c5e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -56,6 +56,7 @@ #include "virstring.h" #include "locking/lock_manager.h" #include "viraccessmanager.h" +#include "virutil.h" #ifdef WITH_DRIVER_MODULES # include "driver.h" @@ -476,11 +477,19 @@ static int daemonSetupNetworking(virNetServerPtr srv, int unix_sock_ro_mask = 0; int unix_sock_rw_mask = 0; + unsigned int cur_fd = STDERR_FILENO + 1; + unsigned int nfds = virGetListenFDs(); + if (config->unix_sock_group) { if (virGetGroupID(config->unix_sock_group, &unix_sock_gid) < 0) return -1; } + if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) { + VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); + return -1; + } + if (virStrToLong_i(config->unix_sock_ro_perms, NULL, 8, &unix_sock_ro_mask) != 0) { VIR_ERROR(_("Failed to parse mode '%s'"), config->unix_sock_ro_perms); goto error; @@ -491,30 +500,30 @@ static int daemonSetupNetworking(virNetServerPtr srv, goto error; } - VIR_DEBUG("Registering unix socket %s", sock_path); - if (!(svc = virNetServerServiceNewUNIX(sock_path, - unix_sock_rw_mask, - unix_sock_gid, - config->auth_unix_rw, + if (!(svc = virNetServerServiceNewFDOrUNIX(sock_path, + unix_sock_rw_mask, + unix_sock_gid, + config->auth_unix_rw, #if WITH_GNUTLS - NULL, + NULL, #endif - false, - config->max_queued_clients, - config->max_client_requests))) + false, + config->max_queued_clients, + config->max_client_requests, + nfds, &cur_fd))) goto error; if (sock_path_ro) { - VIR_DEBUG("Registering unix socket %s", sock_path_ro); - if (!(svcRO = virNetServerServiceNewUNIX(sock_path_ro, - unix_sock_ro_mask, - unix_sock_gid, - config->auth_unix_ro, + if (!(svcRO = virNetServerServiceNewFDOrUNIX(sock_path_ro, + unix_sock_ro_mask, + unix_sock_gid, + config->auth_unix_ro, #if WITH_GNUTLS - NULL, + NULL, #endif - true, - config->max_queued_clients, - config->max_client_requests))) + true, + config->max_queued_clients, + config->max_client_requests, + nfds, &cur_fd))) goto error; } -- 2.0.4

Even line like this: int asdf = i - somevar; was matched by the regex, so this patch adds '=' to the list of characters that break the regexp. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 35ef444..eed8af5 100644 --- a/cfg.mk +++ b/cfg.mk @@ -568,7 +568,7 @@ sc_avoid_attribute_unused_in_header: $(_sc_search_regexp) sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)\>(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(=]* )*(i|j|k)\>(\s|,|;)' \ halt='use size_t, not int/unsigned int for loop vars i, j, k' \ $(_sc_search_regexp) -- 2.0.4

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/commandtest.c | 49 ++++++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 7d2161c..ba823f7 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -62,7 +62,8 @@ main(void) #else -static int checkoutput(const char *testname) +static int checkoutput(const char *testname, + char *prefix) { int ret = -1; char *expectname = NULL; @@ -86,6 +87,16 @@ static int checkoutput(const char *testname) goto cleanup; } + if (prefix) { + char *tmp = NULL; + + if (virAsprintf(&tmp, "%s%s", prefix, expectlog) < 0) + goto cleanup; + + VIR_FREE(expectlog); + expectlog = tmp; + } + if (STRNEQ(expectlog, actuallog)) { virtTestDifference(stderr, expectlog, actuallog); goto cleanup; @@ -173,7 +184,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) return -1; } - if ((ret = checkoutput("test2")) != 0) { + if ((ret = checkoutput("test2", NULL)) != 0) { virCommandFree(cmd); return ret; } @@ -187,7 +198,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test2"); + return checkoutput("test2", NULL); } /* @@ -219,7 +230,7 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test3"); + ret = checkoutput("test3", NULL); cleanup: virCommandFree(cmd); @@ -261,7 +272,7 @@ static int test4(const void *unused ATTRIBUTE_UNUSED) while (kill(pid, 0) != -1) usleep(100*1000); - ret = checkoutput("test4"); + ret = checkoutput("test4", NULL); cleanup: virCommandFree(cmd); @@ -291,7 +302,7 @@ static int test5(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test5"); + return checkoutput("test5", NULL); } @@ -315,7 +326,7 @@ static int test6(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test6"); + return checkoutput("test6", NULL); } @@ -340,7 +351,7 @@ static int test7(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test7"); + return checkoutput("test7", NULL); } /* @@ -365,7 +376,7 @@ static int test8(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test8"); + return checkoutput("test8", NULL); } @@ -403,7 +414,7 @@ static int test9(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test9"); + return checkoutput("test9", NULL); } @@ -429,7 +440,7 @@ static int test10(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test10"); + return checkoutput("test10", NULL); } /* @@ -453,7 +464,7 @@ static int test11(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test11"); + return checkoutput("test11", NULL); } /* @@ -475,7 +486,7 @@ static int test12(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); - return checkoutput("test12"); + return checkoutput("test12", NULL); } /* @@ -510,7 +521,7 @@ static int test13(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test13"); + ret = checkoutput("test13", NULL); cleanup: virCommandFree(cmd); @@ -582,7 +593,7 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test14"); + ret = checkoutput("test14", NULL); cleanup: virCommandFree(cmd); @@ -613,7 +624,7 @@ static int test15(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test15"); + ret = checkoutput("test15", NULL); cleanup: VIR_FREE(cwd); @@ -659,7 +670,7 @@ static int test16(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test16"); + ret = checkoutput("test16", NULL); cleanup: virCommandFree(cmd); @@ -841,7 +852,7 @@ static int test20(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test20"); + ret = checkoutput("test20", NULL); cleanup: virCommandFree(cmd); VIR_FREE(buf); @@ -900,7 +911,7 @@ static int test21(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - ret = checkoutput("test21"); + ret = checkoutput("test21", NULL); cleanup: VIR_FREE(outbuf); VIR_FREE(errbuf); -- 2.0.4

That sets a new flag, but that flag does mean the child will get LISTEN_FDS and LISTEN_PID environment variables properly set and passed FDs reordered so that it corresponds with LISTEN_FDS (they must start right after STDERR_FILENO). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 99 ++++++++++++++++++++++++++++++++++++++++++++ src/util/vircommand.h | 4 +- tests/commanddata/test24.log | 7 ++++ tests/commandtest.c | 56 +++++++++++++++++++++++++ 5 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 tests/commanddata/test24.log diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7516ed3..e09ddd5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1146,6 +1146,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; virCommandRun; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index e775ba6..3b3e6f5 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -66,6 +66,7 @@ enum { VIR_EXEC_CLEAR_CAPS = (1 << 2), VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), + VIR_EXEC_LISTEN_FDS = (1 << 5), }; typedef struct _virCommandFD virCommandFD; @@ -200,6 +201,78 @@ virCommandFDSet(virCommandPtr cmd, return 0; } +static void +virCommandReorderFDs(virCommandPtr cmd) +{ + int maxfd = 0; + int openmax = 0; + size_t i = 0; + + if (!cmd || cmd->has_error || !cmd->npassfd) + return; + + for (i = 0; i < cmd->npassfd; i++) + maxfd = MAX(cmd->passfd[i].fd, maxfd); + + openmax = sysconf(_SC_OPEN_MAX); + if (openmax < 0 || + maxfd + cmd->npassfd > openmax) + goto error; + + /* + * Simple two-pass sort, nothing fancy. This is not designed for + * anything else than passing around 2 FDs into the child. + * + * So first dup2() them somewhere else. + */ + for (i = 0; i < cmd->npassfd; i++) { + int newfd = maxfd + i + 1; + int oldfd = cmd->passfd[i].fd; + if (dup2(oldfd, newfd) != newfd) { + virReportSystemError(errno, + _("Cannot dup2() fd %d before " + "passing it to the child"), + oldfd); + goto error; + } + VIR_FORCE_CLOSE(cmd->passfd[i].fd); + } + + VIR_DEBUG("First reorder pass done"); + + /* + * And then dup2() them in orderly manner. + */ + for (i = 0; i < cmd->npassfd; i++) { + int newfd = STDERR_FILENO + i + 1; + int oldfd = maxfd + i + 1; + if (dup2(oldfd, newfd) != newfd) { + virReportSystemError(errno, + _("Cannot dup2() fd %d before " + "passing it to the child"), + oldfd); + goto error; + } + if (virSetInherit(newfd, true) < 0) { + virReportSystemError(errno, + _("Cannot set O_CLOEXEC on fd %d before " + "passing it to the child"), + newfd); + goto error; + } + VIR_FORCE_CLOSE(oldfd); + cmd->passfd[i].fd = newfd; + } + + VIR_DEBUG("Second reorder pass done"); + + return; + + error: + cmd->has_error = -1; + return; +} + #ifndef WIN32 /** @@ -678,6 +751,15 @@ virExec(virCommandPtr cmd) goto fork_error; } + if (cmd->flags & VIR_EXEC_LISTEN_FDS) { + virCommandReorderFDs(cmd); + virCommandAddEnvFormat(cmd, "LISTEN_PID=%u", getpid()); + virCommandAddEnvFormat(cmd, "LISTEN_FDS=%zu", cmd->npassfd); + + if (cmd->has_error) + goto fork_error; + } + /* Close logging again to ensure no FDs leak to child */ virLogReset(); @@ -919,6 +1001,23 @@ virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) } /** + * virCommandPassListenFDs: + * @cmd: the command to modify + * + * Pass LISTEN_FDS and LISTEN_PID environment variables into the + * child. LISTEN_PID has the value of the child's PID and LISTEN_FDS + * is a number of passed file descriptors starting from 3. + */ +void +virCommandPassListenFDs(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->flags |= VIR_EXEC_LISTEN_FDS; +} + +/** * virCommandSetPidFile: * @cmd: the command to modify * @pidfile: filename to use diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 8cdb31c..d3b286d 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -1,7 +1,7 @@ /* * vircommand.h: Child command execution * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -60,6 +60,8 @@ void virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags); +void virCommandPassListenFDs(virCommandPtr cmd); + void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2); diff --git a/tests/commanddata/test24.log b/tests/commanddata/test24.log new file mode 100644 index 0000000..8670952 --- /dev/null +++ b/tests/commanddata/test24.log @@ -0,0 +1,7 @@ +FD:0 +FD:1 +FD:2 +FD:3 +FD:4 +DAEMON:yes +CWD:/ diff --git a/tests/commandtest.c b/tests/commandtest.c index ba823f7..b3287fa 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1032,6 +1032,61 @@ test23(const void *unused ATTRIBUTE_UNUSED) return ret; } +static int test24(const void *unused ATTRIBUTE_UNUSED) +{ + char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper"); + char *prefix = NULL; + int newfd1 = dup(STDERR_FILENO); + int newfd2 = dup(STDERR_FILENO); + int newfd3 = dup(STDERR_FILENO); + int ret = -1; + pid_t pid; + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + if (!pidfile) + goto cleanup; + + if (VIR_CLOSE(newfd1) < 0) + printf("Cannot close fd %d\n", newfd1); + + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + virCommandPassFD(cmd, newfd2, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassFD(cmd, newfd3, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassListenFDs(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + goto cleanup; + } + + if (virPidFileRead(abs_builddir, "commandhelper", &pid) < 0) { + printf("cannot read pidfile\n"); + goto cleanup; + } + + if (virAsprintf(&prefix, + "ENV:LISTEN_FDS=2\nENV:LISTEN_PID=%u\n", + pid) < 0) + goto cleanup; + + while (kill(pid, 0) != -1) + usleep(100*1000); + + ret = checkoutput("test24", prefix); + + cleanup: + if (pidfile) + unlink(pidfile); + VIR_FREE(pidfile); + virCommandFree(cmd); + /* coverity[double_close] */ + VIR_FORCE_CLOSE(newfd2); + VIR_FORCE_CLOSE(newfd3); + return ret; +} + static void virCommandThreadWorker(void *opaque) { virCommandTestDataPtr test = opaque; @@ -1181,6 +1236,7 @@ mymain(void) DO_TEST(test21); DO_TEST(test22); DO_TEST(test23); + DO_TEST(test24); virMutexLock(&test->lock); if (test->running) { -- 2.0.4

This eliminates the need for active waiting. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 85 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 66 insertions(+), 19 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index a94b2bc..f913365 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1,7 +1,7 @@ /* * virnetsocket.c: generic network socket handling * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -121,7 +121,7 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket) #ifndef WIN32 -static int virNetSocketForkDaemon(const char *binary) +static int virNetSocketForkDaemon(const char *binary, int passfd) { int ret; virCommandPtr cmd = virCommandNewArgList(binary, @@ -134,6 +134,10 @@ static int virNetSocketForkDaemon(const char *binary) virCommandAddEnvPassBlockSUID(cmd, "XDG_RUNTIME_DIR", NULL); virCommandClearCaps(cmd); virCommandDaemonize(cmd); + if (passfd) { + virCommandPassFD(cmd, passfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassListenFDs(cmd); + } ret = virCommandRun(cmd, NULL); virCommandFree(cmd); return ret; @@ -540,10 +544,10 @@ int virNetSocketNewConnectUNIX(const char *path, const char *binary, virNetSocketPtr *retsock) { + char *buf = NULL; + int fd, passfd; virSocketAddr localAddr; virSocketAddr remoteAddr; - int fd; - int retries = 0; memset(&localAddr, 0, sizeof(localAddr)); memset(&remoteAddr, 0, sizeof(remoteAddr)); @@ -570,25 +574,65 @@ int virNetSocketNewConnectUNIX(const char *path, remoteAddr.data.un.sun_path[0] = '\0'; retry: - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { - if ((errno == ECONNREFUSED || - errno == ENOENT) && - spawnDaemon && retries < 20) { - VIR_DEBUG("Connection refused for %s, trying to spawn %s", - path, binary); - if (retries == 0 && - virNetSocketForkDaemon(binary) < 0) - goto error; + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0 && !spawnDaemon) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), + path); + goto error; + } else if (spawnDaemon) { + int status = 0; + pid_t pid = 0; + + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", _("Failed to create socket")); + goto error; + } + + /* + * We have to fork() here, because umask() is set + * per-process, chmod() is racy and fchmod() has undefined + * behaviour on sockets according to POSIX, so it doesn't + * work outside Linux. + */ + if ((pid = virFork()) < 0) + goto error; + + if (pid == 0) { + umask(0077); + if (bind(passfd, &remoteAddr.data.sa, remoteAddr.len) < 0) + _exit(EXIT_FAILURE); + + _exit(EXIT_SUCCESS); + } + + if (virProcessWait(pid, &status, false) < 0) + goto error; - retries++; - usleep(1000 * 100 * retries); + if (status != EXIT_SUCCESS) { + /* + * OK, so the subprocces failed to bind() the socket. This may mean + * that another daemon was starting at the same time and succeeded + * with its bind(). So we'll try connecting again, but this time + * without spawning the daemon. + */ + spawnDaemon = false; goto retry; } - virReportSystemError(errno, - _("Failed to connect socket to '%s'"), - path); - goto error; + if (listen(passfd, 0) < 0) { + virReportSystemError(errno, "%s", + _("Failed to listen on socket that's about " + "to be passed to the daemon")); + goto error; + } + + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), + path); + goto error; + } + + if (virNetSocketForkDaemon(binary, passfd) < 0) + goto error; } localAddr.len = sizeof(localAddr.data); @@ -603,7 +647,10 @@ int virNetSocketNewConnectUNIX(const char *path, return 0; error: + VIR_FREE(buf); VIR_FORCE_CLOSE(fd); + if (spawnDaemon) + unlink(path); return -1; } #else -- 2.0.4

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- .gitignore | 1 + daemon/Makefile.am | 14 ++++++++++++-- daemon/libvirtd.conf | 5 +++++ daemon/libvirtd.service.in | 5 ----- daemon/libvirtd.socket.in | 11 +++++++++++ libvirt.spec.in | 25 ++++++++++++++++++++----- 6 files changed, 49 insertions(+), 12 deletions(-) create mode 100644 daemon/libvirtd.socket.in diff --git a/.gitignore b/.gitignore index 90fee91..9776ea1 100644 --- a/.gitignore +++ b/.gitignore @@ -60,6 +60,7 @@ /daemon/libvirtd.pod /daemon/libvirtd.policy /daemon/libvirtd.service +/daemon/libvirtd.socket /daemon/test_libvirtd.aug /docs/aclperms.htmlinc /docs/apibuild.py.stamp diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 00221ab..70b7655 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -55,6 +55,7 @@ EXTRA_DIST = \ libvirtd.policy.in \ libvirtd.sasl \ libvirtd.service.in \ + libvirtd.socket.in \ libvirtd.sysconf \ libvirtd.sysctl \ libvirtd.aug \ @@ -388,15 +389,18 @@ endif ! LIBVIRT_INIT_SCRIPT_UPSTART if LIBVIRT_INIT_SCRIPT_SYSTEMD SYSTEMD_UNIT_DIR = $(prefix)/lib/systemd/system -BUILT_SOURCES += libvirtd.service +BUILT_SOURCES += libvirtd.service libvirtd.socket -install-init-systemd: install-sysconfig libvirtd.service +install-init-systemd: install-sysconfig libvirtd.service libvirtd.socket $(MKDIR_P) $(DESTDIR)$(SYSTEMD_UNIT_DIR) $(INSTALL_DATA) libvirtd.service \ $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.service + $(INSTALL_DATA) libvirtd.socket \ + $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.socket uninstall-init-systemd: uninstall-sysconfig rm -f $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.service + rm -f $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.socket rmdir $(DESTDIR)$(SYSTEMD_UNIT_DIR) || : else ! LIBVIRT_INIT_SCRIPT_SYSTEMD install-init-systemd: @@ -420,6 +424,12 @@ libvirtd.service: libvirtd.service.in $(top_builddir)/config.status < $< > $@-t && \ mv $@-t $@ +libvirtd.socket: libvirtd.socket.in $(top_builddir)/config.status + $(AM_V_GEN)sed \ + -e 's|[@]runstatedir[@]|$(runstatedir)|g' \ + < $< > $@-t && \ + mv $@-t $@ + check-local: check-augeas diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf index 2d80274..d4f6a1c 100644 --- a/daemon/libvirtd.conf +++ b/daemon/libvirtd.conf @@ -77,6 +77,11 @@ # UNIX socket access controls # +# Beware that if you are changing *any* of these options, and you use +# socket activation with systemd, you need to adjust the settings in +# the libvirtd.socket file as well since it could impose a security +# risk if you rely on file permission checking only. + # Set the UNIX domain socket group ownership. This can be used to # allow a 'trusted' set of users access to management capabilities # without becoming root. diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in index 086da36..1759ac8 100644 --- a/daemon/libvirtd.service.in +++ b/daemon/libvirtd.service.in @@ -1,8 +1,3 @@ -# NB we don't use socket activation. When libvirtd starts it will -# spawn any virtual machines registered for autostart. We want this -# to occur on every boot, regardless of whether any client connects -# to a socket. Thus socket activation doesn't have any benefit - [Unit] Description=Virtualization daemon Before=libvirt-guests.service diff --git a/daemon/libvirtd.socket.in b/daemon/libvirtd.socket.in new file mode 100644 index 0000000..0915bb3 --- /dev/null +++ b/daemon/libvirtd.socket.in @@ -0,0 +1,11 @@ +[Socket] +ListenStream=@runstatedir@/libvirt/libvirt-sock +ListenStream=@runstatedir@/libvirt/libvirt-sock-ro + +; The following settings must match libvirtd.conf file in order to +; work as expected because libvirtd can't change them later. +; SocketMode=0777 is safe only if authentication on the socket is set +; up. For further information, please see the libvirtd.conf file. +SocketMode=0777 +SocketUser=root +SocketGroup=root diff --git a/libvirt.spec.in b/libvirt.spec.in index 29da071..7fc6a8d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1670,11 +1670,13 @@ done %if %{with_systemd} %if %{with_systemd_macros} - %systemd_post virtlockd.socket libvirtd.service + %systemd_post virtlockd.socket libvirtd.service libvirtd.socket %else if [ $1 -eq 1 ] ; then # Initial installation - /bin/systemctl enable virtlockd.socket libvirtd.service >/dev/null 2>&1 || : + /bin/systemctl enable \ + virtlockd.socket \ + libvirtd.service >/dev/null 2>&1 || : fi %endif %else @@ -1695,12 +1697,24 @@ fi %preun daemon %if %{with_systemd} %if %{with_systemd_macros} - %systemd_preun libvirtd.service virtlockd.socket virtlockd.service + %systemd_preun \ + libvirtd.socket \ + libvirtd.service \ + virtlockd.socket \ + virtlockd.service %else if [ $1 -eq 0 ] ; then # Package removal, not upgrade - /bin/systemctl --no-reload disable libvirtd.service virtlockd.socket virtlockd.service > /dev/null 2>&1 || : - /bin/systemctl stop libvirtd.service virtlockd.socket virtlockd.service > /dev/null 2>&1 || : + /bin/systemctl --no-reload disable \ + libvirtd.socket \ + libvirtd.service \ + virtlockd.socket \ + virtlockd.service > /dev/null 2>&1 || : + /bin/systemctl stop \ + libvirtd.socket \ + libvirtd.service \ + virtlockd.socket \ + virtlockd.service > /dev/null 2>&1 || : fi %endif %else @@ -1857,6 +1871,7 @@ exit 0 %if %{with_systemd} %{_unitdir}/libvirtd.service +%{_unitdir}/libvirtd.socket %{_unitdir}/virtlockd.service %{_unitdir}/virtlockd.socket %else -- 2.0.4