From 65d73cf042ba7de11d254f5c4714f467db64b7c3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Aug 2015 16:46:16 +0300 Subject: machined,logind: don't generate errors on signal match functions If we get a weird signal, then we should log about it, but not return an error, since sd-bus will not call us again then anymore, but for these signals we match here we actually do want to be called on the next invocation. --- src/login/logind-dbus.c | 12 +++++++----- src/machine/machined-dbus.c | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 1647bb293a..cc1b868f51 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -2511,7 +2511,7 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err r = sd_bus_message_read(message, "uoss", &id, &path, &unit, &result); if (r < 0) { bus_log_parse_error(r); - return r; + return 0; } if (m->action_job && streq(m->action_job, path)) { @@ -2579,7 +2579,7 @@ int match_unit_removed(sd_bus_message *message, void *userdata, sd_bus_error *er r = sd_bus_message_read(message, "so", &unit, &path); if (r < 0) { bus_log_parse_error(r); - return r; + return 0; } session = hashmap_get(m->session_units, unit); @@ -2611,8 +2611,10 @@ int match_properties_changed(sd_bus_message *message, void *userdata, sd_bus_err r = unit_name_from_dbus_path(path, &unit); if (r == -EINVAL) /* not a unit */ return 0; - if (r < 0) - return r; + if (r < 0) { + log_oom(); + return 0; + } session = hashmap_get(m->session_units, unit); if (session) @@ -2637,7 +2639,7 @@ int match_reloading(sd_bus_message *message, void *userdata, sd_bus_error *error r = sd_bus_message_read(message, "b", &b); if (r < 0) { bus_log_parse_error(r); - return r; + return 0; } if (b) diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c index 3637815fc9..b1f5aebe0c 100644 --- a/src/machine/machined-dbus.c +++ b/src/machine/machined-dbus.c @@ -1116,7 +1116,7 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err r = sd_bus_message_read(message, "uoss", &id, &path, &unit, &result); if (r < 0) { bus_log_parse_error(r); - return r; + return 0; } machine = hashmap_get(m->machine_units, unit); @@ -1242,7 +1242,7 @@ int match_reloading(sd_bus_message *message, void *userdata, sd_bus_error *error r = sd_bus_message_read(message, "b", &b); if (r < 0) { bus_log_parse_error(r); - return r; + return 0; } if (b) return 0; -- cgit v1.2.3-54-g00ecf From 491ac9f2c4aeda8c40edde35112404b737e38b60 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Aug 2015 16:48:17 +0300 Subject: logind,machined: various smaller cleanups Use mfree() where we can. Drop unnecessary {}. Drop unnecessary variable declarations. Cast syscall invocations where explicitly don't care for the return value to (void). Reword a comment. --- src/login/logind-dbus.c | 24 ++++++++---------------- src/login/logind-session.c | 5 ++--- src/login/logind.c | 4 ++-- src/machine/machine.c | 4 ++-- src/machine/machined-dbus.c | 3 +-- 5 files changed, 15 insertions(+), 25 deletions(-) (limited to 'src') diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index cc1b868f51..992a9f5b4a 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -2520,8 +2520,7 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err /* Tell people that they now may take a lock again */ send_prepare_for(m, m->action_what, false); - free(m->action_job); - m->action_job = NULL; + m->action_job = mfree(m->action_job); m->action_unit = NULL; m->action_what = 0; return 0; @@ -2530,10 +2529,8 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err session = hashmap_get(m->session_units, unit); if (session) { - if (streq_ptr(path, session->scope_job)) { - free(session->scope_job); - session->scope_job = NULL; - } + if (streq_ptr(path, session->scope_job)) + session->scope_job = mfree(session->scope_job); session_jobs_reply(session, unit, result); @@ -2545,19 +2542,14 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err user = hashmap_get(m->user_units, unit); if (user) { - if (streq_ptr(path, user->service_job)) { - free(user->service_job); - user->service_job = NULL; - } + if (streq_ptr(path, user->service_job)) + user->service_job = mfree(user->service_job); - if (streq_ptr(path, user->slice_job)) { - free(user->slice_job); - user->slice_job = NULL; - } + if (streq_ptr(path, user->slice_job)) + user->slice_job = mfree(user->slice_job); - LIST_FOREACH(sessions_by_user, session, user->sessions) { + LIST_FOREACH(sessions_by_user, session, user->sessions) session_jobs_reply(session, unit, result); - } user_save(user); user_add_to_gc_queue(user); diff --git a/src/login/logind-session.c b/src/login/logind-session.c index fc92f7f73b..e75c7c042e 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -656,7 +656,6 @@ int session_stop(Session *s, bool force) { } int session_finalize(Session *s) { - int r = 0; SessionDevice *sd; assert(s); @@ -682,7 +681,7 @@ int session_finalize(Session *s) { while ((sd = hashmap_first(s->devices))) session_device_free(sd); - unlink(s->state_file); + (void) unlink(s->state_file); session_add_to_gc_queue(s); user_add_to_gc_queue(s->user); @@ -702,7 +701,7 @@ int session_finalize(Session *s) { user_save(s->user); user_send_changed(s->user, "Sessions", "Display", NULL); - return r; + return 0; } static int release_timeout_callback(sd_event_source *es, uint64_t usec, void *userdata) { diff --git a/src/login/logind.c b/src/login/logind.c index 49a2811842..cf71c0ec5a 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -907,8 +907,8 @@ static void manager_gc(Manager *m, bool drop_not_started) { session_get_state(session) != SESSION_CLOSING) session_stop(session, false); - /* Normally, this should make the session busy again, - * if it doesn't then let's get rid of it + /* Normally, this should make the session referenced + * again, if it doesn't then let's get rid of it * immediately */ if (!session_check_gc(session, drop_not_started)) { session_finalize(session); diff --git a/src/machine/machine.c b/src/machine/machine.c index ab26803683..4fd56a11ae 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -231,11 +231,11 @@ static void machine_unlink(Machine *m) { char *sl; sl = strjoina("/run/systemd/machines/unit:", m->unit); - unlink(sl); + (void) unlink(sl); } if (m->state_file) - unlink(m->state_file); + (void) unlink(m->state_file); } int machine_load(Machine *m) { diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c index b1f5aebe0c..49f41c62d7 100644 --- a/src/machine/machined-dbus.c +++ b/src/machine/machined-dbus.c @@ -1124,8 +1124,7 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err return 0; if (streq_ptr(path, machine->scope_job)) { - free(machine->scope_job); - machine->scope_job = NULL; + machine->scope_job = mfree(machine->scope_job); if (machine->started) { if (streq(result, "done")) -- cgit v1.2.3-54-g00ecf From e5a840c93ac8a175a61fc5944d260127ff2247cc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Aug 2015 16:50:02 +0300 Subject: machine: drop state variable from Machine object We never made use of it, let's get rid of it. --- src/machine/machine.h | 1 - 1 file changed, 1 deletion(-) (limited to 'src') diff --git a/src/machine/machine.h b/src/machine/machine.h index bbe5217f65..b1cd26662f 100644 --- a/src/machine/machine.h +++ b/src/machine/machine.h @@ -67,7 +67,6 @@ struct Machine { char *name; sd_id128_t id; - MachineState state; MachineClass class; char *state_file; -- cgit v1.2.3-54-g00ecf From 49f3fffd94591bdf2bd6c2233a9300daeab79566 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Aug 2015 16:50:54 +0300 Subject: machined: rework state tracking logic for machines This splits up the stopping logic for machines into two steps: first on machine_stop() we begin with the shutdown of a machine by queuing the stop method call for it. Then, in machine_finalize() we actually remove the rest of its runtime context. This mimics closely how sessions are handled in logind. This also reworks the GC logic to strictly check the current state of the machine unit, rather than shortcutting a few cases, like for example assuming that UnitRemoved really means a machine is gone (which it isn't since Reloading might trigger it, see #376). Fixes #376. --- src/machine/machine.c | 32 +++++++++++++++++++++----------- src/machine/machine.h | 2 ++ src/machine/machined-dbus.c | 39 ++++----------------------------------- src/machine/machined.c | 10 +++++++++- 4 files changed, 36 insertions(+), 47 deletions(-) (limited to 'src') diff --git a/src/machine/machine.c b/src/machine/machine.c index 4fd56a11ae..f045159d41 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -419,7 +419,19 @@ static int machine_stop_scope(Machine *m) { } int machine_stop(Machine *m) { - int r = 0, k; + int r; + assert(m); + + r = machine_stop_scope(m); + + m->stopping = true; + + machine_save(m); + + return r; +} + +int machine_finalize(Machine *m) { assert(m); if (m->started) @@ -430,20 +442,15 @@ int machine_stop(Machine *m) { LOG_MESSAGE("Machine %s terminated.", m->name), NULL); - /* Kill cgroup */ - k = machine_stop_scope(m); - if (k < 0) - r = k; - machine_unlink(m); machine_add_to_gc_queue(m); - if (m->started) + if (m->started) { machine_send_signal(m, false); + m->started = false; + } - m->started = false; - - return r; + return 0; } bool machine_check_gc(Machine *m, bool drop_not_started) { @@ -474,8 +481,11 @@ void machine_add_to_gc_queue(Machine *m) { MachineState machine_get_state(Machine *s) { assert(s); + if (s->stopping) + return MACHINE_CLOSING; + if (s->scope_job) - return s->started ? MACHINE_OPENING : MACHINE_CLOSING; + return MACHINE_OPENING; return MACHINE_RUNNING; } diff --git a/src/machine/machine.h b/src/machine/machine.h index b1cd26662f..0132b65a97 100644 --- a/src/machine/machine.h +++ b/src/machine/machine.h @@ -82,6 +82,7 @@ struct Machine { bool in_gc_queue:1; bool started:1; + bool stopping:1; sd_bus_message *create_message; @@ -100,6 +101,7 @@ bool machine_check_gc(Machine *m, bool drop_not_started); void machine_add_to_gc_queue(Machine *m); int machine_start(Machine *m, sd_bus_message *properties, sd_bus_error *error); int machine_stop(Machine *m); +int machine_finalize(Machine *m); int machine_save(Machine *m); int machine_load(Machine *m); int machine_kill(Machine *m, KillWho who, int signo); diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c index 49f41c62d7..08a7f58ef5 100644 --- a/src/machine/machined-dbus.c +++ b/src/machine/machined-dbus.c @@ -1136,8 +1136,9 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err machine_send_create_reply(machine, &e); } - } else - machine_save(machine); + } + + machine_save(machine); } machine_add_to_gc_queue(machine); @@ -1146,7 +1147,7 @@ int match_job_removed(sd_bus_message *message, void *userdata, sd_bus_error *err int match_properties_changed(sd_bus_message *message, void *userdata, sd_bus_error *error) { _cleanup_free_ char *unit = NULL; - const char *path, *interface; + const char *path; Manager *m = userdata; Machine *machine; int r; @@ -1170,36 +1171,6 @@ int match_properties_changed(sd_bus_message *message, void *userdata, sd_bus_err if (!machine) return 0; - r = sd_bus_message_read(message, "s", &interface); - if (r < 0) { - bus_log_parse_error(r); - return 0; - } - - if (streq(interface, "org.freedesktop.systemd1.Unit")) { - struct properties { - char *active_state; - char *sub_state; - } properties = {}; - - const struct bus_properties_map map[] = { - { "ActiveState", "s", NULL, offsetof(struct properties, active_state) }, - { "SubState", "s", NULL, offsetof(struct properties, sub_state) }, - {} - }; - - r = bus_message_map_properties_changed(message, map, &properties); - if (r < 0) - bus_log_parse_error(r); - else if (streq_ptr(properties.active_state, "inactive") || - streq_ptr(properties.active_state, "failed") || - streq_ptr(properties.sub_state, "auto-restart")) - machine_release_unit(machine); - - free(properties.active_state); - free(properties.sub_state); - } - machine_add_to_gc_queue(machine); return 0; } @@ -1223,9 +1194,7 @@ int match_unit_removed(sd_bus_message *message, void *userdata, sd_bus_error *er if (!machine) return 0; - machine_release_unit(machine); machine_add_to_gc_queue(machine); - return 0; } diff --git a/src/machine/machined.c b/src/machine/machined.c index 9bfe2add54..1eeeaf17a5 100644 --- a/src/machine/machined.c +++ b/src/machine/machined.c @@ -247,8 +247,16 @@ void manager_gc(Manager *m, bool drop_not_started) { LIST_REMOVE(gc_queue, m->machine_gc_queue, machine); machine->in_gc_queue = false; - if (!machine_check_gc(machine, drop_not_started)) { + /* First, if we are not closing yet, initiate stopping */ + if (!machine_check_gc(machine, drop_not_started) && + machine_get_state(machine) != MACHINE_CLOSING) machine_stop(machine); + + /* Now, the stop stop probably made this referenced + * again, but if it didn't, then it's time to let it + * go entirely. */ + if (!machine_check_gc(machine, drop_not_started)) { + machine_finalize(machine); machine_free(machine); } } -- cgit v1.2.3-54-g00ecf