4:16 a.m.
This way it behaves more like the daemon itself does (acquiring a
pidfile, deleting the socket before binding, etc.).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/rpc/virnetsocket.c | 65 +++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 57 insertions(+), 8 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 80aeddf..7be1492 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -51,9 +51,11 @@
#include "virlog.h"
#include "virfile.h"
#include "virthread.h"
+#include "virpidfile.h"
#include "virprobe.h"
#include "virprocess.h"
#include "virstring.h"
+#include "dirname.h"
#include "passfd.h"
#if WITH_SSH2
@@ -541,7 +543,10 @@ int virNetSocketNewConnectUNIX(const char *path,
const char *binary,
virNetSocketPtr *retsock)
{
+ char *binname = NULL;
+ char *pidpath = NULL;
int fd, passfd = -1;
+ int pidfd = -1;
virSocketAddr localAddr;
virSocketAddr remoteAddr;
@@ -580,16 +585,47 @@ int virNetSocketNewConnectUNIX(const char *path,
goto error;
}
+ if (!(binname = last_component(binary)) || binname[0] == '\0') {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot determine basename for binary
'%s'"),
+ binary);
+ goto error;
+ }
+
+ if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0)
+ goto error;
+
+ pidfd = virPidFileAcquirePath(pidpath, false, getpid());
+ VIR_FREE(pidpath);
+ if (pidfd < 0) {
+ /*
+ * This can happen in a very rare case of two clients spawning two
+ * daemons at the same time, and the error in the logs that gets
+ * reset here can be a clue to some future debugging.
+ */
+ virResetLastError();
+ spawnDaemon = false;
+ goto retry;
+ }
+
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.
+ * We already even acquired the pidfile, so no one else should be using
+ * @path right now. So we're OK to unlink it and paying attention to
+ * the return value makes no real sense here. Only if it's not an
+ * abstract socket, of course.
+ */
+ if (path[0] != '@')
+ unlink(path);
+
+ /*
+ * 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;
@@ -607,12 +643,15 @@ int virNetSocketNewConnectUNIX(const char *path,
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.
+ * OK, so the child failed to bind() the socket. This may mean that
+ * another daemon was starting at the same time and succeeded with
+ * its bind() (even though it should not happen because we using a
+ * pidfile for the race). So we'll try connecting again, but this
+ * time without spawning the daemon.
*/
spawnDaemon = false;
+ VIR_FORCE_CLOSE(pidfd);
+ VIR_FORCE_CLOSE(passfd);
goto retry;
}
@@ -629,6 +668,12 @@ int virNetSocketNewConnectUNIX(const char *path,
goto error;
}
+ /*
+ * Do we need to eliminate the super-rare race here any more? It would
+ * need incorporating the following VIR_FORCE_CLOSE() into a
+ * virCommandHook inside a virNetSocketForkDaemon().
+ */
+ VIR_FORCE_CLOSE(pidfd);
if (virNetSocketForkDaemon(binary, passfd) < 0)
goto error;
}
@@ -645,8 +690,12 @@ int virNetSocketNewConnectUNIX(const char *path,
return 0;
error:
+ if (pidfd > 0)
+ virPidFileDeletePath(pidpath);
+ VIR_FREE(pidpath);
VIR_FORCE_CLOSE(fd);
VIR_FORCE_CLOSE(passfd);
+ VIR_FORCE_CLOSE(pidfd);
if (spawnDaemon)
unlink(path);
return -1;
--
2.1.0