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