subject:"\[Libevent\-users\] \[PATCH\] event.c\: make internal signal event count as active"

Re: [Libevent-users] [PATCH] event.c: make internal signal event count as active

2007-11-11 Thread Niels Provos

Thank you.  I applied this to trunk.

Niels.

On Nov 10, 2007 9:14 PM, Christopher Layne [EMAIL PROTECTED] wrote:
 On Sat, Nov 10, 2007 at 06:49:58PM -0800, Christopher Layne wrote:
  On Sat, Nov 10, 2007 at 06:44:12PM -0800, Christopher Layne wrote:
   11. here's the funny part: evsignal_process() increments 
   event_count_active - but since our

 Here's a better patch:

 This removes docount entirely. docount is used to determine if the event being
 added or removed from the queue should influence base-event_count. The
 internal signal event should not be counted as an event to wait for
 - such that when one deletes all their events the event loop will
 not count the internal signal event as something to wait around for
 (nothing changes here). However, based on the previous discussion, it
 still needs to be processed as a normal active event, hence we change
 base-event_count_active regardless of if it's internal or not.

 -cl

 Index: event.c
 ===
 --- event.c (revision 507)
 +++ event.c (working copy)
 @@ -829,23 +829,17 @@
  void
  event_queue_remove(struct event_base *base, struct event *ev, int queue)
  {
 -   int docount = 1;
 -
 if (!(ev-ev_flags  queue))
 event_errx(1, %s: %p(fd %d) not on queue %x, __func__,
ev, ev-ev_fd, queue);

 -   if (ev-ev_flags  EVLIST_INTERNAL)
 -   docount = 0;
 -
 -   if (docount)
 +   if (~ev-ev_flags  EVLIST_INTERNAL)
 base-event_count--;

 ev-ev_flags = ~queue;
 switch (queue) {
 case EVLIST_ACTIVE:
 -   if (docount)
 -   base-event_count_active--;
 +   base-event_count_active--;
 TAILQ_REMOVE(base-activequeues[ev-ev_pri],
 ev, ev_active_next);
 break;
 @@ -866,8 +860,6 @@
  void
  event_queue_insert(struct event_base *base, struct event *ev, int queue)
  {
 -   int docount = 1;
 -
 if (ev-ev_flags  queue) {
 /* Double insertion is possible for active events */
 if (queue  EVLIST_ACTIVE)
 @@ -877,17 +869,13 @@
ev, ev-ev_fd, queue);
 }

 -   if (ev-ev_flags  EVLIST_INTERNAL)
 -   docount = 0;
 -
 -   if (docount)
 +   if (~ev-ev_flags  EVLIST_INTERNAL)
 base-event_count++;

 ev-ev_flags |= queue;
 switch (queue) {
 case EVLIST_ACTIVE:
 -   if (docount)
 -   base-event_count_active++;
 +   base-event_count_active++;
 TAILQ_INSERT_TAIL(base-activequeues[ev-ev_pri],
 ev,ev_active_next);
 break;

 ___

 Libevent-users mailing list
 Libevent-users@monkey.org
 http://monkeymail.org/mailman/listinfo/libevent-users


___
Libevent-users mailing list
Libevent-users@monkey.org
http://monkeymail.org/mailman/listinfo/libevent-users

[Libevent-users] [PATCH] event.c: make internal signal event count as active

2007-11-10 Thread Christopher Layne

[ WARNING: Long and ardorous - difficult to trace behaviour ]

Behaviour described is without EV_PERSIST on internal signal handler (as it's
been for months):

1. signal comes in, we jump to our internal signal handler, it writes to pipe.
2. epoll_wait() returns -1/EINTR because a signal handler was called.
3. we do not process epoll events here, we call evsignal_process() and return 
from dispatch.
4. our user-side event is deleted from queues, marked active, and processed 
normally BY evsignal_process().
5. user-side event then adds itself back normally to handle signals again.
6. we head back into epoll_dispatch(), where epoll_wait() immediately returns 
because
signal pipe is ready for reading (from step 1).
7. we implicitly delete the internal signal event (remember, this is without 
EV_PERSIST on it) and
mark is as active. however, since it is an internal event, we do not increment 
event_count_active.
8. we return to our main event loop within event.c, and do not call 
event_process_active() because
event_count_active == 0. this is where the problems really start.
9. we head back into epoll_dispatch() and wait normally. however, since we 
previously deleted
our internal signal pipe fd from the epoll set, and did NOT call it from 
event_process_active() in
the previous step - the pipe never ends up being read. but since the user 
callback was executed
properly, everything looks normal.
10. we send the same signal again.  epoll_wait() returns, and we call 
evsignal_process() again.
11. here's the funny part: evsignal_process() increments event_count_active - 
but since our
internal signal event is actually first on the active queue at this point, when 
we go to
process events in the main loop - we actually end up processing our internal 
signal
event here. it reads from the pipe, and then re-adds itself back into epoll's 
watch set.
but since it's internal, we do not decrement event_count_active - guaranteeing 
we'll
get to our user event when we process the active events from the main loop. 
this is
the flip-flop.
12. repeat.

Now with EV_PERSIST on the internal signal event, steps 7-12 do not happen the 
first
time the signal is called. So not only is the bug still there, it's super bad 
when
it happens as epoll_wait() returns immediately. By sending a signal again - we 
can flip
it from spin to wait.

So this is a bug that needs to be fixed anyways. The EV_PERSIST change just
made it more visible. I'm not so sure how to make a regression test for this 
other
than a specific test to make sure that our signal pipe is empty AFTER returning 
from
event_process_active().

--
test case:
#include signal.h
#include stdlib.h
#include unistd.h
#include event.h

void scb(int sig, short event, void *a)
{
write(2, .\n, 2);
event_add(a, NULL);

return;
}

int main(void)
{
struct event sig;

event_init();
event_set(sig, SIGTSTP, EV_SIGNAL, scb, sig);
event_add(sig, NULL);
event_dispatch();

return 0;
}


--
debug log (unpatched):
$ ./sigtest
[debug] event_add: event: 0x7fff762c0500,call 0x400768
[debug] event_queue_insert: 0x7fff762c0500: docount == 1, event_count == 1, 
queue == 4
[debug] evsignal_add: evsignal  sh_old_max, s == 20, max == 0
[debug] event_add: event: 0x602050, EV_READ   call 0x2ac034a166d4
[debug] event_queue_insert: 0x602050: docount == 0, event_count == 1, queue == 2

wait, signal

[debug] evsignal_handler: wake up
[debug] epoll_dispatch: epoll_wait() res == -1, errno = 4
[debug] evsignal_process: processing signals, caught == 1
[debug] event_del: 0x7fff762c0500, callback 0x400768
[debug] evsignal_del: 0x7fff762c0500: restoring previous handler
[debug] event_active: 0x7fff762c0500: adding to active queue, ev_res == 0, res 
== 8
[debug] event_queue_insert: 0x7fff762c0500: docount == 1, event_count == 1, 
queue == 8
[debug] event_base_loop: event_count_active == 1
.
[debug] event_add: event: 0x7fff762c0500,call 0x400768
[debug] event_queue_insert: 0x7fff762c0500: docount == 1, event_count == 1, 
queue == 4
[debug] epoll_dispatch: epoll_wait reports 1
[debug] event_del: 0x602050, callback 0x2ac034a166d4
[debug] event_active: 0x602050: adding to active queue, ev_res == 0, res == 2
[debug] event_queue_insert: 0x602050: docount == 0, event_count == 1, queue == 8
[debug] event_base_loop: event_count_active == 0


wait, signal
(notice how we immediately call our internal event from event_base_loop?)


[debug] evsignal_handler: wake up
[debug] epoll_dispatch: epoll_wait() res == -1, errno = 4
[debug] evsignal_process: processing signals, caught == 1
[debug] event_del: 0x7fff762c0500, callback 0x400768
[debug] evsignal_del: 0x7fff762c0500: restoring previous handler
[debug] event_active: 0x7fff762c0500: adding to active queue, ev_res == 8, res 
== 8
[debug] event_queue_insert: 0x7fff762c0500: docount == 1, event_count == 1, 
queue == 8
[debug] event_base_loop: event_count_active == 1
[debug] evsignal_cb: n == 2

Re: [Libevent-users] [PATCH] event.c: make internal signal event count as active

2007-11-10 Thread Christopher Layne

On Sat, Nov 10, 2007 at 06:44:12PM -0800, Christopher Layne wrote:
 11. here's the funny part: evsignal_process() increments event_count_active - 
 but since our

should be:
evsignal_process() adds the internal event to the active queue as normal.
before we call event_process_active(), event_count_active is 1 at that point 
(user event).
but since our ...

 internal signal event is actually first on the active queue at this point, 
 when we go to
 process events in the main loop - we actually end up processing our internal 
 signal
 event here. it reads from the pipe, and then re-adds itself back into epoll's 
 watch set.
 but since it's internal, we do not decrement event_count_active - 
 guaranteeing we'll
 get to our user event when we process the active events from the main loop. 
 this is
 the flip-flop.

-cl
___
Libevent-users mailing list
Libevent-users@monkey.org
http://monkeymail.org/mailman/listinfo/libevent-users

Re: [Libevent-users] [PATCH] event.c: make internal signal event count as active

2007-11-10 Thread Christopher Layne

On Sat, Nov 10, 2007 at 06:49:58PM -0800, Christopher Layne wrote:
 On Sat, Nov 10, 2007 at 06:44:12PM -0800, Christopher Layne wrote:
  11. here's the funny part: evsignal_process() increments event_count_active 
  - but since our

Here's a better patch:

This removes docount entirely. docount is used to determine if the event being
added or removed from the queue should influence base-event_count. The
internal signal event should not be counted as an event to wait for
- such that when one deletes all their events the event loop will
not count the internal signal event as something to wait around for
(nothing changes here). However, based on the previous discussion, it
still needs to be processed as a normal active event, hence we change
base-event_count_active regardless of if it's internal or not.

-cl

Index: event.c
===
--- event.c (revision 507)
+++ event.c (working copy)
@@ -829,23 +829,17 @@
 void
 event_queue_remove(struct event_base *base, struct event *ev, int queue)
 {
-   int docount = 1;
-
if (!(ev-ev_flags  queue))
event_errx(1, %s: %p(fd %d) not on queue %x, __func__,
   ev, ev-ev_fd, queue);

-   if (ev-ev_flags  EVLIST_INTERNAL)
-   docount = 0;
-
-   if (docount)
+   if (~ev-ev_flags  EVLIST_INTERNAL)
base-event_count--;

ev-ev_flags = ~queue;
switch (queue) {
case EVLIST_ACTIVE:
-   if (docount)
-   base-event_count_active--;
+   base-event_count_active--;
TAILQ_REMOVE(base-activequeues[ev-ev_pri],
ev, ev_active_next);
break;
@@ -866,8 +860,6 @@
 void
 event_queue_insert(struct event_base *base, struct event *ev, int queue)
 {
-   int docount = 1;
-
if (ev-ev_flags  queue) {
/* Double insertion is possible for active events */
if (queue  EVLIST_ACTIVE)
@@ -877,17 +869,13 @@
   ev, ev-ev_fd, queue);
}

-   if (ev-ev_flags  EVLIST_INTERNAL)
-   docount = 0;
-
-   if (docount)
+   if (~ev-ev_flags  EVLIST_INTERNAL)
base-event_count++;

ev-ev_flags |= queue;
switch (queue) {
case EVLIST_ACTIVE:
-   if (docount)
-   base-event_count_active++;
+   base-event_count_active++;
TAILQ_INSERT_TAIL(base-activequeues[ev-ev_pri],
ev,ev_active_next);
break;

___
Libevent-users mailing list
Libevent-users@monkey.org
http://monkeymail.org/mailman/listinfo/libevent-users

Re: [Libevent-users] [PATCH] event.c: make internal signal event count as active

[Libevent-users] [PATCH] event.c: make internal signal event count as active

Re: [Libevent-users] [PATCH] event.c: make internal signal event count as active

Re: [Libevent-users] [PATCH] event.c: make internal signal event count as active

4 matches

Site Navigation

Mail list logo

Footer information