5:30 a.m.
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
5:30 a.m.
New subject: [libvirt] [PATCH 1/3] Add missing error reporting
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(a)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
9:57 a.m.
New subject: [libvirt] [PATCH 1/3] Add missing error reporting
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(a)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
5:30 a.m.
New subject: [libvirt] [PATCH 2/3] Move daemonPidFilePath into virutil.c
Change daemonPidFilePath to virDaemonPidFilePath so it is accessible
in other parts of the code.
Signed-off-by: Martin Kletzander <mkletzan(a)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
5:41 a.m.
New subject: [libvirt] [PATCH 2/3] Move daemonPidFilePath into virutil.c
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(a)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 :|
5:30 a.m.
New subject: [libvirt] [PATCH 3/3] Speed up waiting for the session daemon
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(a)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
5:43 a.m.
New subject: [libvirt] [PATCH 3/3] Speed up waiting for the session daemon
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(a)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 :|
5:47 a.m.
New subject: [libvirt] [PATCH 3/3] Speed up waiting for the session daemon
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(a)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
5:53 a.m.
New subject: [libvirt] [PATCH 3/3] Speed up waiting for the session daemon
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(a)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 :|
4273
days inactive
4273
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Martin Kletzander