[libvirt] [PATCH 2/2 RFC] Purge marked callbacks before dispatching events

[Michal Privoznik]

On 07.10.2016 19:52, Martin Kletzander wrote:
> I don't have any justification for this except an empirical one: with
> this patch the code from Bug 1363628 doesn't crash after "leaking".
> 
> I currently don't have properly working valgrind, but I'm working on it
> and it might shed some light into this (although it might also not
> happen due to the slowdown).
> 
> However in the meantime I attempted some analysis and I got even more
> confused than before, I guess.  With this patch the code doesn't crash,
> even though virObjectEventStateQueueDispatch() properly skips callbacks
> marked as deleted.  What I feel is even more weird, that if I duplicate
> the purgatory function (instead of moving it), it crashes again, and I
> feel like even more often.
> 
> Weirdly-fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1363628
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  src/conf/object_event.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/conf/object_event.c b/src/conf/object_event.c
> index e5af4be68a7e..4066b4673b42 100644
> --- a/src/conf/object_event.c
> +++ b/src/conf/object_event.c
> @@ -821,13 +821,13 @@ virObjectEventStateFlush(virObjectEventStatePtr state)
>      if (state->timer != -1)
>          virEventUpdateTimeout(state->timer, -1);
> 
> +    /* Purge any deleted callbacks */
> +    virObjectEventCallbackListPurgeMarked(state->callbacks);
> +
>      virObjectEventStateQueueDispatch(state,
>                                       &tempQueue,
>                                       state->callbacks);
> 
> -    /* Purge any deleted callbacks */
> -    virObjectEventCallbackListPurgeMarked(state->callbacks);
> -
>      state->isDispatching = false;
>      virObjectEventStateUnlock(state);
>  }

Well, I have no idea either, because virObjectEventStateQueueDispatch()
calls virObjectEventStateDispatchCallbacks() which in turn calls
virObjectEventDispatchMatchCallback() which returns false if the
callback->deleted is truem and thus the callback is skipped - just like
it is after this patch of yours.

This whole bug feels like a refcounting problem and IMO your patch is
just making the scenario more unlikely.

Michal