Re: [Libevent-users] [PATCH] signal.c, evsignal.h: properly save/restore previous signal handlers and fix memory stomp

2007-11-09 Thread Scott Lamb

Christopher Layne wrote:

+   /* save previous handler setup */
+   if (sigaction(evsignal, NULL, sig-sa_old[evsignal]) == -1
+   || sigaction(evsignal, sa, NULL) == -1)


Not worth changing unless you're redoing the patch anyway, but is there 
some reason you aren't doing this in a single call? I.e.,


if (sigaction(evsignal, sa, sig-sa_old[evsignal]) == -1) {

...


-   if (!base-sig.ev_signal_added) {
-   base-sig.ev_signal_added = 1;
-   event_add(base-sig.ev_signal, NULL);
+   if (!sig-ev_signal_added) {
+   sig-ev_signal_added = 1;
+   event_add(sig-ev_signal, NULL);
}


There's a bug here (that predates your change): this code should handle 
event_add() failure. (E.g., epoll_ctl() returning ENOMEM.)

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


Re: [Libevent-users] [PATCH] signal.c, evsignal.h: properly save/restore previous signal handlers and fix memory stomp

2007-11-09 Thread Christopher Layne
On Fri, Nov 09, 2007 at 04:03:46PM -0800, Scott Lamb wrote:
 Christopher Layne wrote:
 +/* save previous handler setup */
 +if (sigaction(evsignal, NULL, sig-sa_old[evsignal]) == -1
 +|| sigaction(evsignal, sa, NULL) == -1)
 
 Not worth changing unless you're redoing the patch anyway, but is there 
 some reason you aren't doing this in a single call? I.e.,
 
 if (sigaction(evsignal, sa, sig-sa_old[evsignal]) == -1) {

Good idea. I'll change it and resubmit it with the regress.c patch
also included (that Nick mentioned).

 ...
 
 -if (!base-sig.ev_signal_added) {
 -base-sig.ev_signal_added = 1;
 -event_add(base-sig.ev_signal, NULL);
 +if (!sig-ev_signal_added) {
 +sig-ev_signal_added = 1;
 +event_add(sig-ev_signal, NULL);
  }
 
 There's a bug here (that predates your change): this code should handle 
 event_add() failure. (E.g., epoll_ctl() returning ENOMEM.)

Fix in same, or sweep up in a later patch? How many other places are
there where we're not currently checking the return value of
event_add()? If there are more than this, we might as well just do it
separately.

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