From 1f998158a988fcf4cd182d9de27e1d8b16cfe474 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 26 Sep 2023 21:41:29 +0800 Subject: [PATCH 1/6] systemctl: reflect that statically enabled units can be in .upholds/ Follow-up for 38f901791f3c4b1cbd04b71323bbef2fdab65f83 --- src/systemctl/systemctl-enable.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/systemctl/systemctl-enable.c b/src/systemctl/systemctl-enable.c index 940f54607f..4f12f929d2 100644 --- a/src/systemctl/systemctl-enable.c +++ b/src/systemctl/systemctl-enable.c @@ -254,7 +254,7 @@ int verb_enable(int argc, char *argv[], void *userdata) { " \n" /* trick: the space is needed so that the line does not get stripped from output */ "Possible reasons for having this kind of units are:\n" "%1$s A unit may be statically enabled by being symlinked from another unit's\n" - " .wants/ or .requires/ directory.\n" + " .wants/, .requires/, or .upholds/ directory.\n" "%1$s A unit's purpose may be to act as a helper for some other unit which has\n" " a requirement dependency on it.\n" "%1$s A unit may be started when needed via activation (socket, path, timer,\n" From c36c81e467a26c407353c76ad52006db7faa820d Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 26 Sep 2023 21:46:15 +0800 Subject: [PATCH 2/6] systemctl: don't duplicate string needlessly --- src/systemctl/systemctl-enable.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/systemctl/systemctl-enable.c b/src/systemctl/systemctl-enable.c index 4f12f929d2..c2d9336d90 100644 --- a/src/systemctl/systemctl-enable.c +++ b/src/systemctl/systemctl-enable.c @@ -267,7 +267,8 @@ int verb_enable(int argc, char *argv[], void *userdata) { /* If some of the units are disabled in user scope but still enabled in global scope, * we emit a warning for that. */ - _cleanup_strv_free_ char **enabled_in_global_scope = NULL; + /* No strv_free here, strings are owned by 'names' */ + _cleanup_free_ char **enabled_in_global_scope = NULL; STRV_FOREACH(name, names) { UnitFileState state; @@ -279,24 +280,24 @@ int verb_enable(int argc, char *argv[], void *userdata) { return log_error_errno(r, "Failed to get unit file state for %s: %m", *name); if (IN_SET(state, UNIT_FILE_ENABLED, UNIT_FILE_ENABLED_RUNTIME)) { - r = strv_extend(&enabled_in_global_scope, *name); + r = strv_push(&enabled_in_global_scope, *name); if (r < 0) return log_oom(); } } if (!strv_isempty(enabled_in_global_scope)) { - _cleanup_free_ char *units = NULL; + _cleanup_free_ char *joined = NULL; - units = strv_join(enabled_in_global_scope, ", "); - if (!units) + joined = strv_join(enabled_in_global_scope, ", "); + if (!joined) return log_oom(); log_notice("The following unit files have been enabled in global scope. This means\n" "they will still be started automatically after a successful disablement\n" "in user scope:\n" "%s", - units); + joined); } } From 6ea32f61f3039d8ab81788470144bba149bb7d7e Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 27 Sep 2023 23:43:47 +0800 Subject: [PATCH 3/6] systemctl: make unit_is_masked always query manager --- src/systemctl/systemctl-edit.c | 7 +------ src/systemctl/systemctl-util.c | 28 +++++++++++++++++----------- src/systemctl/systemctl-util.h | 2 +- 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index e3f25d52d5..ac0bbb4542 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -318,7 +318,6 @@ int verb_edit(int argc, char *argv[], void *userdata) { .remove_parent = !arg_full, .overwrite_with_origin = true, }; - _cleanup_(lookup_paths_free) LookupPaths lp = {}; _cleanup_strv_free_ char **names = NULL; sd_bus *bus; int r; @@ -329,10 +328,6 @@ int verb_edit(int argc, char *argv[], void *userdata) { if (arg_transport != BUS_TRANSPORT_LOCAL) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot edit units remotely."); - r = lookup_paths_init_or_warn(&lp, arg_runtime_scope, 0, arg_root); - if (r < 0) - return r; - r = mac_init(); if (r < 0) return r; @@ -348,7 +343,7 @@ int verb_edit(int argc, char *argv[], void *userdata) { return log_error_errno(SYNTHETIC_ERRNO(ENOENT), "No units matched the specified patterns."); STRV_FOREACH(tmp, names) { - r = unit_is_masked(bus, &lp, *tmp); + r = unit_is_masked(bus, *tmp); if (r < 0) return r; if (r > 0) diff --git a/src/systemctl/systemctl-util.c b/src/systemctl/systemctl-util.c index 074bb8d83b..10df0087b4 100644 --- a/src/systemctl/systemctl-util.c +++ b/src/systemctl/systemctl-util.c @@ -624,25 +624,31 @@ static int unit_find_template_path( return r; } -int unit_is_masked(sd_bus *bus, LookupPaths *lp, const char *name) { +int unit_is_masked(sd_bus *bus, const char *unit) { _cleanup_free_ char *load_state = NULL; int r; - if (unit_name_is_valid(name, UNIT_NAME_TEMPLATE)) { - _cleanup_free_ char *path = NULL; + assert(bus); + assert(unit); - /* A template cannot be loaded, but it can be still masked, so - * we need to use a different method. */ + if (unit_name_is_valid(unit, UNIT_NAME_TEMPLATE)) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + const char *state; - r = unit_file_find_path(lp, name, &path); + r = bus_call_method(bus, bus_systemd_mgr, "GetUnitFileState", &error, &reply, "s", unit); if (r < 0) - return r; - if (r == 0) - return false; - return null_or_empty_path(path); + return log_debug_errno(r, "Failed to get UnitFileState for '%s': %s", + unit, bus_error_message(&error, r)); + + r = sd_bus_message_read(reply, "s", &state); + if (r < 0) + return bus_log_parse_error_debug(r); + + return STR_IN_SET(state, "masked", "masked-runtime"); } - r = unit_load_state(bus, name, &load_state); + r = unit_load_state(bus, unit, &load_state); if (r < 0) return r; diff --git a/src/systemctl/systemctl-util.h b/src/systemctl/systemctl-util.h index 317bab75b7..344075722a 100644 --- a/src/systemctl/systemctl-util.h +++ b/src/systemctl/systemctl-util.h @@ -38,7 +38,7 @@ int maybe_extend_with_unit_dependencies(sd_bus *bus, char ***list); int unit_file_find_path(LookupPaths *lp, const char *unit_name, char **ret_unit_path); int unit_find_paths(sd_bus *bus, const char *unit_name, LookupPaths *lp, bool force_client_side, Hashmap **cached_id_map, Hashmap **cached_name_map, char **ret_fragment_path, char ***ret_dropin_paths); -int unit_is_masked(sd_bus *bus, LookupPaths *lp, const char *name); +int unit_is_masked(sd_bus *bus, const char *unit); int unit_exists(LookupPaths *lp, const char *unit); int unit_get_dependencies(sd_bus *bus, const char *name, char ***ret); From 002db03f54fe426529e0f841d0b48b56211c7bc1 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 26 Sep 2023 22:26:07 +0800 Subject: [PATCH 4/6] systemctl: clean up check_triggering_units Preparation for #311 --- src/systemctl/systemctl-start-unit.c | 4 +- src/systemctl/systemctl-util.c | 71 ++++++++++++++++++++-------- src/systemctl/systemctl-util.h | 3 +- 3 files changed, 56 insertions(+), 22 deletions(-) diff --git a/src/systemctl/systemctl-start-unit.c b/src/systemctl/systemctl-start-unit.c index 551e6f6768..bec2249c82 100644 --- a/src/systemctl/systemctl-start-unit.c +++ b/src/systemctl/systemctl-start-unit.c @@ -393,8 +393,8 @@ int verb_start(int argc, char *argv[], void *userdata) { /* When stopping units, warn if they can still be triggered by * another active unit (socket, path, timer) */ if (!arg_quiet) - STRV_FOREACH(name, stopped_units) - (void) check_triggering_units(bus, *name); + STRV_FOREACH(unit, stopped_units) + warn_triggering_units(bus, *unit, "Stopping", /* ignore_masked = */ true); } if (arg_wait) { diff --git a/src/systemctl/systemctl-util.c b/src/systemctl/systemctl-util.c index 10df0087b4..2498725d3f 100644 --- a/src/systemctl/systemctl-util.c +++ b/src/systemctl/systemctl-util.c @@ -315,26 +315,33 @@ int expand_unit_names(sd_bus *bus, char **names, const char* suffix, char ***ret return 0; } -int check_triggering_units(sd_bus *bus, const char *unit) { +int get_active_triggering_units(sd_bus *bus, const char *unit, bool ignore_masked, char ***ret) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_free_ char *n = NULL, *dbus_path = NULL, *load_state = NULL; - _cleanup_strv_free_ char **triggered_by = NULL; + _cleanup_strv_free_ char **triggered_by = NULL, **active = NULL; + _cleanup_free_ char *name = NULL, *dbus_path = NULL; int r; - r = unit_name_mangle(unit, 0, &n); - if (r < 0) - return log_error_errno(r, "Failed to mangle unit name: %m"); + assert(bus); + assert(unit); + assert(ret); - r = unit_load_state(bus, n, &load_state); + r = unit_name_mangle(unit, 0, &name); if (r < 0) return r; - if (streq(load_state, "masked")) - return 0; + if (ignore_masked) { + r = unit_is_masked(bus, name); + if (r < 0) + return r; + if (r > 0) { + *ret = NULL; + return 0; + } + } - dbus_path = unit_dbus_path_from_name(n); + dbus_path = unit_dbus_path_from_name(name); if (!dbus_path) - return log_oom(); + return -ENOMEM; r = sd_bus_get_property_strv( bus, @@ -345,9 +352,9 @@ int check_triggering_units(sd_bus *bus, const char *unit) { &error, &triggered_by); if (r < 0) - return log_error_errno(r, "Failed to get triggered by array of %s: %s", n, bus_error_message(&error, r)); + return log_debug_errno(r, "Failed to get TriggeredBy property of unit '%s': %s", + name, bus_error_message(&error, r)); - bool first = true; STRV_FOREACH(i, triggered_by) { UnitActiveState active_state; @@ -358,17 +365,43 @@ int check_triggering_units(sd_bus *bus, const char *unit) { if (!IN_SET(active_state, UNIT_ACTIVE, UNIT_RELOADING)) continue; - if (first) { - log_warning("Warning: Stopping %s, but it can still be activated by:", n); - first = false; - } - - log_warning(" %s", *i); + r = strv_extend(&active, *i); + if (r < 0) + return r; } + *ret = TAKE_PTR(active); return 0; } +void warn_triggering_units(sd_bus *bus, const char *unit, const char *operation, bool ignore_masked) { + _cleanup_strv_free_ char **triggered_by = NULL; + _cleanup_free_ char *joined = NULL; + int r; + + assert(bus); + assert(unit); + assert(operation); + + r = get_active_triggering_units(bus, unit, ignore_masked, &triggered_by); + if (r < 0) { + log_warning_errno(r, + "Failed to get triggering units for '%s', ignoring: %m", unit); + return; + } + + if (strv_isempty(triggered_by)) + return; + + joined = strv_join(triggered_by, ", "); + if (!joined) + return (void) log_oom(); + + log_warning("%s '%s', but its triggering units are still active:\n" + "%s", + operation, unit, joined); +} + int need_daemon_reload(sd_bus *bus, const char *unit) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; const char *path; diff --git a/src/systemctl/systemctl-util.h b/src/systemctl/systemctl-util.h index 344075722a..7bddef07ef 100644 --- a/src/systemctl/systemctl-util.h +++ b/src/systemctl/systemctl-util.h @@ -26,7 +26,8 @@ int get_sub_state_one_unit(sd_bus *bus, const char *unit, char **ret_sub_state); int get_unit_list(sd_bus *bus, const char *machine, char **patterns, UnitInfo **unit_infos, int c, sd_bus_message **ret_reply); int expand_unit_names(sd_bus *bus, char **names, const char* suffix, char ***ret, bool *ret_expanded); -int check_triggering_units(sd_bus *bus, const char *unit); +int get_active_triggering_units(sd_bus *bus, const char *unit, bool ignore_masked, char ***ret); +void warn_triggering_units(sd_bus *bus, const char *unit, const char *operation, bool ignore_masked); int need_daemon_reload(sd_bus *bus, const char *unit); From 0b675f97d6a909a706bdc783caf4945d81f482c2 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 26 Sep 2023 23:01:52 +0800 Subject: [PATCH 5/6] systemctl-start: suppress the triggering unit warning when --no-warn --- man/systemctl.xml | 9 ++++++++- src/systemctl/systemctl-start-unit.c | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index 1a424c1e2b..1b7179e10a 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -443,6 +443,10 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err It will not fail if any of the commands configured to stop the unit (ExecStop=, etc.) fail, because the manager will still forcibly terminate the unit. + + If a unit that gets stopped can still be triggered by other units, a warning containing + the names of the triggering units is shown. can be used to suppress + the warning. @@ -2233,7 +2237,10 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err when using disable combined with on units - that are enabled in global scope. + that are enabled in global scope, + + + when a stop-ped unit still has active triggering units. diff --git a/src/systemctl/systemctl-start-unit.c b/src/systemctl/systemctl-start-unit.c index bec2249c82..6927e97747 100644 --- a/src/systemctl/systemctl-start-unit.c +++ b/src/systemctl/systemctl-start-unit.c @@ -392,7 +392,7 @@ int verb_start(int argc, char *argv[], void *userdata) { /* When stopping units, warn if they can still be triggered by * another active unit (socket, path, timer) */ - if (!arg_quiet) + if (!arg_quiet && !arg_no_warn) STRV_FOREACH(unit, stopped_units) warn_triggering_units(bus, *unit, "Stopping", /* ignore_masked = */ true); } From d708bb7c02687f1dfdcb706676cdc31e82e8cfa8 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 26 Sep 2023 23:21:23 +0800 Subject: [PATCH 6/6] systemctl-enable: warn if disabled/masked units has active triggering units Closes #311 --- man/systemctl.xml | 11 ++++++++++- src/systemctl/systemctl-enable.c | 17 ++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index 1b7179e10a..d07c8eae28 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -896,6 +896,10 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err executed. This output may be suppressed by passing . + If a unit gets disabled but its triggering units are still active, a warning containing + the names of the triggering units is shown. can be used to suppress + the warning. + When this command is used with , the units being operated on might still be enabled in global scope, and thus get started automatically even after a successful disablement in user scope. In this case, a warning about it is shown, which can be suppressed @@ -1083,6 +1087,10 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err mode, in which case the directories are below the user's home directory however. + If a unit gets masked but its triggering units are still active, a warning containing + the names of the triggering units is shown. can be used to suppress + the warning. + @@ -2240,7 +2248,8 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err that are enabled in global scope, - when a stop-ped unit still has active triggering units. + when a stop-ped, disable-d, or mask-ed + unit still has active triggering units. diff --git a/src/systemctl/systemctl-enable.c b/src/systemctl/systemctl-enable.c index c2d9336d90..4d554444d3 100644 --- a/src/systemctl/systemctl-enable.c +++ b/src/systemctl/systemctl-enable.c @@ -138,7 +138,8 @@ int verb_enable(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; bool expect_carries_install_info = false; bool send_runtime = true, send_force = true, send_preset_mode = false; - const char *method; + const char *method, *warn_trigger_operation = NULL; + bool warn_trigger_ignore_masked = true; /* suppress "used uninitialized" warning */ sd_bus *bus; if (STR_IN_SET(verb, "mask", "unmask")) { @@ -170,6 +171,9 @@ int verb_enable(int argc, char *argv[], void *userdata) { method = "DisableUnitFilesWithFlagsAndInstallInfo"; expect_carries_install_info = true; send_force = false; + + warn_trigger_operation = "Disabling"; + warn_trigger_ignore_masked = true; } else if (streq(verb, "reenable")) { method = "ReenableUnitFiles"; expect_carries_install_info = true; @@ -185,9 +189,12 @@ int verb_enable(int argc, char *argv[], void *userdata) { expect_carries_install_info = true; ignore_carries_install_info = true; - } else if (streq(verb, "mask")) + } else if (streq(verb, "mask")) { method = "MaskUnitFiles"; - else if (streq(verb, "unmask")) { + + warn_trigger_operation = "Masking"; + warn_trigger_ignore_masked = false; + } else if (streq(verb, "unmask")) { method = "UnmaskUnitFiles"; send_force = false; } else if (streq(verb, "revert")) { @@ -245,6 +252,10 @@ int verb_enable(int argc, char *argv[], void *userdata) { if (r < 0) return r; } + + if (warn_trigger_operation && !arg_quiet && !arg_no_warn) + STRV_FOREACH(unit, names) + warn_triggering_units(bus, *unit, warn_trigger_operation, warn_trigger_ignore_masked); } if (carries_install_info == 0 && !ignore_carries_install_info)