From bff8f950ab15ef3beb9c363c0303fc6ed5c13c13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 13 Feb 2018 08:57:31 +0100 Subject: [PATCH 01/11] test-strv: add a simple test for strv_free_free() --- src/test/test-strv.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/test/test-strv.c b/src/test/test-strv.c index 76cd551eeb..c6698f4396 100644 --- a/src/test/test-strv.c +++ b/src/test/test-strv.c @@ -676,6 +676,17 @@ static void test_strv_make_nulstr(void) { test_strv_make_nulstr_one(STRV_MAKE("foo", "bar", "quuux")); } +static void test_strv_free_free(void) { + char ***t; + + assert_se(t = new(char**, 3)); + assert_se(t[0] = strv_new("a", "b", NULL)); + assert_se(t[1] = strv_new("c", "d", "e", NULL)); + t[2] = NULL; + + t = strv_free_free(t); +} + static void test_foreach_string(void) { const char * const t[] = { "foo", @@ -763,6 +774,7 @@ int main(int argc, char *argv[]) { test_strv_skip(); test_strv_extend_n(); test_strv_make_nulstr(); + test_strv_free_free(); test_foreach_string(); test_strv_fnmatch(); From 94e91c8319fa207bcfed29a695e95a699e84235e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 13 Feb 2018 10:03:09 +0100 Subject: [PATCH 02/11] analyze: fix typo in error message --- src/analyze/analyze.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 2d65fdbdb8..1aa8c00512 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -1295,7 +1295,7 @@ static int dump(int argc, char *argv[], void *userdata) { &reply, NULL); if (r < 0) - return log_error_errno(r, "Failed issue method call: %s", bus_error_message(&error, r)); + return log_error_errno(r, "Failed to issue method call: %s", bus_error_message(&error, r)); r = sd_bus_message_read(reply, "s", &text); if (r < 0) From f2f725e5cc950e84ebfd09bd64bc01c0ebdb6734 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 13 Feb 2018 10:50:13 +0100 Subject: [PATCH 03/11] pid1: rename unit_check_gc to unit_may_gc "check" is unclear: what is true, what is false? Let's rename to "can_gc" and revert the return value ("positive" values are easier to grok). v2: - rename from unit_can_gc to unit_may_gc --- src/core/automount.c | 13 ++++++++----- src/core/manager.c | 2 +- src/core/mount.c | 9 ++++++--- src/core/service.c | 10 +++++----- src/core/socket.c | 6 +++--- src/core/socket.h | 4 ++-- src/core/swap.c | 9 ++++++--- src/core/unit.c | 36 ++++++++++++++++++------------------ src/core/unit.h | 9 ++++----- 9 files changed, 53 insertions(+), 45 deletions(-) diff --git a/src/core/automount.c b/src/core/automount.c index c191336c0e..01a6ff806e 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -965,13 +965,16 @@ static const char *automount_sub_state_to_string(Unit *u) { return automount_state_to_string(AUTOMOUNT(u)->state); } -static bool automount_check_gc(Unit *u) { +static bool automount_may_gc(Unit *u) { + Unit *t; + assert(u); - if (!UNIT_TRIGGER(u)) - return false; + t = UNIT_TRIGGER(u); + if (!t) + return true; - return UNIT_VTABLE(UNIT_TRIGGER(u))->check_gc(UNIT_TRIGGER(u)); + return UNIT_VTABLE(t)->may_gc(t); } static int automount_dispatch_io(sd_event_source *s, int fd, uint32_t events, void *userdata) { @@ -1124,7 +1127,7 @@ const UnitVTable automount_vtable = { .active_state = automount_active_state, .sub_state_to_string = automount_sub_state_to_string, - .check_gc = automount_check_gc, + .may_gc = automount_may_gc, .trigger_notify = automount_trigger_notify, diff --git a/src/core/manager.c b/src/core/manager.c index 5021e00b87..c36a954e61 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1057,7 +1057,7 @@ static void unit_gc_sweep(Unit *u, unsigned gc_marker) { if (u->in_cleanup_queue) goto bad; - if (unit_check_gc(u)) + if (!unit_may_gc(u)) goto good; u->gc_marker = gc_marker + GC_OFFSET_IN_PATH; diff --git a/src/core/mount.c b/src/core/mount.c index 914458f8e6..fa33a71d14 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1234,12 +1234,15 @@ _pure_ static const char *mount_sub_state_to_string(Unit *u) { return mount_state_to_string(MOUNT(u)->state); } -_pure_ static bool mount_check_gc(Unit *u) { +_pure_ static bool mount_may_gc(Unit *u) { Mount *m = MOUNT(u); assert(m); - return m->from_proc_self_mountinfo; + if (m->from_proc_self_mountinfo) + return false; + + return true; } static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) { @@ -1995,7 +1998,7 @@ const UnitVTable mount_vtable = { .active_state = mount_active_state, .sub_state_to_string = mount_sub_state_to_string, - .check_gc = mount_check_gc, + .may_gc = mount_may_gc, .sigchld_event = mount_sigchld_event, diff --git a/src/core/service.c b/src/core/service.c index 37904874b5..b6106593ee 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2833,20 +2833,20 @@ static const char *service_sub_state_to_string(Unit *u) { return service_state_to_string(SERVICE(u)->state); } -static bool service_check_gc(Unit *u) { +static bool service_may_gc(Unit *u) { Service *s = SERVICE(u); assert(s); /* Never clean up services that still have a process around, even if the service is formally dead. Note that - * unit_check_gc() already checked our cgroup for us, we just check our two additional PIDs, too, in case they + * unit_may_gc() already checked our cgroup for us, we just check our two additional PIDs, too, in case they * have moved outside of the cgroup. */ if (main_pid_good(s) > 0 || control_pid_good(s) > 0) - return true; + return false; - return false; + return true; } static int service_retry_pid_file(Service *s) { @@ -3934,7 +3934,7 @@ const UnitVTable service_vtable = { .will_restart = service_will_restart, - .check_gc = service_check_gc, + .may_gc = service_may_gc, .sigchld_event = service_sigchld_event, diff --git a/src/core/socket.c b/src/core/socket.c index 703f9f760f..f3e034d2af 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2819,12 +2819,12 @@ SocketType socket_port_type_from_string(const char *s) { return _SOCKET_TYPE_INVALID; } -_pure_ static bool socket_check_gc(Unit *u) { +_pure_ static bool socket_may_gc(Unit *u) { Socket *s = SOCKET(u); assert(u); - return s->n_connections > 0; + return s->n_connections == 0; } static int socket_accept_do(Socket *s, int fd) { @@ -3324,7 +3324,7 @@ const UnitVTable socket_vtable = { .active_state = socket_active_state, .sub_state_to_string = socket_sub_state_to_string, - .check_gc = socket_check_gc, + .may_gc = socket_may_gc, .sigchld_event = socket_sigchld_event, diff --git a/src/core/socket.h b/src/core/socket.h index 9c528fb39c..84ec9cff08 100644 --- a/src/core/socket.h +++ b/src/core/socket.h @@ -104,8 +104,8 @@ struct Socket { DynamicCreds dynamic_creds; /* For Accept=no sockets refers to the one service we'll - activate. For Accept=yes sockets is either NULL, or filled - when the next service we spawn. */ + * activate. For Accept=yes sockets is either NULL, or filled + * to refer to the next service we spawn. */ UnitRef service; SocketState state, deserialized_state; diff --git a/src/core/swap.c b/src/core/swap.c index fffd8d4627..0af6538338 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -979,12 +979,15 @@ _pure_ static const char *swap_sub_state_to_string(Unit *u) { return swap_state_to_string(SWAP(u)->state); } -_pure_ static bool swap_check_gc(Unit *u) { +_pure_ static bool swap_may_gc(Unit *u) { Swap *s = SWAP(u); assert(s); - return s->from_proc_swaps; + if (s->from_proc_swaps) + return false; + + return true; } static void swap_sigchld_event(Unit *u, pid_t pid, int code, int status) { @@ -1506,7 +1509,7 @@ const UnitVTable swap_vtable = { .active_state = swap_active_state, .sub_state_to_string = swap_sub_state_to_string, - .check_gc = swap_check_gc, + .may_gc = swap_may_gc, .sigchld_event = swap_sigchld_event, diff --git a/src/core/unit.c b/src/core/unit.c index 9a57bcfb4b..fd2cc6a667 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -336,20 +336,21 @@ int unit_set_description(Unit *u, const char *description) { return 0; } -bool unit_check_gc(Unit *u) { +bool unit_may_gc(Unit *u) { UnitActiveState state; int r; assert(u); - /* Checks whether the unit is ready to be unloaded for garbage collection. Returns true, when the unit shall - * stay around, false if there's no reason to keep it loaded. */ + /* Checks whether the unit is ready to be unloaded for garbage collection. + * Returns true when the unit may be collected, and false if there's some + * reason to keep it loaded. */ if (u->job) - return true; + return false; if (u->nop_job) - return true; + return false; state = unit_active_state(u); @@ -359,26 +360,26 @@ bool unit_check_gc(Unit *u) { UNIT_VTABLE(u)->release_resources(u); if (u->perpetual) - return true; + return false; if (u->refs) - return true; + return false; if (sd_bus_track_count(u->bus_track) > 0) - return true; + return false; /* But we keep the unit object around for longer when it is referenced or configured to not be gc'ed */ switch (u->collect_mode) { case COLLECT_INACTIVE: if (state != UNIT_INACTIVE) - return true; + return false; break; case COLLECT_INACTIVE_OR_FAILED: if (!IN_SET(state, UNIT_INACTIVE, UNIT_FAILED)) - return true; + return false; break; @@ -394,14 +395,13 @@ bool unit_check_gc(Unit *u) { if (r < 0) log_unit_debug_errno(u, r, "Failed to determine whether cgroup %s is empty: %m", u->cgroup_path); if (r <= 0) - return true; + return false; } - if (UNIT_VTABLE(u)->check_gc) - if (UNIT_VTABLE(u)->check_gc(u)) - return true; + if (UNIT_VTABLE(u)->may_gc && !UNIT_VTABLE(u)->may_gc(u)) + return false; - return false; + return true; } void unit_add_to_load_queue(Unit *u) { @@ -431,7 +431,7 @@ void unit_add_to_gc_queue(Unit *u) { if (u->in_gc_queue || u->in_cleanup_queue) return; - if (unit_check_gc(u)) + if (!unit_may_gc(u)) return; LIST_PREPEND(gc_queue, u->manager->gc_unit_queue, u); @@ -1119,7 +1119,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { "%s\tActive Enter Timestamp: %s\n" "%s\tActive Exit Timestamp: %s\n" "%s\tInactive Enter Timestamp: %s\n" - "%s\tGC Check Good: %s\n" + "%s\tMay GC: %s\n" "%s\tNeed Daemon Reload: %s\n" "%s\tTransient: %s\n" "%s\tPerpetual: %s\n" @@ -1137,7 +1137,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { prefix, strna(format_timestamp(timestamp2, sizeof(timestamp2), u->active_enter_timestamp.realtime)), prefix, strna(format_timestamp(timestamp3, sizeof(timestamp3), u->active_exit_timestamp.realtime)), prefix, strna(format_timestamp(timestamp4, sizeof(timestamp4), u->inactive_enter_timestamp.realtime)), - prefix, yes_no(unit_check_gc(u)), + prefix, yes_no(unit_may_gc(u)), prefix, yes_no(unit_need_daemon_reload(u)), prefix, yes_no(u->transient), prefix, yes_no(u->perpetual), diff --git a/src/core/unit.h b/src/core/unit.h index 3210583050..8e3d9bf323 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -490,10 +490,9 @@ struct UnitVTable { /* Additionally to UnitActiveState determine whether unit is to be restarted. */ bool (*will_restart)(Unit *u); - /* Return true when there is reason to keep this entry around - * even nothing references it and it isn't active in any - * way */ - bool (*check_gc)(Unit *u); + /* Return false when there is a reason to prevent this unit from being gc'ed + * even though nothing references it and it isn't active in any way. */ + bool (*may_gc)(Unit *u); /* When the unit is not running and no job for it queued we shall release its runtime resources */ void (*release_resources)(Unit *u); @@ -623,7 +622,7 @@ int unit_add_exec_dependencies(Unit *u, ExecContext *c); int unit_choose_id(Unit *u, const char *name); int unit_set_description(Unit *u, const char *description); -bool unit_check_gc(Unit *u); +bool unit_may_gc(Unit *u); void unit_add_to_load_queue(Unit *u); void unit_add_to_dbus_queue(Unit *u); From 7f7d01ed5804afef220ebdb29f22d8177d0d3a5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 13 Feb 2018 13:12:43 +0100 Subject: [PATCH 04/11] pid1: include the source unit in UnitRef No functional change. The source unit manages the reference. It allocates the UnitRef structure and registers it in the target unit, and then the reference must be destroyed before the source unit is destroyed. Thus, is should be OK to include the pointer to the source unit, it should be live as long as the reference exists. v2: - rename refs to refs_by_target --- src/core/load-fragment.c | 2 +- src/core/service.c | 4 ++-- src/core/slice.c | 2 +- src/core/socket.c | 4 ++-- src/core/unit.c | 36 ++++++++++++++++++------------------ src/core/unit.h | 12 ++++++------ 6 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 00408c4b84..f7e82199f5 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -1860,7 +1860,7 @@ int config_parse_socket_service( return -ENOEXEC; } - unit_ref_set(&s->service, x); + unit_ref_set(&s->service, UNIT(s), x); return 0; } diff --git a/src/core/service.c b/src/core/service.c index b6106593ee..e5113838bf 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2684,7 +2684,7 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, if (r < 0) log_unit_debug_errno(u, r, "Failed to load accept-socket unit: %s", value); else { - unit_ref_set(&s->accept_socket, socket); + unit_ref_set(&s->accept_socket, u, socket); SOCKET(socket)->n_connections++; } @@ -3770,7 +3770,7 @@ int service_set_socket_fd(Service *s, int fd, Socket *sock, bool selinux_context s->socket_fd = fd; s->socket_fd_selinux_context_net = selinux_context_net; - unit_ref_set(&s->accept_socket, UNIT(sock)); + unit_ref_set(&s->accept_socket, UNIT(s), UNIT(sock)); return 0; } diff --git a/src/core/slice.c b/src/core/slice.c index ef2177279a..2228669e00 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -76,7 +76,7 @@ static int slice_add_parent_slice(Slice *s) { if (r < 0) return r; - unit_ref_set(&u->slice, parent); + unit_ref_set(&u->slice, u, parent); return 0; } diff --git a/src/core/socket.c b/src/core/socket.c index f3e034d2af..cb42a3eb50 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -250,7 +250,7 @@ int socket_instantiate_service(Socket *s) { if (r < 0) return r; - unit_ref_set(&s->service, u); + unit_ref_set(&s->service, UNIT(s), u); return unit_add_two_dependencies(UNIT(s), UNIT_BEFORE, UNIT_TRIGGERS, u, false, UNIT_DEPENDENCY_IMPLICIT); } @@ -377,7 +377,7 @@ static int socket_add_extras(Socket *s) { if (r < 0) return r; - unit_ref_set(&s->service, x); + unit_ref_set(&s->service, u, x); } r = unit_add_two_dependencies(u, UNIT_BEFORE, UNIT_TRIGGERS, UNIT_DEREF(s->service), true, UNIT_DEPENDENCY_IMPLICIT); diff --git a/src/core/unit.c b/src/core/unit.c index fd2cc6a667..761ce40f2a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -362,7 +362,7 @@ bool unit_may_gc(Unit *u) { if (u->perpetual) return false; - if (u->refs) + if (u->refs_by_target) return false; if (sd_bus_track_count(u->bus_track) > 0) @@ -662,9 +662,8 @@ void unit_free(Unit *u) { free(u->reboot_arg); unit_ref_unset(&u->slice); - - while (u->refs) - unit_ref_unset(u->refs); + while (u->refs_by_target) + unit_ref_unset(u->refs_by_target); safe_close(u->ip_accounting_ingress_map_fd); safe_close(u->ip_accounting_egress_map_fd); @@ -896,8 +895,8 @@ int unit_merge(Unit *u, Unit *other) { return r; /* Redirect all references */ - while (other->refs) - unit_ref_set(other->refs, u); + while (other->refs_by_target) + unit_ref_set(other->refs_by_target, other->refs_by_target->source, u); /* Merge dependencies */ for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) @@ -2976,8 +2975,7 @@ int unit_set_slice(Unit *u, Unit *slice) { if (UNIT_ISSET(u->slice) && u->cgroup_realized) return -EBUSY; - unit_ref_unset(&u->slice); - unit_ref_set(&u->slice, slice); + unit_ref_set(&u->slice, u, slice); return 1; } @@ -3986,30 +3984,32 @@ int unit_get_unit_file_preset(Unit *u) { return u->unit_file_preset; } -Unit* unit_ref_set(UnitRef *ref, Unit *u) { +Unit* unit_ref_set(UnitRef *ref, Unit *source, Unit *target) { assert(ref); - assert(u); + assert(source); + assert(target); - if (ref->unit) + if (ref->target) unit_ref_unset(ref); - ref->unit = u; - LIST_PREPEND(refs, u->refs, ref); - return u; + ref->source = source; + ref->target = target; + LIST_PREPEND(refs_by_target, target->refs_by_target, ref); + return target; } void unit_ref_unset(UnitRef *ref) { assert(ref); - if (!ref->unit) + if (!ref->target) return; /* We are about to drop a reference to the unit, make sure the garbage collection has a look at it as it might * be unreferenced now. */ - unit_add_to_gc_queue(ref->unit); + unit_add_to_gc_queue(ref->target); - LIST_REMOVE(refs, ref->unit->refs, ref); - ref->unit = NULL; + LIST_REMOVE(refs_by_target, ref->target->refs_by_target, ref); + ref->source = ref->target = NULL; } static int user_from_unit_name(Unit *u, char **ret) { diff --git a/src/core/unit.h b/src/core/unit.h index 8e3d9bf323..286115cd87 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -123,8 +123,8 @@ struct UnitRef { * that we can merge two units if necessary and correct all * references to them */ - Unit* unit; - LIST_FIELDS(UnitRef, refs); + Unit *source, *target; + LIST_FIELDS(UnitRef, refs_by_target); }; typedef enum UnitCGroupBPFState { @@ -187,7 +187,7 @@ struct Unit { char *job_timeout_reboot_arg; /* References to this */ - LIST_HEAD(UnitRef, refs); + LIST_HEAD(UnitRef, refs_by_target); /* Conditions to check */ LIST_HEAD(Condition, conditions); @@ -724,11 +724,11 @@ void unit_trigger_notify(Unit *u); UnitFileState unit_get_unit_file_state(Unit *u); int unit_get_unit_file_preset(Unit *u); -Unit* unit_ref_set(UnitRef *ref, Unit *u); +Unit* unit_ref_set(UnitRef *ref, Unit *source, Unit *target); void unit_ref_unset(UnitRef *ref); -#define UNIT_DEREF(ref) ((ref).unit) -#define UNIT_ISSET(ref) (!!(ref).unit) +#define UNIT_DEREF(ref) ((ref).target) +#define UNIT_ISSET(ref) (!!(ref).target) int unit_patch_contexts(Unit *u); From 2641f02e23ac7d5385db7f932aff221a063f245e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 13 Feb 2018 14:37:11 +0100 Subject: [PATCH 05/11] pid1: fix collection of cycles of units which reference one another A .socket will reference a .service unit, by registering a UnitRef with the .service unit. If this .service unit has the .socket unit listed in Wants or Sockets or such, a cycle will be created. We would not free this cycle properly, because we treated any unit with non-empty refs as uncollectable. To solve this issue, treats refs with UnitRef in u->refs_by_target similarly to the refs in u->dependencies, and check if the "other" unit is known to be needed. If it is not needed, do not treat the reference from it as preventing the unit we are looking at from being freed. --- src/core/manager.c | 14 ++++++++++++++ src/core/unit.c | 9 +++++---- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index c36a954e61..f6c88ba114 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1074,6 +1074,20 @@ static void unit_gc_sweep(Unit *u, unsigned gc_marker) { is_bad = false; } + if (u->refs_by_target) { + const UnitRef *ref; + + LIST_FOREACH(refs_by_target, ref, u->refs_by_target) { + unit_gc_sweep(ref->source, gc_marker); + + if (ref->source->gc_marker == gc_marker + GC_OFFSET_GOOD) + goto good; + + if (ref->source->gc_marker != gc_marker + GC_OFFSET_BAD) + is_bad = false; + } + } + if (is_bad) goto bad; diff --git a/src/core/unit.c b/src/core/unit.c index 761ce40f2a..0330ae51fd 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -344,7 +344,11 @@ bool unit_may_gc(Unit *u) { /* Checks whether the unit is ready to be unloaded for garbage collection. * Returns true when the unit may be collected, and false if there's some - * reason to keep it loaded. */ + * reason to keep it loaded. + * + * References from other units are *not* checked here. Instead, this is done + * in unit_gc_sweep(), but using markers to properly collect dependency loops. + */ if (u->job) return false; @@ -362,9 +366,6 @@ bool unit_may_gc(Unit *u) { if (u->perpetual) return false; - if (u->refs_by_target) - return false; - if (sd_bus_track_count(u->bus_track) > 0) return false; From a946fa9bb968ac197d7a99970e27388b751dca94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 14 Feb 2018 00:01:05 +0100 Subject: [PATCH 06/11] pid1: free basic unit information at the very end, before freeing the unit We would free stuff like the names of the unit first, and then recurse into other structures to remove the unit from there. Technically this was OK, since the code did not access the name, but this makes debugging harder. And if any log messages are added in any of those functions, they are likely to access u->id and such other basic information about the unit. So let's move the removal of this "basic" information towards the end of unit_free(). --- src/core/unit.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 0330ae51fd..15cfe84e2f 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -644,24 +644,8 @@ void unit_free(Unit *u) { (void) manager_update_failed_units(u->manager, u, false); set_remove(u->manager->startup_units, u); - free(u->description); - strv_free(u->documentation); - free(u->fragment_path); - free(u->source_path); - strv_free(u->dropin_paths); - free(u->instance); - - free(u->job_timeout_reboot_arg); - - set_free_free(u->names); - unit_unwatch_all_pids(u); - condition_free_list(u->conditions); - condition_free_list(u->asserts); - - free(u->reboot_arg); - unit_ref_unset(&u->slice); while (u->refs_by_target) unit_ref_unset(u->refs_by_target); @@ -677,6 +661,22 @@ void unit_free(Unit *u) { bpf_program_unref(u->ip_bpf_ingress); bpf_program_unref(u->ip_bpf_egress); + condition_free_list(u->conditions); + condition_free_list(u->asserts); + + free(u->description); + strv_free(u->documentation); + free(u->fragment_path); + free(u->source_path); + strv_free(u->dropin_paths); + free(u->instance); + + free(u->job_timeout_reboot_arg); + + set_free_free(u->names); + + free(u->reboot_arg); + free(u); } From 1bdf2790025e661e41894129eb390bb032b88585 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 13 Feb 2018 23:57:43 +0100 Subject: [PATCH 07/11] pid1: properly remove references to the unit from gc queue during final cleanup When various references to the unit were dropped during cleanup in unit_free(), add_to_gc_queue() could be called on this unit. If the unit was previously in the gc queue (at the time when unit_free() was called on it), this wouldn't matter, because it'd have in_gc_queue still set even though it was already removed from the queue. But if it wasn't set, then the unit could be added to the queue. Then after unit_free() would deallocate the unit, we would be left with a dangling pointer in gc_queue. A unit could be added to the gc queue in two places called from unit_free(): in the job_install calls, and in unit_ref_unset(). The first was OK, because it was above the LIST_REMOVE(gc_queue,...) call, but the second was not, because it was after that. Move the all LIST_REMOVE() calls down. --- src/core/unit.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 15cfe84e2f..5e619b0b07 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -610,27 +610,6 @@ void unit_free(Unit *u) { for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) bidi_set_free(u, u->dependencies[d]); - if (u->type != _UNIT_TYPE_INVALID) - LIST_REMOVE(units_by_type, u->manager->units_by_type[u->type], u); - - if (u->in_load_queue) - LIST_REMOVE(load_queue, u->manager->load_queue, u); - - if (u->in_dbus_queue) - LIST_REMOVE(dbus_queue, u->manager->dbus_unit_queue, u); - - if (u->in_cleanup_queue) - LIST_REMOVE(cleanup_queue, u->manager->cleanup_queue, u); - - if (u->in_gc_queue) - LIST_REMOVE(gc_queue, u->manager->gc_unit_queue, u); - - if (u->in_cgroup_realize_queue) - LIST_REMOVE(cgroup_realize_queue, u->manager->cgroup_realize_queue, u); - - if (u->in_cgroup_empty_queue) - LIST_REMOVE(cgroup_empty_queue, u->manager->cgroup_empty_queue, u); - if (u->on_console) manager_unref_console(u->manager); @@ -650,6 +629,27 @@ void unit_free(Unit *u) { while (u->refs_by_target) unit_ref_unset(u->refs_by_target); + if (u->type != _UNIT_TYPE_INVALID) + LIST_REMOVE(units_by_type, u->manager->units_by_type[u->type], u); + + if (u->in_load_queue) + LIST_REMOVE(load_queue, u->manager->load_queue, u); + + if (u->in_dbus_queue) + LIST_REMOVE(dbus_queue, u->manager->dbus_unit_queue, u); + + if (u->in_gc_queue) + LIST_REMOVE(gc_queue, u->manager->gc_unit_queue, u); + + if (u->in_cgroup_realize_queue) + LIST_REMOVE(cgroup_realize_queue, u->manager->cgroup_realize_queue, u); + + if (u->in_cgroup_empty_queue) + LIST_REMOVE(cgroup_empty_queue, u->manager->cgroup_empty_queue, u); + + if (u->in_cleanup_queue) + LIST_REMOVE(cleanup_queue, u->manager->cleanup_queue, u); + safe_close(u->ip_accounting_ingress_map_fd); safe_close(u->ip_accounting_egress_map_fd); From 2ab3050f6ed1b1a725f3fc2b7643317f958f9f58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 14 Feb 2018 00:39:06 +0100 Subject: [PATCH 08/11] pid1: rename job_check_gc to job_may_gc The reasoning is the same as for unit_can_gc. v2: - rename can_gc to may_gc --- src/core/job.c | 21 +++++++++++---------- src/core/job.h | 2 +- src/core/manager.c | 2 +- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index c6de8d27e4..1a3bb3bcff 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -1272,7 +1272,7 @@ int job_get_timeout(Job *j, usec_t *timeout) { return 1; } -bool job_check_gc(Job *j) { +bool job_may_gc(Job *j) { Unit *other; Iterator i; void *v; @@ -1280,13 +1280,14 @@ bool job_check_gc(Job *j) { assert(j); /* Checks whether this job should be GC'ed away. We only do this for jobs of units that have no effect on their - * own and just track external state. For now the only unit type that qualifies for this are .device units. */ + * own and just track external state. For now the only unit type that qualifies for this are .device units. + * Returns true if the job can be collected. */ if (!UNIT_VTABLE(j->unit)->gc_jobs) - return true; + return false; if (sd_bus_track_count(j->bus_track) > 0) - return true; + return false; /* FIXME: So this is a bit ugly: for now we don't properly track references made via private bus connections * (because it's nasty, as sd_bus_track doesn't apply to it). We simply remember that the job was once @@ -1296,11 +1297,11 @@ bool job_check_gc(Job *j) { if (set_isempty(j->unit->manager->private_buses)) j->ref_by_private_bus = false; else - return true; + return false; } if (j->type == JOB_NOP) - return true; + return false; /* If a job is ordered after ours, and is to be started, then it needs to wait for us, regardless if we stop or * start, hence let's not GC in that case. */ @@ -1312,7 +1313,7 @@ bool job_check_gc(Job *j) { continue; if (IN_SET(other->job->type, JOB_START, JOB_VERIFY_ACTIVE, JOB_RELOAD)) - return true; + return false; } /* If we are going down, but something else is ordered After= us, then it needs to wait for us */ @@ -1324,7 +1325,7 @@ bool job_check_gc(Job *j) { if (other->job->ignore_order) continue; - return true; + return false; } /* The logic above is kinda the inverse of the job_is_runnable() logic. Specifically, if the job "we" is @@ -1344,7 +1345,7 @@ bool job_check_gc(Job *j) { * */ - return false; + return true; } void job_add_to_gc_queue(Job *j) { @@ -1353,7 +1354,7 @@ void job_add_to_gc_queue(Job *j) { if (j->in_gc_queue) return; - if (job_check_gc(j)) + if (!job_may_gc(j)) return; LIST_PREPEND(gc_queue, j->unit->manager->gc_job_queue, j); diff --git a/src/core/job.h b/src/core/job.h index a2f3b7b5d2..2edb63169c 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -233,7 +233,7 @@ void job_shutdown_magic(Job *j); int job_get_timeout(Job *j, usec_t *timeout) _pure_; -bool job_check_gc(Job *j); +bool job_may_gc(Job *j); void job_add_to_gc_queue(Job *j); int job_get_before(Job *j, Job*** ret); diff --git a/src/core/manager.c b/src/core/manager.c index f6c88ba114..d6ce77d009 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1158,7 +1158,7 @@ static unsigned manager_dispatch_gc_job_queue(Manager *m) { n++; - if (job_check_gc(j)) + if (!job_may_gc(j)) continue; log_unit_debug(j->unit, "Collecting job."); From f698d99cd5f51f311d3c946c15147bda2c611f97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 14 Feb 2018 00:52:21 +0100 Subject: [PATCH 09/11] pid1: also show gc status for jobs like we do for units --- src/core/job.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index 1a3bb3bcff..a653694d32 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -312,11 +312,13 @@ void job_dump(Job *j, FILE*f, const char *prefix) { "%s-> Job %u:\n" "%s\tAction: %s -> %s\n" "%s\tState: %s\n" - "%s\tIrreversible: %s\n", + "%s\tIrreversible: %s\n" + "%s\tMay GC: %s\n", prefix, j->id, prefix, j->unit->id, job_type_to_string(j->type), prefix, job_state_to_string(j->state), - prefix, yes_no(j->irreversible)); + prefix, yes_no(j->irreversible), + prefix, yes_no(job_may_gc(j))); } /* From 5c093a23688ff4d98657da44c25848392a0caf69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 15 Feb 2018 13:14:35 +0100 Subject: [PATCH 10/11] logind: change check_gc to may_gc everywhere --- src/login/logind-seat.c | 10 +++++----- src/login/logind-seat.h | 2 +- src/login/logind-session.c | 14 +++++++------- src/login/logind-session.h | 2 +- src/login/logind-user.c | 14 +++++++------- src/login/logind-user.h | 2 +- src/login/logind.c | 10 +++++----- 7 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index b99e7abf91..46e7c06ddc 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -639,16 +639,16 @@ int seat_get_idle_hint(Seat *s, dual_timestamp *t) { return idle_hint; } -bool seat_check_gc(Seat *s, bool drop_not_started) { +bool seat_may_gc(Seat *s, bool drop_not_started) { assert(s); if (drop_not_started && !s->started) - return false; - - if (seat_is_seat0(s)) return true; - return seat_has_master_device(s); + if (seat_is_seat0(s)) + return false; + + return !seat_has_master_device(s); } void seat_add_to_gc_queue(Seat *s) { diff --git a/src/login/logind-seat.h b/src/login/logind-seat.h index 5427ac21c8..4982ea3381 100644 --- a/src/login/logind-seat.h +++ b/src/login/logind-seat.h @@ -79,7 +79,7 @@ int seat_start(Seat *s); int seat_stop(Seat *s, bool force); int seat_stop_sessions(Seat *s, bool force); -bool seat_check_gc(Seat *s, bool drop_not_started); +bool seat_may_gc(Seat *s, bool drop_not_started); void seat_add_to_gc_queue(Seat *s); bool seat_name_is_valid(const char *name); diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 92eb2943fe..1859150b5e 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -1000,27 +1000,27 @@ static void session_remove_fifo(Session *s) { } } -bool session_check_gc(Session *s, bool drop_not_started) { +bool session_may_gc(Session *s, bool drop_not_started) { assert(s); if (drop_not_started && !s->started) - return false; + return true; if (!s->user) - return false; + return true; if (s->fifo_fd >= 0) { if (pipe_eof(s->fifo_fd) <= 0) - return true; + return false; } if (s->scope_job && manager_job_is_active(s->manager, s->scope_job)) - return true; + return false; if (s->scope && manager_unit_is_active(s->manager, s->scope)) - return true; + return false; - return false; + return true; } void session_add_to_gc_queue(Session *s) { diff --git a/src/login/logind-session.h b/src/login/logind-session.h index 8491832402..16a278c792 100644 --- a/src/login/logind-session.h +++ b/src/login/logind-session.h @@ -131,7 +131,7 @@ struct Session { Session *session_new(Manager *m, const char *id); void session_free(Session *s); void session_set_user(Session *s, User *u); -bool session_check_gc(Session *s, bool drop_not_started); +bool session_may_gc(Session *s, bool drop_not_started); void session_add_to_gc_queue(Session *s); int session_activate(Session *s); bool session_is_active(Session *s); diff --git a/src/login/logind-user.c b/src/login/logind-user.c index 32b2045696..f85564f221 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -680,25 +680,25 @@ int user_check_linger_file(User *u) { return access(p, F_OK) >= 0; } -bool user_check_gc(User *u, bool drop_not_started) { +bool user_may_gc(User *u, bool drop_not_started) { assert(u); if (drop_not_started && !u->started) - return false; + return true; if (u->sessions) - return true; + return false; if (user_check_linger_file(u) > 0) - return true; + return false; if (u->slice_job && manager_job_is_active(u->manager, u->slice_job)) - return true; + return false; if (u->service_job && manager_job_is_active(u->manager, u->service_job)) - return true; + return false; - return false; + return true; } void user_add_to_gc_queue(User *u) { diff --git a/src/login/logind-user.h b/src/login/logind-user.h index ad1686ffc5..c3452dcd08 100644 --- a/src/login/logind-user.h +++ b/src/login/logind-user.h @@ -66,7 +66,7 @@ User *user_free(User *u); DEFINE_TRIVIAL_CLEANUP_FUNC(User *, user_free); -bool user_check_gc(User *u, bool drop_not_started); +bool user_may_gc(User *u, bool drop_not_started); void user_add_to_gc_queue(User *u); int user_start(User *u); int user_stop(User *u, bool force); diff --git a/src/login/logind.c b/src/login/logind.c index d9b5f026cf..3a6ab7fcb6 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -958,7 +958,7 @@ static void manager_gc(Manager *m, bool drop_not_started) { LIST_REMOVE(gc_queue, m->seat_gc_queue, seat); seat->in_gc_queue = false; - if (!seat_check_gc(seat, drop_not_started)) { + if (seat_may_gc(seat, drop_not_started)) { seat_stop(seat, false); seat_free(seat); } @@ -969,14 +969,14 @@ static void manager_gc(Manager *m, bool drop_not_started) { session->in_gc_queue = false; /* First, if we are not closing yet, initiate stopping */ - if (!session_check_gc(session, drop_not_started) && + if (session_may_gc(session, drop_not_started) && session_get_state(session) != SESSION_CLOSING) session_stop(session, false); /* 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)) { + if (session_may_gc(session, drop_not_started)) { session_finalize(session); session_free(session); } @@ -987,11 +987,11 @@ static void manager_gc(Manager *m, bool drop_not_started) { user->in_gc_queue = false; /* First step: queue stop jobs */ - if (!user_check_gc(user, drop_not_started)) + if (user_may_gc(user, drop_not_started)) user_stop(user, false); /* Second step: finalize user */ - if (!user_check_gc(user, drop_not_started)) { + if (user_may_gc(user, drop_not_started)) { user_finalize(user); user_free(user); } From 554ce41f619f0a03b5fc777af4b923ff68fe13d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 15 Feb 2018 13:15:45 +0100 Subject: [PATCH 11/11] machined: change check_gc to may_gc everywhere --- src/machine/machine.c | 14 +++++++------- src/machine/machine.h | 2 +- src/machine/machined.c | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/machine/machine.c b/src/machine/machine.c index 7375d83a44..4bacf91d26 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -486,22 +486,22 @@ int machine_finalize(Machine *m) { return 0; } -bool machine_check_gc(Machine *m, bool drop_not_started) { +bool machine_may_gc(Machine *m, bool drop_not_started) { assert(m); if (m->class == MACHINE_HOST) - return true; - - if (drop_not_started && !m->started) return false; - if (m->scope_job && manager_job_is_active(m->manager, m->scope_job)) + if (drop_not_started && !m->started) return true; + if (m->scope_job && manager_job_is_active(m->manager, m->scope_job)) + return false; + if (m->unit && manager_unit_is_active(m->manager, m->unit)) - return true; + return false; - return false; + return true; } void machine_add_to_gc_queue(Machine *m) { diff --git a/src/machine/machine.h b/src/machine/machine.h index 1ee82ffe81..1ef5dcdb89 100644 --- a/src/machine/machine.h +++ b/src/machine/machine.h @@ -85,7 +85,7 @@ struct Machine { Machine* machine_new(Manager *manager, MachineClass class, const char *name); void machine_free(Machine *m); -bool machine_check_gc(Machine *m, bool drop_not_started); +bool machine_may_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); diff --git a/src/machine/machined.c b/src/machine/machined.c index 34b2024043..9fb67882e1 100644 --- a/src/machine/machined.c +++ b/src/machine/machined.c @@ -292,14 +292,14 @@ void manager_gc(Manager *m, bool drop_not_started) { machine->in_gc_queue = false; /* First, if we are not closing yet, initiate stopping */ - if (!machine_check_gc(machine, drop_not_started) && + if (machine_may_gc(machine, drop_not_started) && machine_get_state(machine) != MACHINE_CLOSING) machine_stop(machine); /* Now, the 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)) { + if (machine_may_gc(machine, drop_not_started)) { machine_finalize(machine); machine_free(machine); }