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) >> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list