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@xxxxxxxxxx> --- 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 youif (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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list