[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] rpc: make daemon spawning a bit more intelligent



On Wed, Sep 17, 2014 at 10:59:21AM -0400, John Ferlan wrote:


On 09/17/2014 10:00 AM, Martin Kletzander wrote:
On Tue, Sep 16, 2014 at 10:22:53AM -0400, John Ferlan wrote:


On 09/16/2014 05:16 AM, Martin Kletzander wrote:
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>
---
 src/rpc/virnetsocket.c | 65 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 8 deletions(-)


The error path/retry logic needs a tweak... I added some inline thinking
since we don't have a virtual whiteboard to share on this!


Yes, it does.  I didn't think it through from scratch, just adjusted
to your comments.  This time I went through it few times.  Just let me
confirm I understood what you meant everywhere.


You did :-)

<...snip...>


Wow, I just reached this part of the mail when I wrote it already :)

Funny how that happens.


My current diff to the previous version looks like this:

diff --git i/src/rpc/virnetsocket.c w/src/rpc/virnetsocket.c
index 7be1492..e0efb14 100644
--- i/src/rpc/virnetsocket.c
+++ w/src/rpc/virnetsocket.c
@@ -596,7 +596,6 @@ int virNetSocketNewConnectUNIX(const char *path,
             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
@@ -650,6 +649,7 @@ int virNetSocketNewConnectUNIX(const char *path,
              * time without spawning the daemon.
              */
             spawnDaemon = false;
+            virPidFileDeletePath(pidpath);
             VIR_FORCE_CLOSE(pidfd);
             VIR_FORCE_CLOSE(passfd);
             goto retry;
@@ -674,6 +674,7 @@ int virNetSocketNewConnectUNIX(const char *path,
          * virCommandHook inside a virNetSocketForkDaemon().
          */
         VIR_FORCE_CLOSE(pidfd);
+        pidfd = -1;

VIR_FORCE_CLOSE() will do this for you

         if (virNetSocketForkDaemon(binary, passfd) < 0)
             goto error;
     }
@@ -687,10 +688,12 @@ 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)
+    if (pidfd >= 0)
         virPidFileDeletePath(pidpath);
     VIR_FREE(pidpath);
     VIR_FORCE_CLOSE(fd);
--

Is that fine or should I use that goto error; everywhere after
acquiring the pidfile or is it better for you to see it in another
version?


This is fine - I think things are now covered.

ACK


Thanks for you thorough review, I squashed it in, remove that one
unnecessary pidfd = -1 and pushed it.

Martin

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]