event: rework sd-event exit logic

With this change a failing event source handler will not cause the
entire event loop to fail. Instead, we just disable the specific event
source, log a message at debug level and go on.

This also introduces a new concept of "exit code" which can be stored in
the event loop and is returned by sd_event_loop(). We also rename "quit"
to "exit" everywhere else.

Altogether this should make things more robus and keep errors local
while still providing a way to return event loop errors in a clear way.

diff --git a/TODO b/TODO
index dad55c4..4274b37 100644 (file)
--- a/TODO
+++ b/TODO
@@ -131,13 +131,10 @@ Features:
   - longer term:
     * priority queues
     * priority inheritance
+  - fix sd-event hookup when we connect to multiple servers one after the other
 
 * sd-event
   - allow multiple signal handlers per signal
-  - when a handler returns an error, just turn off its event source,
-    but do not return anything up to the event loop caller. Instead
-    add parameter to sd_event_request_quit() to take retval. This way
-    errors rippling upwards are the option, not the default
   - when dispatching an event source then _unref() on it should remove it from the epoll
 
 * in the final killing spree, detect processes from the root directory, and

diff --git a/src/hostname/hostnamed.c b/src/hostname/hostnamed.c
index 3f08d1c..eb2b35f 100644 (file)
--- a/src/hostname/hostnamed.c
+++ b/src/hostname/hostnamed.c
@@ -645,8 +645,6 @@ int main(int argc, char *argv[]) {
                 goto finish;
         }
 
-        r = 0;
-
 finish:
         context_free(&context, bus);
 

diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c
index 0f67fb8..3f8b95d 100644 (file)
--- a/src/journal/journald-server.c
+++ b/src/journal/journald-server.c
@@ -1243,7 +1243,7 @@ static int dispatch_sigterm(sd_event_source *es, const struct signalfd_siginfo *
 
         log_info("Received SIG%s", signal_to_string(si->ssi_signo));
 
-        sd_event_request_quit(s->event);
+        sd_event_exit(s->event, 0);
         return 0;
 }
 

diff --git a/src/libsystemd-bus/bus-util.c b/src/libsystemd-bus/bus-util.c
index 3afcb82..30ee67e 100644 (file)
--- a/src/libsystemd-bus/bus-util.c
+++ b/src/libsystemd-bus/bus-util.c
@@ -35,18 +35,18 @@
 #include "bus-util.h"
 #include "bus-internal.h"
 
-static int quit_callback(sd_bus *bus, sd_bus_message *m, void *userdata, sd_bus_error *ret_error) {
+static int name_owner_change_callback(sd_bus *bus, sd_bus_message *m, void *userdata, sd_bus_error *ret_error) {
         sd_event *e = userdata;
 
         assert(bus);
         assert(m);
         assert(e);
 
-        sd_event_request_quit(e);
+        sd_event_exit(e, 0);
         return 1;
 }
 
-int bus_async_unregister_and_quit(sd_event *e, sd_bus *bus, const char *name) {
+int bus_async_unregister_and_exit(sd_event *e, sd_bus *bus, const char *name) {
         _cleanup_free_ char *match = NULL;
         const char *unique;
         int r;
@@ -55,6 +55,11 @@ int bus_async_unregister_and_quit(sd_event *e, sd_bus *bus, const char *name) {
         assert(bus);
         assert(name);
 
+        /* We unregister the name here and then wait for the
+         * NameOwnerChanged signal for this event to arrive before we
+         * quit. We do this in order to make sure that any queued
+         * requests are still processed before we really exit. */
+
         r = sd_bus_get_unique_name(bus, &unique);
         if (r < 0)
                 return r;
@@ -71,7 +76,7 @@ int bus_async_unregister_and_quit(sd_event *e, sd_bus *bus, const char *name) {
         if (r < 0)
                 return -ENOMEM;
 
-        r = sd_bus_add_match(bus, match, quit_callback, e);
+        r = sd_bus_add_match(bus, match, name_owner_change_callback, e);
         if (r < 0)
                 return r;
 
@@ -84,7 +89,7 @@ int bus_async_unregister_and_quit(sd_event *e, sd_bus *bus, const char *name) {
 
 int bus_event_loop_with_idle(sd_event *e, sd_bus *bus, const char *name, usec_t timeout) {
         bool exiting = false;
-        int r;
+        int r, code;
 
         assert(e);
         assert(bus);
@@ -103,7 +108,7 @@ int bus_event_loop_with_idle(sd_event *e, sd_bus *bus, const char *name, usec_t
                         return r;
 
                 if (r == 0 && !exiting) {
-                        r = bus_async_unregister_and_quit(e, bus, name);
+                        r = bus_async_unregister_and_exit(e, bus, name);
                         if (r < 0)
                                 return r;
 
@@ -111,7 +116,11 @@ int bus_event_loop_with_idle(sd_event *e, sd_bus *bus, const char *name, usec_t
                 }
         }
 
-        return 0;
+        r = sd_event_get_exit_code(e, &code);
+        if (r < 0)
+                return r;
+
+        return code;
 }
 
 int bus_name_has_owner(sd_bus *c, const char *name, sd_bus_error *error) {

diff --git a/src/libsystemd-bus/bus-util.h b/src/libsystemd-bus/bus-util.h
index 9d49237..9a90c2a 100644 (file)
--- a/src/libsystemd-bus/bus-util.h
+++ b/src/libsystemd-bus/bus-util.h
@@ -52,7 +52,7 @@ int bus_map_all_properties(sd_bus *bus,
                            const struct bus_properties_map *map,
                            void *userdata);
 
-int bus_async_unregister_and_quit(sd_event *e, sd_bus *bus, const char *name);
+int bus_async_unregister_and_exit(sd_event *e, sd_bus *bus, const char *name);
 
 int bus_event_loop_with_idle(sd_event *e, sd_bus *bus, const char *name, usec_t timeout);
 

diff --git a/src/libsystemd-bus/libsystemd-bus.sym b/src/libsystemd-bus/libsystemd-bus.sym
index 4a849b3..f3dbb76 100644 (file)
--- a/src/libsystemd-bus/libsystemd-bus.sym
+++ b/src/libsystemd-bus/libsystemd-bus.sym
@@ -227,15 +227,15 @@ global:
         sd_event_add_signal;
         sd_event_add_child;
         sd_event_add_defer;
-        sd_event_add_quit;
+        sd_event_add_exit;
 
         sd_event_run;
         sd_event_loop;
+        sd_event_exit;
 
         sd_event_get_state;
         sd_event_get_tid;
-        sd_event_get_quit;
-        sd_event_request_quit;
+        sd_event_get_exit_code;
         sd_event_get_now_realtime;
         sd_event_get_now_monotonic;
         sd_event_set_watchdog;

diff --git a/src/libsystemd-bus/sd-bus.c b/src/libsystemd-bus/sd-bus.c
index 060dcc1..a4709a1 100644 (file)
--- a/src/libsystemd-bus/sd-bus.c
+++ b/src/libsystemd-bus/sd-bus.c
@@ -2646,7 +2646,7 @@ _public_ int sd_bus_attach_event(sd_bus *bus, sd_event *event, int priority) {
         if (r < 0)
                 goto fail;
 
-        r = sd_event_add_quit(bus->event, quit_callback, bus, &bus->quit_event_source);
+        r = sd_event_add_exit(bus->event, quit_callback, bus, &bus->quit_event_source);
         if (r < 0)
                 goto fail;
 

diff --git a/src/libsystemd-bus/sd-event.c b/src/libsystemd-bus/sd-event.c
index 462dd41..b396432 100644 (file)
--- a/src/libsystemd-bus/sd-event.c
+++ b/src/libsystemd-bus/sd-event.c
@@ -44,7 +44,7 @@ typedef enum EventSourceType {
         SOURCE_SIGNAL,
         SOURCE_CHILD,
         SOURCE_DEFER,
-        SOURCE_QUIT,
+        SOURCE_EXIT,
         SOURCE_WATCHDOG
 } EventSourceType;
 
@@ -96,7 +96,7 @@ struct sd_event_source {
                 struct {
                         sd_event_handler_t callback;
                         unsigned prioq_index;
-                } quit;
+                } exit;
         };
 };
 
@@ -132,7 +132,7 @@ struct sd_event {
         Hashmap *child_sources;
         unsigned n_enabled_child_sources;
 
-        Prioq *quit;
+        Prioq *exit;
 
         pid_t original_pid;
 
@@ -140,10 +140,12 @@ struct sd_event {
         dual_timestamp timestamp;
         int state;
 
-        bool quit_requested:1;
+        bool exit_requested:1;
         bool need_process_child:1;
         bool watchdog:1;
 
+        int exit_code;
+
         pid_t tid;
         sd_event **default_event_ptr;
 
@@ -284,11 +286,11 @@ static int latest_time_prioq_compare(const void *a, const void *b) {
         return 0;
 }
 
-static int quit_prioq_compare(const void *a, const void *b) {
+static int exit_prioq_compare(const void *a, const void *b) {
         const sd_event_source *x = a, *y = b;
 
-        assert(x->type == SOURCE_QUIT);
-        assert(y->type == SOURCE_QUIT);
+        assert(x->type == SOURCE_EXIT);
+        assert(y->type == SOURCE_EXIT);
 
         /* Enabled ones first */
         if (x->enabled != SD_EVENT_OFF && y->enabled == SD_EVENT_OFF)
@@ -338,7 +340,7 @@ static void event_free(sd_event *e) {
         prioq_free(e->monotonic_latest);
         prioq_free(e->realtime_earliest);
         prioq_free(e->realtime_latest);
-        prioq_free(e->quit);
+        prioq_free(e->exit);
 
         free(e->signal_sources);
 
@@ -515,8 +517,8 @@ static void source_free(sd_event_source *s) {
                         /* nothing */
                         break;
 
-                case SOURCE_QUIT:
-                        prioq_remove(s->event->quit, s, &s->quit.prioq_index);
+                case SOURCE_EXIT:
+                        prioq_remove(s->event->exit, s, &s->exit.prioq_index);
                         break;
                 }
 
@@ -536,7 +538,7 @@ static int source_set_pending(sd_event_source *s, bool b) {
         int r;
 
         assert(s);
-        assert(s->type != SOURCE_QUIT);
+        assert(s->type != SOURCE_EXIT);
 
         if (s->pending == b)
                 return 0;
@@ -934,7 +936,7 @@ _public_ int sd_event_add_defer(
         return 0;
 }
 
-_public_ int sd_event_add_quit(
+_public_ int sd_event_add_exit(
                 sd_event *e,
                 sd_event_handler_t callback,
                 void *userdata,
@@ -949,22 +951,22 @@ _public_ int sd_event_add_quit(
         assert_return(e->state != SD_EVENT_FINISHED, -ESTALE);
         assert_return(!event_pid_changed(e), -ECHILD);
 
-        if (!e->quit) {
-                e->quit = prioq_new(quit_prioq_compare);
-                if (!e->quit)
+        if (!e->exit) {
+                e->exit = prioq_new(exit_prioq_compare);
+                if (!e->exit)
                         return -ENOMEM;
         }
 
-        s = source_new(e, SOURCE_QUIT);
+        s = source_new(e, SOURCE_EXIT);
         if (!s)
                 return -ENOMEM;
 
-        s->quit.callback = callback;
+        s->exit.callback = callback;
         s->userdata = userdata;
-        s->quit.prioq_index = PRIOQ_IDX_NULL;
+        s->exit.prioq_index = PRIOQ_IDX_NULL;
         s->enabled = SD_EVENT_ONESHOT;
 
-        r = prioq_put(s->event->quit, s, &s->quit.prioq_index);
+        r = prioq_put(s->event->exit, s, &s->exit.prioq_index);
         if (r < 0) {
                 source_free(s);
                 return r;
@@ -1005,7 +1007,7 @@ _public_ sd_event *sd_event_source_get_event(sd_event_source *s) {
 
 _public_ int sd_event_source_get_pending(sd_event_source *s) {
         assert_return(s, -EINVAL);
-        assert_return(s->type != SOURCE_QUIT, -EDOM);
+        assert_return(s->type != SOURCE_EXIT, -EDOM);
         assert_return(s->event->state != SD_EVENT_FINISHED, -ESTALE);
         assert_return(!event_pid_changed(s->event), -ECHILD);
 
@@ -1096,8 +1098,8 @@ _public_ int sd_event_source_set_priority(sd_event_source *s, int priority) {
         if (s->prepare)
                 prioq_reshuffle(s->event->prepare, s, &s->prepare_index);
 
-        if (s->type == SOURCE_QUIT)
-                prioq_reshuffle(s->event->quit, s, &s->quit.prioq_index);
+        if (s->type == SOURCE_EXIT)
+                prioq_reshuffle(s->event->exit, s, &s->exit.prioq_index);
 
         return 0;
 }
@@ -1168,9 +1170,9 @@ _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) {
 
                         break;
 
-                case SOURCE_QUIT:
+                case SOURCE_EXIT:
                         s->enabled = m;
-                        prioq_reshuffle(s->event->quit, s, &s->quit.prioq_index);
+                        prioq_reshuffle(s->event->exit, s, &s->exit.prioq_index);
                         break;
 
                 case SOURCE_DEFER:
@@ -1223,9 +1225,9 @@ _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) {
                         }
                         break;
 
-                case SOURCE_QUIT:
+                case SOURCE_EXIT:
                         s->enabled = m;
-                        prioq_reshuffle(s->event->quit, s, &s->quit.prioq_index);
+                        prioq_reshuffle(s->event->exit, s, &s->exit.prioq_index);
                         break;
 
                 case SOURCE_DEFER:
@@ -1321,7 +1323,7 @@ _public_ int sd_event_source_set_prepare(sd_event_source *s, sd_event_handler_t
         int r;
 
         assert_return(s, -EINVAL);
-        assert_return(s->type != SOURCE_QUIT, -EDOM);
+        assert_return(s->type != SOURCE_EXIT, -EDOM);
         assert_return(s->event->state != SD_EVENT_FINISHED, -ESTALE);
         assert_return(!event_pid_changed(s->event), -ECHILD);
 
@@ -1673,9 +1675,9 @@ static int source_dispatch(sd_event_source *s) {
         int r = 0;
 
         assert(s);
-        assert(s->pending || s->type == SOURCE_QUIT);
+        assert(s->pending || s->type == SOURCE_EXIT);
 
-        if (s->type != SOURCE_DEFER && s->type != SOURCE_QUIT) {
+        if (s->type != SOURCE_DEFER && s->type != SOURCE_EXIT) {
                 r = source_set_pending(s, false);
                 if (r < 0)
                         return r;
@@ -1727,14 +1729,18 @@ static int source_dispatch(sd_event_source *s) {
                 r = s->defer.callback(s, s->userdata);
                 break;
 
-        case SOURCE_QUIT:
-                r = s->quit.callback(s, s->userdata);
+        case SOURCE_EXIT:
+                r = s->exit.callback(s, s->userdata);
                 break;
         }
 
-        sd_event_source_unref(s);
+        if (r < 0) {
+                log_debug("Event source %p returned error, disabling: %s", s, strerror(-r));
+                sd_event_source_set_enabled(s, SD_EVENT_OFF);
+        }
 
-        return r;
+        sd_event_source_unref(s);
+        return 1;
 }
 
 static int event_prepare(sd_event *e) {
@@ -1764,13 +1770,13 @@ static int event_prepare(sd_event *e) {
         return 0;
 }
 
-static int dispatch_quit(sd_event *e) {
+static int dispatch_exit(sd_event *e) {
         sd_event_source *p;
         int r;
 
         assert(e);
 
-        p = prioq_peek(e->quit);
+        p = prioq_peek(e->exit);
         if (!p || p->enabled == SD_EVENT_OFF) {
                 e->state = SD_EVENT_FINISHED;
                 return 0;
@@ -1778,7 +1784,7 @@ static int dispatch_quit(sd_event *e) {
 
         sd_event_ref(e);
         e->iteration++;
-        e->state = SD_EVENT_QUITTING;
+        e->state = SD_EVENT_EXITING;
 
         r = source_dispatch(p);
 
@@ -1850,8 +1856,8 @@ _public_ int sd_event_run(sd_event *e, uint64_t timeout) {
         assert_return(e->state != SD_EVENT_FINISHED, -ESTALE);
         assert_return(e->state == SD_EVENT_PASSIVE, -EBUSY);
 
-        if (e->quit_requested)
-                return dispatch_quit(e);
+        if (e->exit_requested)
+                return dispatch_exit(e);
 
         sd_event_ref(e);
         e->iteration++;
@@ -1946,7 +1952,7 @@ _public_ int sd_event_loop(sd_event *e) {
                         goto finish;
         }
 
-        r = 0;
+        r = e->exit_code;
 
 finish:
         sd_event_unref(e);
@@ -1960,19 +1966,26 @@ _public_ int sd_event_get_state(sd_event *e) {
         return e->state;
 }
 
-_public_ int sd_event_get_quit(sd_event *e) {
+_public_ int sd_event_get_exit_code(sd_event *e, int *code) {
         assert_return(e, -EINVAL);
+        assert_return(code, -EINVAL);
         assert_return(!event_pid_changed(e), -ECHILD);
 
-        return e->quit_requested;
+        if (!e->exit_requested)
+                return -ENODATA;
+
+        *code = e->exit_code;
+        return 0;
 }
 
-_public_ int sd_event_request_quit(sd_event *e) {
+_public_ int sd_event_exit(sd_event *e, int code) {
         assert_return(e, -EINVAL);
         assert_return(e->state != SD_EVENT_FINISHED, -ESTALE);
         assert_return(!event_pid_changed(e), -ECHILD);
 
-        e->quit_requested = true;
+        e->exit_requested = true;
+        e->exit_code = code;
+
         return 0;
 }
 

diff --git a/src/libsystemd-bus/test-event.c b/src/libsystemd-bus/test-event.c
index 2b91eb0..5317008 100644 (file)
--- a/src/libsystemd-bus/test-event.c
+++ b/src/libsystemd-bus/test-event.c
@@ -63,7 +63,7 @@ static int child_handler(sd_event_source *s, const siginfo_t *si, void *userdata
 
         assert(userdata == INT_TO_PTR('f'));
 
-        assert_se(sd_event_request_quit(sd_event_source_get_event(s)) >= 0);
+        assert_se(sd_event_exit(sd_event_source_get_event(s), 0) >= 0);
         sd_event_source_unref(s);
 
         return 1;
@@ -143,12 +143,12 @@ static int time_handler(sd_event_source *s, uint64_t usec, void *userdata) {
         return 2;
 }
 
-static bool got_quit = false;
+static bool got_exit = false;
 
-static int quit_handler(sd_event_source *s, void *userdata) {
+static int exit_handler(sd_event_source *s, void *userdata) {
         log_info("got quit handler on %c", PTR_TO_INT(userdata));
 
-        got_quit = true;
+        got_exit = true;
 
         return 3;
 }
@@ -183,7 +183,7 @@ int main(int argc, char *argv[]) {
         assert_se(sd_event_add_io(e, a[0], EPOLLIN, io_handler, INT_TO_PTR('a'), &x) >= 0);
         assert_se(sd_event_add_io(e, b[0], EPOLLIN, io_handler, INT_TO_PTR('b'), &y) >= 0);
         assert_se(sd_event_add_monotonic(e, 0, 0, time_handler, INT_TO_PTR('c'), &z) >= 0);
-        assert_se(sd_event_add_quit(e, quit_handler, INT_TO_PTR('g'), &q) >= 0);
+        assert_se(sd_event_add_exit(e, exit_handler, INT_TO_PTR('g'), &q) >= 0);
 
         assert_se(sd_event_source_set_priority(x, 99) >= 0);
         assert_se(sd_event_source_set_enabled(y, SD_EVENT_ONESHOT) >= 0);

diff --git a/src/libsystemd-rtnl/rtnl-internal.h b/src/libsystemd-rtnl/rtnl-internal.h
index dabf12d..a1050a0 100644 (file)
--- a/src/libsystemd-rtnl/rtnl-internal.h
+++ b/src/libsystemd-rtnl/rtnl-internal.h
@@ -74,7 +74,7 @@ struct sd_rtnl {
 
         sd_event_source *io_event_source;
         sd_event_source *time_event_source;
-        sd_event_source *quit_event_source;
+        sd_event_source *exit_event_source;
         sd_event *event;
 };
 

diff --git a/src/libsystemd-rtnl/sd-rtnl.c b/src/libsystemd-rtnl/sd-rtnl.c
index 57d0b76..98d0f89 100644 (file)
--- a/src/libsystemd-rtnl/sd-rtnl.c
+++ b/src/libsystemd-rtnl/sd-rtnl.c
@@ -743,7 +743,7 @@ static int prepare_callback(sd_event_source *s, void *userdata) {
         return 1;
 }
 
-static int quit_callback(sd_event_source *event, void *userdata) {
+static int exit_callback(sd_event_source *event, void *userdata) {
         sd_rtnl *rtnl = userdata;
 
         assert(event);
@@ -790,7 +790,7 @@ int sd_rtnl_attach_event(sd_rtnl *rtnl, sd_event *event, int priority) {
         if (r < 0)
                 goto fail;
 
-        r = sd_event_add_quit(rtnl->event, quit_callback, rtnl, &rtnl->quit_event_source);
+        r = sd_event_add_exit(rtnl->event, exit_callback, rtnl, &rtnl->exit_event_source);
         if (r < 0)
                 goto fail;
 
@@ -811,8 +811,8 @@ int sd_rtnl_detach_event(sd_rtnl *rtnl) {
         if (rtnl->time_event_source)
                 rtnl->time_event_source = sd_event_source_unref(rtnl->time_event_source);
 
-        if (rtnl->quit_event_source)
-                rtnl->quit_event_source = sd_event_source_unref(rtnl->quit_event_source);
+        if (rtnl->exit_event_source)
+                rtnl->exit_event_source = sd_event_source_unref(rtnl->exit_event_source);
 
         if (rtnl->event)
                 rtnl->event = sd_event_unref(rtnl->event);

diff --git a/src/locale/localed.c b/src/locale/localed.c
index 65318b6..2c90180 100644 (file)
--- a/src/locale/localed.c
+++ b/src/locale/localed.c
@@ -1155,8 +1155,6 @@ int main(int argc, char *argv[]) {
                 goto finish;
         }
 
-        r = 0;
-
 finish:
         context_free(&context, bus);
 

diff --git a/src/systemd/sd-event.h b/src/systemd/sd-event.h
index 3551077..c7c4506 100644 (file)
--- a/src/systemd/sd-event.h
+++ b/src/systemd/sd-event.h
@@ -53,7 +53,7 @@ enum {
 enum {
         SD_EVENT_PASSIVE,
         SD_EVENT_RUNNING,
-        SD_EVENT_QUITTING,
+        SD_EVENT_EXITING,
         SD_EVENT_FINISHED
 };
 
@@ -82,15 +82,15 @@ int sd_event_add_realtime(sd_event *e, uint64_t usec, uint64_t accuracy, sd_even
 int sd_event_add_signal(sd_event *e, int sig, sd_event_signal_handler_t callback, void *userdata, sd_event_source **s);
 int sd_event_add_child(sd_event *e, pid_t pid, int options, sd_event_child_handler_t callback, void *userdata, sd_event_source **s);
 int sd_event_add_defer(sd_event *e, sd_event_handler_t callback, void *userdata, sd_event_source **s);
-int sd_event_add_quit(sd_event *e, sd_event_handler_t callback, void *userdata, sd_event_source **s);
+int sd_event_add_exit(sd_event *e, sd_event_handler_t callback, void *userdata, sd_event_source **s);
 
 int sd_event_run(sd_event *e, uint64_t timeout);
 int sd_event_loop(sd_event *e);
+int sd_event_exit(sd_event *e, int code);
 
 int sd_event_get_state(sd_event *e);
 int sd_event_get_tid(sd_event *e, pid_t *tid);
-int sd_event_get_quit(sd_event *e);
-int sd_event_request_quit(sd_event *e);
+int sd_event_get_exit_code(sd_event *e, int *code);
 int sd_event_get_now_realtime(sd_event *e, uint64_t *usec);
 int sd_event_get_now_monotonic(sd_event *e, uint64_t *usec);
 int sd_event_set_watchdog(sd_event *e, int b);

diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c
index 37173a2..8ac933c 100644 (file)
--- a/src/timedate/timedated.c
+++ b/src/timedate/timedated.c
@@ -863,8 +863,6 @@ int main(int argc, char *argv[]) {
                 goto finish;
         }
 
-        r = 0;
-
 finish:
         context_free(&context, bus);