Re: [libvirt] [PATCH v2 3/3] util: event loop: document another reason to defer deletion
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.
All this is true. I've just wanted to add why we need keep this state of affairs.
The thing is that in previous patch series I tried to free callback object
instantly rather then deffering freeing. This way I tried to address a specific
problem and this looks like possible solution. Then Daniel noted that this
implementation is public and I'll break API. Meanwhile I find out that I
actually can not free callback object without deffering and want to comment
on this.
Nikolay
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.
I've missed a word:
from a thread other then loop thread
> * Actual deletion will be done out-of-band
> */
> int virEventPollRemoveTimeout(int timer)
>