Re: [PATCH 2/2] libxl: fix stale timeout event callback race

[Jim Fehlig <jfehlig@suse.com>]

Ian Jackson wrote:
> Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout event callback race"):
>   
>> Ian Jackson wrote:
>>     
>>> Well your patch is only correct when used with the new libxl, with my
>>> patches.
>>>       
>> Hmm, it is not clear to me how to make the libxl driver work correctly
>> with libxl pre and post your patches :-/.
>>     
>
> This should be straightforward I think, except for the deregistration
> race bug which is unavoidable with the old libxl.  Simply correctly
> implementing both the deregistration callback and the modification
> callback ought to do it.
>   

Yes, after thinking about it some more, I agree.

As for the modify callback, do you agree that it is fine to ignore the
timeval parameter and just update the timer in libvirt's event loop to
fire immediately?  I.e. always treat the timeval parameter as containing
{0,0} regardless of "old" or "new" libxl?  I think my patch here is correct

http://lists.xen.org/archives/html/xen-devel/2012-12/msg00985.html

>   
>>>> @@ -184,6 +184,8 @@ static void libxlTimerCallback(int timer
>>>> ATTRIBUTE_UNUSED, void *timer_v)
>>>>  {
>>>>      struct libxlOSEventHookTimerInfo *timer_info = timer_v;
>>>>  
>>>> +    virEventRemoveTimeout(timer_info->id);
>>>> +    timer_info->id = -1;
>>>>     
>>>>         
>>> I don't understand why you need this.  Doesn't libvirt remove timers
>>> when they fire ?  If it doesn't, do they otherwise not keep reoccurring ?
>>>       
>> No, timers are not removed when they fire.  And they are continuous, so
>> will keep firing at each timeout.  They can be disabled by setting the
>> timeout to -1, and can be set to fire on each iteration of the event
>> loop by setting the timeout to 0.  But they must be explicitly removed
>> with virEventRemoveTimeout when no longer needed.
>>     
>
> Right.  Well then yes you need to call virEventRemoveTimeout here.
> But that applies to both old and new libxl.
>   

Right.

Thanks for the help with getting this issue resolved!

Jim