[libvirt] [PATCH 0/3] Speeding up the wait for session daemon
When the session daemon is spawned and ends with an error before the spawning process notices, it becomes waiting for Godot (fortunately with a timeout of about 20 seconds). With this series, there is pidfile created for the spawned daemon and the pid is being checked for to make sure we are not wasting precious time. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Martin Kletzander (3): Add missing error reporting Move daemonPidFilePath into virutil.c Speed up waiting for the session daemon daemon/libvirtd.c | 43 +++---------------------------------------- src/libvirt_private.syms | 1 + src/rpc/virnetsocket.c | 41 +++++++++++++++++++++++++++++++++++------ src/util/virutil.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- src/util/virutil.h | 4 ++++ 5 files changed, 87 insertions(+), 47 deletions(-) -- 1.8.1.5
On two places, there were errors not being reported. One strdup without virReportOOMError() and call for virFileMakePathHelper() which doesn't report any errors, just sets errno (or leaves it set by underlying functions). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virutil.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 6890362..5fd0ce0 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1322,10 +1322,14 @@ virFileMakePathWithMode(const char *path, int ret = -1; char *tmp; - if ((tmp = strdup(path)) == NULL) + if ((tmp = strdup(path)) == NULL) { + virReportOOMError(); goto cleanup; + } ret = virFileMakePathHelper(tmp, mode); + if (ret < 0) + virReportSystemError(errno, _("Cannot create directory '%s'"), path); cleanup: VIR_FREE(tmp); -- 1.8.1.5
On 04/18/2013 04:30 AM, Martin Kletzander wrote:
On two places, there were errors not being reported. One strdup without virReportOOMError() and call for virFileMakePathHelper() which doesn't report any errors, just sets errno (or leaves it set by underlying functions).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virutil.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Needs work. Right now, this function intentionally does not report errors; all direct callers (src/util/virlockspace.c and tools/virsh.c) are aware of this convention. On the other hand, I looked through indirect callers of virFileMakePath(), and found at least the following problems: daemon/libvirtd.c: daemonPidFilePath() doesn't report an error src/conf/*_conf.c: 5 callers report error libxl_driver.c: reports errors with VIR_ERROR instead of virReportSystemError, so it gets logged but not reported to caller src/locking/lock_daemon.c: same bug as daemon/libvirt.c I stopped looking here - lots of other callers, and probably a similar mix of functions that report the error themselves, vs. functions that fail to do error reporting. We need to decide which way to go, then audit ALL users to obey that convention. Note that there are cases (such as tools/virsh.c) that intentionally want to suppress errors of EEXIST, except that virFileMakePathHelper already guarantees that EEXIST won't be the cause of failure, so maybe cleaning up ALL callers to expect virutil.c to do error reporting is the way to go. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Change daemonPidFilePath to virDaemonPidFilePath so it is accessible in other parts of the code. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- daemon/libvirtd.c | 43 +++---------------------------------------- src/libvirt_private.syms | 1 + src/util/virutil.c | 39 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 4 ++++ 4 files changed, 47 insertions(+), 40 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 38b7346..0dbe6d4 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1,7 +1,7 @@ /* * libvirtd.c: daemon start of day, guest process & i/o management * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -237,43 +237,6 @@ error: static int -daemonPidFilePath(bool privileged, - char **pidfile) -{ - if (privileged) { - if (!(*pidfile = strdup(LOCALSTATEDIR "/run/libvirtd.pid"))) - goto no_memory; - } else { - char *rundir = NULL; - mode_t old_umask; - - if (!(rundir = virGetUserRuntimeDirectory())) - goto error; - - old_umask = umask(077); - if (virFileMakePath(rundir) < 0) { - umask(old_umask); - goto error; - } - umask(old_umask); - - if (virAsprintf(pidfile, "%s/libvirtd.pid", rundir) < 0) { - VIR_FREE(rundir); - goto no_memory; - } - - VIR_FREE(rundir); - } - - return 0; - -no_memory: - virReportOOMError(); -error: - return -1; -} - -static int daemonUnixSocketPaths(struct daemonConfig *config, bool privileged, char **sockfile, @@ -1252,8 +1215,8 @@ int main(int argc, char **argv) { } if (!pid_file && - daemonPidFilePath(privileged, - &pid_file) < 0) { + virGetDaemonPidFilePath(privileged, + &pid_file) < 0) { VIR_ERROR(_("Can't determine pid file path.")); exit(EXIT_FAILURE); } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 30fdcd7..d1989fd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1869,6 +1869,7 @@ virFileWriteStr; virFindFCHostCapableVport; virFindFileInPath; virFormatIntDecimal; +virGetDaemonPidFilePath; virGetDeviceID; virGetDeviceUnprivSGIO; virGetFCHostNameByWWN; diff --git a/src/util/virutil.c b/src/util/virutil.c index 5fd0ce0..8b15894 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -90,6 +90,7 @@ #include "nonblocking.h" #include "passfd.h" #include "virprocess.h" +#include "configmake.h" #ifndef NSIG # define NSIG 32 @@ -2339,6 +2340,44 @@ check_and_return: return result; } + +int +virGetDaemonPidFilePath(bool privileged, + char **pidfile) +{ + if (privileged) { + if (!(*pidfile = strdup(LOCALSTATEDIR "/run/libvirtd.pid"))) + goto no_memory; + } else { + char *rundir = NULL; + mode_t old_umask; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto error; + + old_umask = umask(077); + if (virFileMakePath(rundir) < 0) { + umask(old_umask); + goto error; + } + umask(old_umask); + + if (virAsprintf(pidfile, "%s/libvirtd.pid", rundir) < 0) { + VIR_FREE(rundir); + goto no_memory; + } + + VIR_FREE(rundir); + } + + return 0; + +no_memory: + virReportOOMError(); +error: + return -1; +} + #ifdef HAVE_GETPWUID_R enum { VIR_USER_ENT_DIRECTORY, diff --git a/src/util/virutil.h b/src/util/virutil.h index 7b37d65..aa90c32 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -262,6 +262,10 @@ static inline int getgid (void) { return 0; } char *virGetHostname(virConnectPtr conn); +int virGetDaemonPidFilePath(bool privileged, + char **pidfile) + ATTRIBUTE_NONNULL(2); + char *virGetUserDirectory(void); char *virGetUserConfigDirectory(void); char *virGetUserCacheDirectory(void); -- 1.8.1.5
On Thu, Apr 18, 2013 at 12:30:55PM +0200, Martin Kletzander wrote:
Change daemonPidFilePath to virDaemonPidFilePath so it is accessible in other parts of the code.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- daemon/libvirtd.c | 43 +++---------------------------------------- src/libvirt_private.syms | 1 + src/util/virutil.c | 39 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 4 ++++ 4 files changed, 47 insertions(+), 40 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 38b7346..0dbe6d4 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1,7 +1,7 @@ /* * libvirtd.c: daemon start of day, guest process & i/o management * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -237,43 +237,6 @@ error:
static int -daemonPidFilePath(bool privileged, - char **pidfile) -{ - if (privileged) { - if (!(*pidfile = strdup(LOCALSTATEDIR "/run/libvirtd.pid"))) - goto no_memory; - } else { - char *rundir = NULL; - mode_t old_umask; - - if (!(rundir = virGetUserRuntimeDirectory())) - goto error; - - old_umask = umask(077); - if (virFileMakePath(rundir) < 0) { - umask(old_umask); - goto error; - } - umask(old_umask); - - if (virAsprintf(pidfile, "%s/libvirtd.pid", rundir) < 0) { - VIR_FREE(rundir); - goto no_memory; - } - - VIR_FREE(rundir); - } - - return 0; - -no_memory: - virReportOOMError(); -error: - return -1; -} - -static int daemonUnixSocketPaths(struct daemonConfig *config, bool privileged, char **sockfile, @@ -1252,8 +1215,8 @@ int main(int argc, char **argv) { }
if (!pid_file && - daemonPidFilePath(privileged, - &pid_file) < 0) { + virGetDaemonPidFilePath(privileged, + &pid_file) < 0) { VIR_ERROR(_("Can't determine pid file path.")); exit(EXIT_FAILURE); } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 30fdcd7..d1989fd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1869,6 +1869,7 @@ virFileWriteStr; virFindFCHostCapableVport; virFindFileInPath; virFormatIntDecimal; +virGetDaemonPidFilePath; virGetDeviceID; virGetDeviceUnprivSGIO; virGetFCHostNameByWWN; diff --git a/src/util/virutil.c b/src/util/virutil.c index 5fd0ce0..8b15894 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -90,6 +90,7 @@ #include "nonblocking.h" #include "passfd.h" #include "virprocess.h" +#include "configmake.h"
#ifndef NSIG # define NSIG 32 @@ -2339,6 +2340,44 @@ check_and_return: return result; }
+ +int +virGetDaemonPidFilePath(bool privileged, + char **pidfile) +{ + if (privileged) { + if (!(*pidfile = strdup(LOCALSTATEDIR "/run/libvirtd.pid"))) + goto no_memory; + } else { + char *rundir = NULL; + mode_t old_umask; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto error; + + old_umask = umask(077); + if (virFileMakePath(rundir) < 0) { + umask(old_umask); + goto error; + } + umask(old_umask); + + if (virAsprintf(pidfile, "%s/libvirtd.pid", rundir) < 0) { + VIR_FREE(rundir); + goto no_memory; + } + + VIR_FREE(rundir); + } + + return 0; + +no_memory: + virReportOOMError(); +error: + return -1; +}
This method is pretending to be generic, but in fact the path is completely specific to libvirtd, so won't work with virtlockd for example. 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 :|
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 When launching the session daemon, we were waiting for around 20 seconds even it died before creating any sockets etc. The following modification pre-creates the pidfile which will be used by the forked daemon and cleans it up in case the daemon doesn't start successfully. This makes it possible to check whether the daemon started and died immediately. The pidfile is needed due to the process being daemonized. Neither creation nor locking the pidfile will fail in the daemon thanks to the same PID keeping the lock. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c4fd9ee..835e11f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -52,6 +52,7 @@ #include "virfile.h" #include "virthread.h" #include "virprocess.h" +#include "virpidfile.h" #include "passfd.h" @@ -119,20 +120,29 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket) #ifndef WIN32 -static int virNetSocketForkDaemon(const char *binary) +static int virNetSocketForkDaemon(const char *binary, char **pidfile) { - int ret; + int ret = -1; virCommandPtr cmd = virCommandNewArgList(binary, "--timeout=30", NULL); + if (pidfile && + virGetDaemonPidFilePath(false, pidfile) < 0) { + goto cleanup; + } + virCommandAddEnvPassCommon(cmd); virCommandAddEnvPass(cmd, "XDG_CACHE_HOME"); virCommandAddEnvPass(cmd, "XDG_CONFIG_HOME"); virCommandAddEnvPass(cmd, "XDG_RUNTIME_DIR"); virCommandClearCaps(cmd); virCommandDaemonize(cmd); + if (pidfile) + virCommandSetPidFile(cmd, *pidfile); ret = virCommandRun(cmd, NULL); + + cleanup: virCommandFree(cmd); return ret; } @@ -523,6 +533,9 @@ int virNetSocketNewConnectUNIX(const char *path, virSocketAddr remoteAddr; int fd; int retries = 0; + char *pidfile = NULL; + pid_t pid; + int ret = -1; memset(&localAddr, 0, sizeof(localAddr)); memset(&remoteAddr, 0, sizeof(remoteAddr)); @@ -556,8 +569,24 @@ retry: VIR_DEBUG("Connection refused for %s, trying to spawn %s", path, binary); if (retries == 0 && - virNetSocketForkDaemon(binary) < 0) + virNetSocketForkDaemon(binary, &pidfile) < 0) + goto error; + + ret = virPidFileReadPathIfAlive(pidfile, &pid, binary); + if (ret < 0) { + virReportSystemError(-ret, + _("Cannot read pidfile '%s'"), + pidfile); goto error; + } + ret = -1; + + if (pid == -1) { + virPidFileDeletePath(pidfile); + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("Spawned binary died prematurely")); + goto error; + } retries++; usleep(1000 * 100 * retries); @@ -579,11 +608,11 @@ retry: if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) goto error; - return 0; - + ret = 0; error: + VIR_FREE(pidfile); VIR_FORCE_CLOSE(fd); - return -1; + return ret; } #else int virNetSocketNewConnectUNIX(const char *path ATTRIBUTE_UNUSED, -- 1.8.1.5
On Thu, Apr 18, 2013 at 12:30:56PM +0200, Martin Kletzander wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
When launching the session daemon, we were waiting for around 20 seconds even it died before creating any sockets etc.
The following modification pre-creates the pidfile which will be used by the forked daemon and cleans it up in case the daemon doesn't start successfully. This makes it possible to check whether the daemon started and died immediately.
The pidfile is needed due to the process being daemonized. Neither creation nor locking the pidfile will fail in the daemon thanks to the same PID keeping the lock.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c4fd9ee..835e11f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -52,6 +52,7 @@ #include "virfile.h" #include "virthread.h" #include "virprocess.h" +#include "virpidfile.h"
#include "passfd.h"
@@ -119,20 +120,29 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket)
#ifndef WIN32 -static int virNetSocketForkDaemon(const char *binary) +static int virNetSocketForkDaemon(const char *binary, char **pidfile) { - int ret; + int ret = -1; virCommandPtr cmd = virCommandNewArgList(binary, "--timeout=30", NULL);
+ if (pidfile && + virGetDaemonPidFilePath(false, pidfile) < 0) { + goto cleanup; + }
And here, you're hardcoding use of the libvirtd pid file in a method that is used to spawn any type of daemon. The much better way to deal with this is to follow the approach that systemd uses for race-free activation of daemons. Namely pass in the pre-opened listen socket to the daemon. We already support this for the virtlockd daemon and it would be easy to support it for libvirtd too. This leaves the daemons in charge of their own pidfiles. 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 04/18/2013 12:43 PM, Daniel P. Berrange wrote:
On Thu, Apr 18, 2013 at 12:30:56PM +0200, Martin Kletzander wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
When launching the session daemon, we were waiting for around 20 seconds even it died before creating any sockets etc.
The following modification pre-creates the pidfile which will be used by the forked daemon and cleans it up in case the daemon doesn't start successfully. This makes it possible to check whether the daemon started and died immediately.
The pidfile is needed due to the process being daemonized. Neither creation nor locking the pidfile will fail in the daemon thanks to the same PID keeping the lock.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c4fd9ee..835e11f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -52,6 +52,7 @@ #include "virfile.h" #include "virthread.h" #include "virprocess.h" +#include "virpidfile.h"
#include "passfd.h"
@@ -119,20 +120,29 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket)
#ifndef WIN32 -static int virNetSocketForkDaemon(const char *binary) +static int virNetSocketForkDaemon(const char *binary, char **pidfile) { - int ret; + int ret = -1; virCommandPtr cmd = virCommandNewArgList(binary, "--timeout=30", NULL);
+ if (pidfile && + virGetDaemonPidFilePath(false, pidfile) < 0) { + goto cleanup; + }
And here, you're hardcoding use of the libvirtd pid file in a method that is used to spawn any type of daemon.
The much better way to deal with this is to follow the approach that systemd uses for race-free activation of daemons. Namely pass in the pre-opened listen socket to the daemon. We already support this for the virtlockd daemon and it would be easy to support it for libvirtd too. This leaves the daemons in charge of their own pidfiles.
I completely missed the fact that this could be used for something else than libvirtd, thanks for pointing that out. Could you point me to the place where we use the approach you described? Thanks, Martin
On Thu, Apr 18, 2013 at 12:47:46PM +0200, Martin Kletzander wrote:
On 04/18/2013 12:43 PM, Daniel P. Berrange wrote:
On Thu, Apr 18, 2013 at 12:30:56PM +0200, Martin Kletzander wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
When launching the session daemon, we were waiting for around 20 seconds even it died before creating any sockets etc.
The following modification pre-creates the pidfile which will be used by the forked daemon and cleans it up in case the daemon doesn't start successfully. This makes it possible to check whether the daemon started and died immediately.
The pidfile is needed due to the process being daemonized. Neither creation nor locking the pidfile will fail in the daemon thanks to the same PID keeping the lock.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c4fd9ee..835e11f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -52,6 +52,7 @@ #include "virfile.h" #include "virthread.h" #include "virprocess.h" +#include "virpidfile.h"
#include "passfd.h"
@@ -119,20 +120,29 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket)
#ifndef WIN32 -static int virNetSocketForkDaemon(const char *binary) +static int virNetSocketForkDaemon(const char *binary, char **pidfile) { - int ret; + int ret = -1; virCommandPtr cmd = virCommandNewArgList(binary, "--timeout=30", NULL);
+ if (pidfile && + virGetDaemonPidFilePath(false, pidfile) < 0) { + goto cleanup; + }
And here, you're hardcoding use of the libvirtd pid file in a method that is used to spawn any type of daemon.
The much better way to deal with this is to follow the approach that systemd uses for race-free activation of daemons. Namely pass in the pre-opened listen socket to the daemon. We already support this for the virtlockd daemon and it would be easy to support it for libvirtd too. This leaves the daemons in charge of their own pidfiles.
I completely missed the fact that this could be used for something else than libvirtd, thanks for pointing that out. Could you point me to the place where we use the approach you described?
Look at the method virLockDaemonSetupNetworkingSystemD in src/locking/lock_daemon.c This allows virtlockd to inherit a pre-opened UNIX domain socket from systemd when it starts. We can easily do the same in libvirtd (don't need to worry about TCP sockets). Then the method virNetSocketForkDaemon() could be made to pass in such a pre-opened socket, so the client has the ability to connect. Basically youd want to - Create the listen socket in the client - Connect to the listen socket in the client - Spawn the daemon, passing in in the listen socket So you have no race there, but if the daemon fails to complete its startup for whatever reason the listen socket will be closed & the client will see EOF as normal. 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