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

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(a)redhat.com&gt; >> --- >> 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.

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

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;

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?