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

[John Ferlan]

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 at 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

John