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:
>>     
>>> Specifically, this code has an integer arithmetic overflow.
>>>       
>> Well, this patch is removing that buggy code :).
>>     
>
> I think you need to have some code somewhere which does the
> complicated arithmetic dance to avoid the integer overflow.  Does your
> timer registration function have the same bug ?
>   

Ah yes.  I'll take care of it following your suggestion around
beforepoll_internal.

>   
>>  After again reading
>> libxl_event.h, I'm considering the below patch in the libvirt libxl
>> driver.  The change is primarily inspired by this comment for
>> libxl_osevent_occurred_timeout:
>>     
> ...
>   
>> /* Implicitly, on entry to this function the timeout has been
>>  * deregistered.  If _occurred_timeout is called, libxl will not
>>  * call timeout_deregister; if it wants to requeue the timeout it
>>  * will call timeout_register again.
>>  */
>>     
>
> 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 :-/.

>   
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 302f81c..d4055be 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -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.

Regards,
Jim