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

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(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. > 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