From b2735463b886ac88027cec0ff1a4dbaa4f8be739 Mon Sep 17 00:00:00 2001

From: Martin Kletzander <mkletzan@redhat.com>

Date: Sun, 7 Sep 2014 20:41:11 +0200

Subject: [PATCH] rpc: make daemon spawning a bit more intelligent

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@redhat.com>

(cherry picked from commit bd9ad91a4036649645fffb1598213339263478de)

---

 src/rpc/virnetsocket.c | 67 ++++++++++++++++++++++++++++++++++++++++++++------

 1 file changed, 59 insertions(+), 8 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c

index 306c9ea..5feccf6 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,46 @@ 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());

+        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;

@@ -610,12 +645,16 @@ 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;

+            virPidFileDeletePath(pidpath);

+            VIR_FORCE_CLOSE(pidfd);

+            VIR_FORCE_CLOSE(passfd);

             goto retry;

         }

@@ -632,6 +671,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,11 +690,17 @@ int virNetSocketNewConnectUNIX(const char *path,

     if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0)))

         goto error;

+    VIR_FREE(pidpath);

+

     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;