[libvirt] [PATCH 5/5] rpc: make daemon spawning a bit more intelligent

[Martin Kletzander]

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 at redhat.com>
---
 src/rpc/virnetsocket.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 42184bd..18a5a8f 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
@@ -544,7 +546,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;

@@ -583,16 +588,45 @@ 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;
+
+        if ((pidfd = virPidFileAcquirePath(pidpath, false, getpid())) < 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 noone 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;
@@ -612,8 +646,9 @@ int virNetSocketNewConnectUNIX(const char *path,
             /*
              * 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.
+             * 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;
             goto retry;
@@ -632,6 +667,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;
     }
@@ -648,8 +689,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