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)