Re: [Qemu-devel] [PATCH] GuestAgent: PIDFILE remains when daemon start fails

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 01/06/2012 01:06 PM, Luiz Capitulino wrote:
> On Fri, 6 Jan 2012 17:05:53 +0000
> "Daniel P. Berrange"<berrange@redhat.com>  wrote:
>
>> On Fri, Jan 06, 2012 at 11:00:42AM -0600, Michael Roth wrote:
>>> On 01/06/2012 04:56 AM, Daniel P. Berrange wrote:
>>>> On Thu, Jan 05, 2012 at 06:18:26PM -0600, Michael Roth wrote:
>>>>> On 01/05/2012 04:26 PM, MATSUDA, Daiki wrote:
>>>>>> Hi, all.
>>>>>>
>>>>>> I am trying QEMU Guest Agent and encountered a small bug. It is that the
>>>>>> PIDFILE remains when daemon start fails. And maybe forgotton to g_free().
>>>>>>
>>>>>> MATSUDA, Daiki
>>>>>>
>>>>>
>>>>> Thanks for the patch. There was some contention in the past about
>>>>> whether or not to clean up pidfiles when there was abnormal
>>>>> termination, but personally I like this approach better.
>
> Ok, but can't we use atexit() instead then?

I guess I prefer it to this patch, but I don't believe that covers 
segfaults and the like, so maybe a combination of atexit() and F_SETLK 
would be best (as F_SETLK can still leave stale PID files, they just 
wouldn't obstruct subsequent instances, but we should still clean them 
up whenever we can)

>
>>>>
>>>> Yep, this still leaves open the problem of pidfile cleanup when the
>>>> daemon crashes. For libvirtd we recently switched over to a crash-safe
>>>> pidfile acquisition design, that uses fcntl(F_SETLK) to maintain
>>>> exclusive access over the pidfile. With this you don't need to worry
>>>> about forgetting to unlink() on termination, since the POSIX lock is
>>>> automatically released when process exits (or crashes).
>>>
>>> Yup, we did the same at some point via lockf(). An argument was made
>>> that stale PID files from unresolved crashes should stick around, so
>>> we dropped it. I think we should re-evaluate that decision...libvirt
>>> taking the same approach is pretty good precedence for me. I don't
>>> expect to have state from crashed programs interrupting attempts to
>>> restart them, it's more an unpleasant surprise than a feature, I
>>> think.
>
> Ok, I'll agree with you this time. Let's do it.
>
>>
>> Yeah, I think that is rather unpleasant, particularly for something
>> like qemu guest agent, which we want to try to ensure is reliably
>> running. In any case, if qemu guest agent is being launched by
>> something like SystemD, then you can configure whether systemd
>> will auto-restart it when it dies with non-zero exit status, so
>> I don't think we should delibrately leave stale pidfiles for that
>> scenario.
>>
>> Regards,
>> Daniel
>