[libvirt] [PATCH v2 0/8] Speed up waiting for the session daemon
This is complete rework of: http://www.redhat.com/archives/libvir-list/2013-April/msg01351.html where Daniel suggested we use systemd-like passing of socket fd in combination with the LISTEN_FDS environment variable: http://www.redhat.com/archives/libvir-list/2013-April/msg01356.html Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Martin Kletzander (8): util: abstract parsing of passed FDs into virGetListenFDs() remote: create virNetServerServiceNewFDOrUNIX() wrapper rpc: set listen backlog on FDs as well as on other sockets daemon: support passing FDs from the calling process cfg.mk: allow integers to be assigned a value computed with i|j|k tests: support dynamic prefixes in commandtest util: add virCommandPassListenFDs() function rpc: pass listen FD to the daemon being started cfg.mk | 2 +- daemon/libvirtd.c | 45 ++++++++++-------- src/libvirt_private.syms | 2 + src/libvirt_remote.syms | 1 + src/locking/lock_daemon.c | 47 ++----------------- src/rpc/virnetserverservice.c | 53 ++++++++++++++++++++- src/rpc/virnetserverservice.h | 15 +++++- src/rpc/virnetsocket.c | 58 +++++++++++++++-------- src/util/vircommand.c | 99 +++++++++++++++++++++++++++++++++++++++ src/util/vircommand.h | 4 +- src/util/virutil.c | 51 ++++++++++++++++++++ src/util/virutil.h | 2 + tests/commanddata/test24.log | 7 +++ tests/commandtest.c | 105 ++++++++++++++++++++++++++++++++++-------- 14 files changed, 389 insertions(+), 102 deletions(-) create mode 100644 tests/commanddata/test24.log -- 2.0.0
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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 4 files changed, 58 insertions(+), 41 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8d3671c..138a9fa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2123,6 +2123,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 95d1ff9..6f3f411 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2227,3 +2227,54 @@ 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; + 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); + + return nfds; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 2bb74e2..76c1ef3 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -203,4 +203,6 @@ bool virIsSUID(void); time_t virGetSelfLastChanged(void); void virUpdateSelfLastChanged(const char *path); +unsigned int virGetListenFDs(void); + #endif /* __VIR_UTIL_H__ */ -- 2.0.0
On Wed, Jul 16, 2014 at 08:29:55PM +0200, Martin Kletzander wrote:
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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 4 files changed, 58 insertions(+), 41 deletions(-)
[...]
diff --git a/src/util/virutil.c b/src/util/virutil.c index 95d1ff9..6f3f411 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2227,3 +2227,54 @@ 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; + 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); + + return nfds; +}
Note (not just) to self (but also reviewers): This function should probably set O_CLOEXEC flag on all the passed FDs, but I just realized that now and am not sure whether this was already done in virtlockd or not. Martin
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 | 48 ++++++++++++++++++++++++++++++++++++++++++- src/rpc/virnetserverservice.h | 14 ++++++++++++- 3 files changed, 61 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..b50f57e 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 @@ -90,6 +90,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 - 2 > 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.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/locking/lock_daemon.c | 2 +- src/rpc/virnetserverservice.c | 5 +++++ src/rpc/virnetserverservice.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index e9219d5..02d77e3 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -614,7 +614,7 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr srv) #if WITH_GNUTLS NULL, #endif - false, 1))) + false, 0, 1))) return -1; if (virNetServerAddService(srv, svc, NULL) < 0) { diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index b50f57e..7892a95 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -131,6 +131,7 @@ virNetServerServiceNewFDOrUNIX(const char *path, tls, #endif readonly, + max_queued_clients, nrequests_client_max); } } @@ -263,6 +264,7 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, virNetTLSContextPtr tls, #endif bool readonly, + size_t max_queued_clients, size_t nrequests_client_max) { virNetServerServicePtr svc; @@ -290,6 +292,9 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, goto error; for (i = 0; i < svc->nsocks; i++) { + if (virNetSocketListen(svc->socks[i], max_queued_clients) < 0) + goto error; + /* IO callback is initially disabled, until we're ready * to deal with incoming clients */ if (virNetSocketAddIOCallback(svc->socks[i], diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index a1c8960..b1d6c2d 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -74,6 +74,7 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, virNetTLSContextPtr tls, # endif bool readonly, + size_t max_queued_clients, size_t nrequests_client_max); virNetServerServicePtr virNetServerServiceNewPostExecRestart(virJSONValuePtr object); -- 2.0.0
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 4c926b3..d20aeae 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 = STDIN_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.0
On Wed, Jul 16, 2014 at 08:29:58PM +0200, Martin Kletzander wrote:
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 4c926b3..d20aeae 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 = STDIN_FILENO + 1;
Shouldn't that be STDERR not STDIN since the passed FDs start at 3 not 1.
+ unsigned int nfds = virGetListenFDs(); +
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Jul 17, 2014 at 09:40:12AM +0100, Daniel P. Berrange wrote:
On Wed, Jul 16, 2014 at 08:29:58PM +0200, Martin Kletzander wrote:
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 4c926b3..d20aeae 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 = STDIN_FILENO + 1;
Shouldn't that be STDERR not STDIN since the passed FDs start at 3 not 1.
Oh, yes, I tried that with the value "3" and changed it right before sending, my bad.
+ unsigned int nfds = virGetListenFDs(); +
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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 baaab71..85a7060 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.0
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.0
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 138a9fa..3332952 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1165,6 +1165,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.0
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 | 58 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index a94b2bc..c00209c 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; @@ -542,8 +546,7 @@ int virNetSocketNewConnectUNIX(const char *path, { virSocketAddr localAddr; virSocketAddr remoteAddr; - int fd; - int retries = 0; + int fd, passfd; memset(&localAddr, 0, sizeof(localAddr)); memset(&remoteAddr, 0, sizeof(remoteAddr)); @@ -569,28 +572,45 @@ int virNetSocketNewConnectUNIX(const char *path, if (remoteAddr.data.un.sun_path[0] == '@') 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 (spawnDaemon) { + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", _("Failed to create socket")); + goto error; + } - retries++; - usleep(1000 * 100 * retries); - goto retry; + /* + * We cannot do the umask() trick here because that's not + * thread-safe. fchmod(), however, is not guaranteed to work on + * some BSD favours, but *should* work on Linux before the socket + * is bound. POSIX says the behaviour of fchmod() called on + * socket is unspecified, though. + */ + if (fchmod(passfd, 0700) < 0) { + virReportSystemError(errno, "%s", + _("Failed to change permissions on socket")); + goto error; } - virReportSystemError(errno, - _("Failed to connect socket to '%s'"), + if (bind(passfd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to bind socket to %s"), path); + goto error; + } + + if (listen(passfd, 0) < 0) { + virReportSystemError(errno, "%s", _("Failed to listen on socket to")); + goto error; + } + } + + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; } + if (spawnDaemon && virNetSocketForkDaemon(binary, passfd) < 0) + goto error; + localAddr.len = sizeof(localAddr.data); if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { virReportSystemError(errno, "%s", _("Unable to get local socket name")); -- 2.0.0
On Wed, Jul 16, 2014 at 08:30:02PM +0200, Martin Kletzander wrote:
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 | 58 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 19 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index a94b2bc..c00209c 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c [...] @@ -569,28 +572,45 @@ int virNetSocketNewConnectUNIX(const char *path, [...] + /* + * We cannot do the umask() trick here because that's not + * thread-safe. fchmod(), however, is not guaranteed to work on + * some BSD favours, but *should* work on Linux before the socket + * is bound. POSIX says the behaviour of fchmod() called on + * socket is unspecified, though. + */ + if (fchmod(passfd, 0700) < 0) { + virReportSystemError(errno, "%s", + _("Failed to change permissions on socket")); + goto error; }
I've finally found a way out of this. We can fork() and in the child do only umask() and bind(). It shouldn't be a problem that fstat() returns different mode for the socket than stat(), it should work everywhere and thanks to the fact that we do this pretty rarely and copy-on-write pages there shouldn't be significant impact. Is this acceptable? Martin
On Wed, Jul 16, 2014 at 08:30:02PM +0200, Martin Kletzander wrote:
- 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 (spawnDaemon) { + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", _("Failed to create socket")); + goto error; + }
- retries++; - usleep(1000 * 100 * retries); - goto retry; + /* + * We cannot do the umask() trick here because that's not + * thread-safe. fchmod(), however, is not guaranteed to work on + * some BSD favours, but *should* work on Linux before the socket + * is bound. POSIX says the behaviour of fchmod() called on + * socket is unspecified, though. + */
IIUC, the entire concept of UNIX domain socket permissions is entirely unspecified not just fchmod(). We could however use umask() here if you use a virFork() to start a single threaded process. Set umask, create the socket, and then exit, allowing the parent to continue.
+ if (fchmod(passfd, 0700) < 0) { + virReportSystemError(errno, "%s", + _("Failed to change permissions on socket")); + goto error; }
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/23/2014 03:25 AM, Daniel P. Berrange wrote:
We could however use umask() here if you use a virFork() to start a single threaded process. Set umask, create the socket, and then exit, allowing the parent to continue.
Similar to what we already do in virfile.c for virFileAccessibleAs, virFileOpenAs, and virDirCreate. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Jul 23, 2014 at 07:01:06AM -0600, Eric Blake wrote:
On 07/23/2014 03:25 AM, Daniel P. Berrange wrote:
We could however use umask() here if you use a virFork() to start a single threaded process. Set umask, create the socket, and then exit, allowing the parent to continue.
Similar to what we already do in virfile.c for virFileAccessibleAs, virFileOpenAs, and virDirCreate.
Oh, I had the feeling that I saw such scenario somewhere. And now that I have v3 of the patches working, I won't be able to get some kind of abstracting this out of my head. Martin
On Wed, Jul 16, 2014 at 08:29:54PM +0200, Martin Kletzander wrote:
This is complete rework of:
http://www.redhat.com/archives/libvir-list/2013-April/msg01351.html
where Daniel suggested we use systemd-like passing of socket fd in combination with the LISTEN_FDS environment variable:
http://www.redhat.com/archives/libvir-list/2013-April/msg01356.html
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
Obviously that bug is focused on starting of the session daemon, but the code we're adding here should work with systemd. Have you tested that this actually allows for systemd activation of the privileged libvirtd. If we're adding this FD passing, I think we really ought to make sure we support this, so we don't have to revisit it later. Should add a libvirtd.socket unit file too so we have systemd activation by default for libvirtd. NB, we stil need to enable the daemon by default anyway since libvirtd needs todo autostart of VMs, but having the socket activation too avoids some race conditions with startup. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 22, 2014 at 01:36:56PM +0100, Daniel P. Berrange wrote:
On Wed, Jul 16, 2014 at 08:29:54PM +0200, Martin Kletzander wrote:
This is complete rework of:
http://www.redhat.com/archives/libvir-list/2013-April/msg01351.html
where Daniel suggested we use systemd-like passing of socket fd in combination with the LISTEN_FDS environment variable:
http://www.redhat.com/archives/libvir-list/2013-April/msg01356.html
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
Obviously that bug is focused on starting of the session daemon, but the code we're adding here should work with systemd. Have you tested that this actually allows for systemd activation of the privileged libvirtd. If we're adding this FD passing, I think we really ought to make sure we support this, so we don't have to revisit it later. Should add a libvirtd.socket unit file too so we have systemd activation by default for libvirtd.
Obviously I haven't. I just wanted to get rid of that silly, silly bug.
NB, we stil need to enable the daemon by default anyway since libvirtd needs todo autostart of VMs, but having the socket activation too avoids some race conditions with startup.
Yes, that's one of the reasons why I think it will create more confusion than races it will resolve. I'll _try_ to work this in, but how would you suggest to set up the initial permissions? Anything the user will change in libvirtd.conf he will also have to change in the libvirt.socket file, because someone might use the filesystem-level permission checking for isolating some users (or anything else) because we don't want to break that.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 22, 2014 at 05:11:23PM +0200, Martin Kletzander wrote:
On Tue, Jul 22, 2014 at 01:36:56PM +0100, Daniel P. Berrange wrote:
On Wed, Jul 16, 2014 at 08:29:54PM +0200, Martin Kletzander wrote:
This is complete rework of:
http://www.redhat.com/archives/libvir-list/2013-April/msg01351.html
where Daniel suggested we use systemd-like passing of socket fd in combination with the LISTEN_FDS environment variable:
http://www.redhat.com/archives/libvir-list/2013-April/msg01356.html
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
Obviously that bug is focused on starting of the session daemon, but the code we're adding here should work with systemd. Have you tested that this actually allows for systemd activation of the privileged libvirtd. If we're adding this FD passing, I think we really ought to make sure we support this, so we don't have to revisit it later. Should add a libvirtd.socket unit file too so we have systemd activation by default for libvirtd.
Obviously I haven't. I just wanted to get rid of that silly, silly bug.
And now I tried and ... it Just Works(TM). I'll add a configuration comment and libvirtd.socket.in file in the patch and send a v3. Let me know what do you think of those permission settings there.
NB, we stil need to enable the daemon by default anyway since libvirtd needs todo autostart of VMs, but having the socket activation too avoids some race conditions with startup.
Yes, that's one of the reasons why I think it will create more confusion than races it will resolve.
I'll _try_ to work this in, but how would you suggest to set up the initial permissions? Anything the user will change in libvirtd.conf he will also have to change in the libvirt.socket file, because someone might use the filesystem-level permission checking for isolating some users (or anything else) because we don't want to break that.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 23, 2014 at 10:49:33AM +0200, Martin Kletzander wrote:
On Tue, Jul 22, 2014 at 05:11:23PM +0200, Martin Kletzander wrote:
On Tue, Jul 22, 2014 at 01:36:56PM +0100, Daniel P. Berrange wrote:
On Wed, Jul 16, 2014 at 08:29:54PM +0200, Martin Kletzander wrote:
This is complete rework of:
http://www.redhat.com/archives/libvir-list/2013-April/msg01351.html
where Daniel suggested we use systemd-like passing of socket fd in combination with the LISTEN_FDS environment variable:
http://www.redhat.com/archives/libvir-list/2013-April/msg01356.html
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
Obviously that bug is focused on starting of the session daemon, but the code we're adding here should work with systemd. Have you tested that this actually allows for systemd activation of the privileged libvirtd. If we're adding this FD passing, I think we really ought to make sure we support this, so we don't have to revisit it later. Should add a libvirtd.socket unit file too so we have systemd activation by default for libvirtd.
Obviously I haven't. I just wanted to get rid of that silly, silly bug.
And now I tried and ... it Just Works(TM). I'll add a configuration comment and libvirtd.socket.in file in the patch and send a v3. Let me know what do you think of those permission settings there.
That's great ! Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Martin Kletzander