[libvirt] [PATCH v2 3/3] util: event loop: document another reason to defer deletion

John Ferlan jferlan at redhat.com
Mon Nov 14 22:13:48 UTC 2016



On 11/14/2016 09:27 AM, Nikolay Shirokovskiy wrote:
> So the above patches are pushed now. What about the last one? I've elaborated
> on its meaning in sibling letter.
> 
> Nikolay
> 

You can always retry with a new patch that adds what you think will be
helpful. Keep in mind what I wrote, what you wrote, and propose the
adjustment.

I agree with your feeling that perhaps the code isn't as self
documenting as perhaps originally felt, but in order to alter the
descriptions (comments) - I think the new comments should be more
helpful. Like I pointed out in my response - I was having difficulty
figuring out what was being noted by just reading the new comments.
After reading the cover, other code, Dan's response to your original
series, I started to piece together things which is what I posted. I
think at one time I was going to note perhaps adjusting the docs as
well, e.g. http://libvirt.org/internals/eventloop.html

John

> On 28.10.2016 00:58, John Ferlan wrote:
>>
>>
>> On 10/04/2016 10:27 AM, Nikolay Shirokovskiy wrote:
>>> This is why we should not free callback object synchronously
>>> upon removing handle or timeout. Imagine:
>>>
>>> 1. loop thread,    drops the lock and is about to call event callback
>>> 2. another thread, enters remove handle and frees callback object
>>> 3. loop thread,    enters event callback, uses callback object BOOOM
>>> ---
>>>  src/util/vireventpoll.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>
>>
>> I'm having difficulty trying to decipher the point you're trying to make
>> in the context of the existing comments, the previous upstream series,
>> and the cover letter explanation.
>>
>> While not explicitly stated for each, the 'flag' that's set is
>> 'deleted'. Once the 'deleted' flag is set, it's expected that the object
>> will be reaped later when it's safer to do so (such as
>> virEventPollCleanupTimeouts and virEventPollCleanupHandles) via the 'ff'
>> passed during virEventAddHandle and virEventAddTimeout.
>>
>> I'm going to pass on pushing this one.
>>
>> John
>>
>>
>>> diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
>>> index 81ecab4..802b679 100644
>>> --- a/src/util/vireventpoll.c
>>> +++ b/src/util/vireventpoll.c
>>> @@ -176,6 +176,10 @@ void virEventPollUpdateHandle(int watch, int events)
>>>   * Unregister a callback from a file handle
>>>   * NB, it *must* be safe to call this from within a callback
>>>   * For this reason we only ever set a flag in the existing list.
>>> + * Another reason is that as we drop the lock upon event callback
>>> + * invocation we can't free callback object if we called
>>> + * from a thread then loop thread.
>>> + *
>>>   * Actual deletion will be done out-of-band
>>>   */
>>>  int virEventPollRemoveHandle(int watch)
>>> @@ -295,6 +299,9 @@ void virEventPollUpdateTimeout(int timer, int frequency)
>>>   * Unregister a callback for a timer
>>>   * NB, it *must* be safe to call this from within a callback
>>>   * For this reason we only ever set a flag in the existing list.
>>> + * Another reason is that as we drop the lock upon event callback
>>> + * invocation we can't free callback object if we called
>>> + * from a thread then loop thread.
>>>   * Actual deletion will be done out-of-band
>>>   */
>>>  int virEventPollRemoveTimeout(int timer)
>>>




More information about the libvir-list mailing list