Re: [systemd-devel] [PATCH 1/2] sd-event: be more careful when enabling/disabling signals

2014-10-08 Thread Lennart Poettering
On Sun, 05.10.14 20:42, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:

  
 @@ -626,11 +634,13 @@ static void source_disconnect(sd_event_source *s) {
  
  case SOURCE_SIGNAL:
  if (s-signal.sig  0) {
 -if (s-signal.sig != SIGCHLD || 
 s-event-n_enabled_child_sources == 0)
 -assert_se(sigdelset(s-event-sigset, 
 s-signal.sig) == 0);
 -
  if (s-event-signal_sources)
  s-event-signal_sources[s-signal.sig] = 
 NULL;
 +
 +/* If the signal was on and now it is off... */
 +if (s-enabled != SD_EVENT_OFF  
 !need_signal(s-event, s-signal.sig)) {
 +assert_se(sigdelset(s-event-sigset, 
 s-signal.sig) == 0);
 +}

Coding style: the extra {} should go.

  s-event-n_enabled_child_sources--;
 -}
  
 -if (!s-event-signal_sources || 
 !s-event-signal_sources[SIGCHLD])
 -assert_se(sigdelset(s-event-sigset, 
 SIGCHLD) == 0);
 +/* We know the signal was on, if it is off 
 now... */
 +if (!need_signal(s-event, SIGCHLD)) {
 +
 assert_se(sigdelset(s-event-sigset, SIGCHLD) == 0);
 +}
 +}

Same here.

Patch looks good to me. If all tests pass and the system still boots
looks good to commit to me.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] [PATCH 1/2] sd-event: be more careful when enabling/disabling signals

2014-10-08 Thread Zbigniew Jędrzejewski-Szmek
On Wed, Oct 08, 2014 at 09:44:33PM +0200, Lennart Poettering wrote:
 On Sun, 05.10.14 20:42, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
 
   
  @@ -626,11 +634,13 @@ static void source_disconnect(sd_event_source *s) {
   
   case SOURCE_SIGNAL:
   if (s-signal.sig  0) {
  -if (s-signal.sig != SIGCHLD || 
  s-event-n_enabled_child_sources == 0)
  -assert_se(sigdelset(s-event-sigset, 
  s-signal.sig) == 0);
  -
   if (s-event-signal_sources)
   s-event-signal_sources[s-signal.sig] = 
  NULL;
  +
  +/* If the signal was on and now it is off... */
  +if (s-enabled != SD_EVENT_OFF  
  !need_signal(s-event, s-signal.sig)) {
  +assert_se(sigdelset(s-event-sigset, 
  s-signal.sig) == 0);
  +}
 
 Coding style: the extra {} should go.
This was in preparation for the next patch, which adds more the body.
Wonders of 'git add -p' :)

   s-event-n_enabled_child_sources--;
  -}
   
  -if (!s-event-signal_sources || 
  !s-event-signal_sources[SIGCHLD])
  -assert_se(sigdelset(s-event-sigset, 
  SIGCHLD) == 0);
  +/* We know the signal was on, if it is off 
  now... */
  +if (!need_signal(s-event, SIGCHLD)) {
  +
  assert_se(sigdelset(s-event-sigset, SIGCHLD) == 0);
  +}
  +}
 
 Same here.
 
 Patch looks good to me. If all tests pass and the system still boots
 looks good to commit to me.
Yeah, it boots. Pushed.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


[systemd-devel] [PATCH 1/2] sd-event: be more careful when enabling/disabling signals

2014-10-05 Thread Zbigniew Jędrzejewski-Szmek
When a child event is disabled (in order to be freed) and there is no
SIGCHLD signal event, sd_event_source_set_enabled will disable SIGCHLD
even if there are other child events.

Also remove some unneeded signalfd updates.

https://bugs.freedesktop.org/show_bug.cgi?id=84659

Based-on-a-patch-by: Hristo Venev mustrum...@gmail.com
---
I think this patch is correct, but since this is easy to get wrong,
I'm sending it to the list first.

 src/libsystemd/sd-event/sd-event.c | 86 --
 1 file changed, 63 insertions(+), 23 deletions(-)

diff --git a/src/libsystemd/sd-event/sd-event.c 
b/src/libsystemd/sd-event/sd-event.c
index 4c67ee87e1..c5f062b3e0 100644
--- a/src/libsystemd/sd-event/sd-event.c
+++ b/src/libsystemd/sd-event/sd-event.c
@@ -590,6 +590,14 @@ static struct clock_data* event_get_clock_data(sd_event 
*e, EventSourceType t) {
 }
 }
 
+static bool need_signal(sd_event *e, int signal) {
+return (e-signal_sources  e-signal_sources[signal] 
+e-signal_sources[signal]-enabled != SD_EVENT_OFF)
+||
+   (signal == SIGCHLD 
+e-n_enabled_child_sources  0);
+}
+
 static void source_disconnect(sd_event_source *s) {
 sd_event *event;
 
@@ -626,11 +634,13 @@ static void source_disconnect(sd_event_source *s) {
 
 case SOURCE_SIGNAL:
 if (s-signal.sig  0) {
-if (s-signal.sig != SIGCHLD || 
s-event-n_enabled_child_sources == 0)
-assert_se(sigdelset(s-event-sigset, 
s-signal.sig) == 0);
-
 if (s-event-signal_sources)
 s-event-signal_sources[s-signal.sig] = NULL;
+
+/* If the signal was on and now it is off... */
+if (s-enabled != SD_EVENT_OFF  
!need_signal(s-event, s-signal.sig)) {
+assert_se(sigdelset(s-event-sigset, 
s-signal.sig) == 0);
+}
 }
 
 break;
@@ -640,10 +650,12 @@ static void source_disconnect(sd_event_source *s) {
 if (s-enabled != SD_EVENT_OFF) {
 assert(s-event-n_enabled_child_sources  0);
 s-event-n_enabled_child_sources--;
-}
 
-if (!s-event-signal_sources || 
!s-event-signal_sources[SIGCHLD])
-assert_se(sigdelset(s-event-sigset, 
SIGCHLD) == 0);
+/* We know the signal was on, if it is off 
now... */
+if (!need_signal(s-event, SIGCHLD)) {
+assert_se(sigdelset(s-event-sigset, 
SIGCHLD) == 0);
+}
+}
 
 hashmap_remove(s-event-child_sources, 
INT_TO_PTR(s-child.pid));
 }
@@ -963,6 +975,7 @@ _public_ int sd_event_add_signal(
 sd_event_source *s;
 sigset_t ss;
 int r;
+bool previous;
 
 assert_return(e, -EINVAL);
 assert_return(sig  0, -EINVAL);
@@ -987,6 +1000,8 @@ _public_ int sd_event_add_signal(
 } else if (e-signal_sources[sig])
 return -EBUSY;
 
+previous = need_signal(e, sig);
+
 s = source_new(e, !ret, SOURCE_SIGNAL);
 if (!s)
 return -ENOMEM;
@@ -997,9 +1012,10 @@ _public_ int sd_event_add_signal(
 s-enabled = SD_EVENT_ON;
 
 e-signal_sources[sig] = s;
-assert_se(sigaddset(e-sigset, sig) == 0);
 
-if (sig != SIGCHLD || e-n_enabled_child_sources == 0) {
+if (!previous) {
+assert_se(sigaddset(e-sigset, sig) == 0);
+
 r = event_update_signal_fd(e);
 if (r  0) {
 source_free(s);
@@ -1023,6 +1039,7 @@ _public_ int sd_event_add_child(
 
 sd_event_source *s;
 int r;
+bool previous;
 
 assert_return(e, -EINVAL);
 assert_return(pid  1, -EINVAL);
@@ -1039,6 +1056,8 @@ _public_ int sd_event_add_child(
 if (hashmap_contains(e-child_sources, INT_TO_PTR(pid)))
 return -EBUSY;
 
+previous = need_signal(e, SIGCHLD);
+
 s = source_new(e, !ret, SOURCE_CHILD);
 if (!s)
 return -ENOMEM;
@@ -1057,9 +1076,9 @@ _public_ int sd_event_add_child(
 
 e-n_enabled_child_sources ++;
 
-assert_se(sigaddset(e-sigset, SIGCHLD) == 0);
+if (!previous) {
+assert_se(sigaddset(e-sigset, SIGCHLD) == 0);
 
-if (!e-signal_sources || !e-signal_sources[SIGCHLD]) {
 r = event_update_signal_fd(e);
 if (r  0) {
 source_free(s);
@@ -1437,23 +1456,32 @@ _public_ int 
sd_event_source_set_enabled(sd_event_source *s, int m) {
 }
 
 case