[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 :-)


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);
             goto retry;
@@ -674,6 +674,7 @@ int virNetSocketNewConnectUNIX(const char *path,
          * virCommandHook inside a virNetSocketForkDaemon().
+        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;

-    if (pidfd > 0)
+    if (pidfd >= 0)

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

This is fine - I think things are now covered.


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


Attachment: signature.asc
Description: Digital signature

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