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