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

[Martin Kletzander]

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

Thanks for you thorough review, I squashed it in, remove that one
unnecessary pidfd = -1 and pushed it.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140917/edc17909/attachment-0001.sig>