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&gt; --- 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