From 846e541830bce41fcea2f197ae1c8b34e2308cb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 May 2018 17:01:53 +0200 Subject: [PATCH 1/5] journal: small simplification --- src/journal/journal-file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index 606ca604ac..c9b6466bab 100644 --- a/src/journal/journal-file.c +++ b/src/journal/journal-file.c @@ -212,7 +212,7 @@ int journal_file_set_offline(JournalFile *f, bool wait) { if (!f->writable) return -EPERM; - if (!(f->fd >= 0 && f->header)) + if (f->fd < 0 || !f->header) return -EINVAL; /* An offlining journal is implicitly online and may modify f->header->state, @@ -269,7 +269,7 @@ static int journal_file_set_online(JournalFile *f) { if (!f->writable) return -EPERM; - if (!(f->fd >= 0 && f->header)) + if (f->fd < 0 || !f->header) return -EINVAL; while (wait) { From 9f77ba244379346d461596bb4901a64a614746e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 30 May 2018 11:22:18 +0000 Subject: [PATCH 2/5] core/dbus: simplify bus_done_{api,system} functions destroy_bus already has a check for NULL, so we don't need to repeat it here. --- src/core/dbus.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/core/dbus.c b/src/core/dbus.c index e535d9eda9..b206f3af56 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -1098,17 +1098,11 @@ static void destroy_bus(Manager *m, sd_bus **bus) { } void bus_done_api(Manager *m) { - assert(m); - - if (m->api_bus) - destroy_bus(m, &m->api_bus); + destroy_bus(m, &m->api_bus); } void bus_done_system(Manager *m) { - assert(m); - - if (m->system_bus) - destroy_bus(m, &m->system_bus); + destroy_bus(m, &m->system_bus); } void bus_done_private(Manager *m) { From bb2682bc0493ac42bec800e263c4025219012645 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 31 May 2018 15:16:06 +0200 Subject: [PATCH 3/5] Fix reporting of enabled-runtime units We would always report them as "enabled", because path_is_config() includes /run, despite the name. Fixes #9122. --- src/shared/install.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index ecda662f28..e246cd727c 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -894,16 +894,13 @@ static int find_symlinks_in_scope( if (r > 0) { /* We found symlinks in this dir? Yay! Let's see where precisely it is enabled. */ - r = path_is_config(paths, *p, false); - if (r < 0) - return r; - if (r > 0) { + if (path_equal_ptr(*p, paths->persistent_config)) { /* This is the best outcome, let's return it immediately. */ *state = UNIT_FILE_ENABLED; return 1; } - /* look for globally enablement of user units */ + /* look for global enablement of user units */ if (scope == UNIT_FILE_USER && path_is_user_config_dir(*p)) { *state = UNIT_FILE_ENABLED; return 1; @@ -918,11 +915,7 @@ static int find_symlinks_in_scope( enabled_at_all = true; } else if (same_name_link) { - - r = path_is_config(paths, *p, false); - if (r < 0) - return r; - if (r > 0) + if (path_equal_ptr(*p, paths->persistent_config)) same_name_link_config = true; else { r = path_is_runtime(paths, *p, false); From e2e6ca54c30645d8715778b6300c493106f633db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 31 May 2018 15:32:49 +0200 Subject: [PATCH 4/5] systemctl: remove newlines This file is long enough already, we don't need extra vertical whitespace. --- src/systemctl/systemctl.c | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 60861dd791..fbaa464991 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -233,7 +233,6 @@ static void release_busses(void) { } static void ask_password_agent_open_if_enabled(void) { - /* Open the password agent as a child process if necessary */ if (arg_dry_run) @@ -297,7 +296,6 @@ static int translate_bus_error_to_exit_status(int r, const sd_bus_error *error) } static bool install_client_side(void) { - /* Decides when to execute enable/disable/... operations * client-side rather than server-side. */ @@ -1612,7 +1610,6 @@ static int list_dependencies_print(const char *name, int level, unsigned int bra } static int list_dependencies_get_dependencies(sd_bus *bus, const char *name, char ***deps) { - struct DependencyStatusInfo { char **dep[5]; } info = {}; @@ -6501,7 +6498,6 @@ static int show_installation_targets(sd_bus *bus, const char *name) { } static int unit_is_enabled(int argc, char *argv[], void *userdata) { - _cleanup_strv_free_ char **names = NULL; bool enabled; char **name; @@ -7002,7 +6998,6 @@ end: } static void systemctl_help(void) { - (void) pager_open(arg_no_pager, false); printf("%s [OPTIONS...] {COMMAND} ...\n\n" @@ -7260,7 +7255,6 @@ static void help_states(void) { } static int systemctl_parse_argv(int argc, char *argv[]) { - enum { ARG_FAIL = 0x100, ARG_REVERSE, @@ -7695,7 +7689,6 @@ static int systemctl_parse_argv(int argc, char *argv[]) { } static int halt_parse_argv(int argc, char *argv[]) { - enum { ARG_HELP = 0x100, ARG_HALT, @@ -7839,7 +7832,6 @@ static int parse_shutdown_time_spec(const char *t, usec_t *_u) { } static int shutdown_parse_argv(int argc, char *argv[]) { - enum { ARG_HELP = 0x100, ARG_NO_WALL @@ -7946,7 +7938,6 @@ static int shutdown_parse_argv(int argc, char *argv[]) { } static int telinit_parse_argv(int argc, char *argv[]) { - enum { ARG_HELP = 0x100, ARG_NO_WALL @@ -8033,7 +8024,6 @@ static int telinit_parse_argv(int argc, char *argv[]) { } static int runlevel_parse_argv(int argc, char *argv[]) { - enum { ARG_HELP = 0x100, }; @@ -8135,7 +8125,6 @@ static int parse_argv(int argc, char *argv[]) { #if HAVE_SYSV_COMPAT _pure_ static int action_to_runlevel(void) { - static const char table[_ACTION_MAX] = { [ACTION_HALT] = '0', [ACTION_POWEROFF] = '0', @@ -8195,7 +8184,6 @@ static int talk_initctl(void) { } static int systemctl_main(int argc, char *argv[]) { - static const Verb verbs[] = { { "list-units", VERB_ANY, VERB_ANY, VERB_DEFAULT|VERB_ONLINE_ONLY, list_units }, { "list-unit-files", VERB_ANY, VERB_ANY, 0, list_unit_files }, @@ -8271,7 +8259,6 @@ static int systemctl_main(int argc, char *argv[]) { } static int reload_with_fallback(void) { - /* First, try systemd via D-Bus. */ if (daemon_reload(0, NULL, NULL) >= 0) return 0; @@ -8286,7 +8273,6 @@ static int reload_with_fallback(void) { } static int start_with_fallback(void) { - /* First, try systemd via D-Bus. */ if (start_unit(0, NULL, NULL) >= 0) return 0; @@ -8300,7 +8286,6 @@ static int start_with_fallback(void) { } static int halt_now(enum action a) { - /* The kernel will automatically flush ATA disks and suchlike on reboot(), but the file systems need to be * synce'd explicitly in advance. */ if (!arg_no_sync && !arg_dry_run) From 4910b35078ad24dcbc63f372b2fee087640201d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 31 May 2018 15:28:07 +0200 Subject: [PATCH 5/5] systemctl: when removing enablement or mask symlinks, cover both /run and /etc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'systemctl disable --runtime' would disable a unit, but only if it was enabled with '--runtime', and silently do nothing if the unit was enabled persistently. And similarly 'systemctl disable' would do nothing if the unit was enabled in /run. This just doesn't seem useful. This pathch changes enable/disable and mask/unmask to be asymmetrical. enable and mask create symlinks in /etc or /run, depending on whether --runtime was specified. disable and unmask remove symlinks from both locations. --runtime cannot be specified for the disable and unmask verbs. The advantage is that 'disable' now means that the unit is disabled, period. And similarly for 'unmask', all masks are removed. Similarly for preset and preset-all, they now cannot be called with --runtime, and are asymmetrical: when they enable a unit, symlinks are created in /etc. When they disable a unit, all symlinks are nuked. $ systemctl --root=/ enable bluetooth Created symlink /etc/systemd/system/dbus-org.bluez.service → /usr/lib/systemd/system/bluetooth.service. Created symlink /etc/systemd/system/bluetooth.target.wants/bluetooth.service → /usr/lib/systemd/system/bluetooth.service. $ systemctl --root=/ --runtime enable bluetooth Created symlink /run/systemd/system/dbus-org.bluez.service → /usr/lib/systemd/system/bluetooth.service. Created symlink /run/systemd/system/bluetooth.target.wants/bluetooth.service → /usr/lib/systemd/system/bluetooth.service. $ systemctl --root=/ disable bluetooth Removed /run/systemd/system/bluetooth.target.wants/bluetooth.service. Removed /run/systemd/system/dbus-org.bluez.service. Removed /etc/systemd/system/bluetooth.target.wants/bluetooth.service. Removed /etc/systemd/system/dbus-org.bluez.service. $ systemctl --root=/ disable --runtime bluetooth --runtime cannot be used with disable $ systemctl --root=/ mask --runtime bluetooth Created symlink /run/systemd/system/bluetooth.service → /dev/null. $ systemctl --root=/ mask bluetooth Created symlink /etc/systemd/system/bluetooth.service → /dev/null. $ systemctl --root=/ unmask bluetooth Removed /run/systemd/system/bluetooth.service. Removed /etc/systemd/system/bluetooth.service. $ systemctl --root=/ unmask --runtime bluetooth --runtime cannot be used with unmask $ systemctl --root=/ --runtime enable bluetooth Created symlink /run/systemd/system/dbus-org.bluez.service → /usr/lib/systemd/system/bluetooth.service. Created symlink /run/systemd/system/bluetooth.target.wants/bluetooth.service → /usr/lib/systemd/system/bluetooth.service. $ systemctl --root=/ enable bluetooth Created symlink /etc/systemd/system/dbus-org.bluez.service → /usr/lib/systemd/system/bluetooth.service. Created symlink /etc/systemd/system/bluetooth.target.wants/bluetooth.service → /usr/lib/systemd/system/bluetooth.service. $ systemctl --root=/ preset bluetooth Removed /run/systemd/system/bluetooth.target.wants/bluetooth.service. Removed /run/systemd/system/dbus-org.bluez.service. Removed /etc/systemd/system/bluetooth.target.wants/bluetooth.service. Removed /etc/systemd/system/dbus-org.bluez.service. $ systemctl --root=/ preset --runtime bluetooth --runtime cannot be used with preset $ systemctl preset-all --runtime --runtime cannot be used with preset-all --- man/systemctl.xml | 22 +++--- src/shared/install.c | 162 ++++++++++++++++++-------------------- src/systemctl/systemctl.c | 5 ++ 3 files changed, 91 insertions(+), 98 deletions(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index b5b9bec30e..7a2777664e 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -570,19 +570,19 @@ - When used with enable, - disable, edit, - (and related commands), make changes only temporarily, so - that they are lost on the next reboot. This will have the - effect that changes are not made in subdirectories of - /etc but in /run, - with identical immediate effects, however, since the latter + When used with set-property, make changes only + temporarily, so that they are lost on the next reboot. + + Similarily, when used with enable, mask, + edit and related commands, make temporary changes, which are lost on + the next reboot. Changes are not made in subdirectories of /etc, but + in /run. The immediate effect is identical, however since the latter is lost on reboot, the changes are lost too. - Similarly, when used with - set-property, make changes only - temporarily, so that they are lost on the next - reboot. + Note: this option cannot be used with disable, + unmask, preset, or preset-all, + because those operations sometimes need to remove symlinks under /etc + to have the desired effect, which would cause a persistent change. diff --git a/src/shared/install.c b/src/shared/install.c index e246cd727c..4cf40df97d 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1916,7 +1916,6 @@ static int install_context_mark_for_removal( InstallContext *c, const LookupPaths *paths, Set **remove_symlinks_to, - const char *config_path, UnitFileChange **changes, size_t *n_changes) { @@ -1925,7 +1924,6 @@ static int install_context_mark_for_removal( assert(c); assert(paths); - assert(config_path); /* Marks all items for removal */ @@ -2035,7 +2033,7 @@ int unit_file_unmask( size_t n_todo = 0, n_allocated = 0; const char *config_path; char **i; - bool dry_run; + bool dry_run = !!(flags & UNIT_FILE_DRY_RUN); int r, q; assert(scope >= 0); @@ -2045,73 +2043,71 @@ int unit_file_unmask( if (r < 0) return r; - config_path = (flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; - if (!config_path) - return -ENXIO; - - dry_run = !!(flags & UNIT_FILE_DRY_RUN); - STRV_FOREACH(i, files) { - _cleanup_free_ char *path = NULL; - if (!unit_name_is_valid(*i, UNIT_NAME_ANY)) return -EINVAL; - path = path_make_absolute(*i, config_path); - if (!path) - return -ENOMEM; + FOREACH_STRING(config_path, paths.runtime_config, paths.persistent_config) { + _cleanup_free_ char *path = NULL; - r = null_or_empty_path(path); - if (r == -ENOENT) - continue; - if (r < 0) - return r; - if (r == 0) - continue; + path = path_make_absolute(*i, config_path); + if (!path) + return -ENOMEM; - if (!GREEDY_REALLOC0(todo, n_allocated, n_todo + 2)) - return -ENOMEM; + r = null_or_empty_path(path); + if (r == -ENOENT) + continue; + if (r < 0) + return r; + if (r == 0) + continue; - todo[n_todo] = strdup(*i); - if (!todo[n_todo]) - return -ENOMEM; + if (!GREEDY_REALLOC0(todo, n_allocated, n_todo + 2)) + return -ENOMEM; - n_todo++; + todo[n_todo] = strdup(*i); + if (!todo[n_todo]) + return -ENOMEM; + + n_todo++; + } } strv_uniq(todo); r = 0; - STRV_FOREACH(i, todo) { - _cleanup_free_ char *path = NULL; - const char *rp; + FOREACH_STRING(config_path, paths.runtime_config, paths.persistent_config) { + STRV_FOREACH(i, todo) { + _cleanup_free_ char *path = NULL; + const char *rp; - path = path_make_absolute(*i, config_path); - if (!path) - return -ENOMEM; + path = path_make_absolute(*i, config_path); + if (!path) + return -ENOMEM; - if (!dry_run && unlink(path) < 0) { - if (errno != ENOENT) { - if (r >= 0) - r = -errno; - unit_file_changes_add(changes, n_changes, -errno, path, NULL); + if (!dry_run && unlink(path) < 0) { + if (errno != ENOENT) { + if (r >= 0) + r = -errno; + unit_file_changes_add(changes, n_changes, -errno, path, NULL); + } + + continue; } - continue; + unit_file_changes_add(changes, n_changes, UNIT_FILE_UNLINK, path, NULL); + + rp = skip_root(&paths, path); + q = mark_symlink_for_removal(&remove_symlinks_to, rp ?: path); + if (q < 0) + return q; } - unit_file_changes_add(changes, n_changes, UNIT_FILE_UNLINK, path, NULL); - - rp = skip_root(&paths, path); - q = mark_symlink_for_removal(&remove_symlinks_to, rp ?: path); - if (q < 0) - return q; + q = remove_marked_symlinks(remove_symlinks_to, config_path, &paths, dry_run, changes, n_changes); + if (r >= 0) + r = q; } - q = remove_marked_symlinks(remove_symlinks_to, config_path, &paths, dry_run, changes, n_changes); - if (r >= 0) - r = q; - return r; } @@ -2503,6 +2499,7 @@ int unit_file_disable( _cleanup_(lookup_paths_free) LookupPaths paths = {}; _cleanup_(install_context_done) InstallContext c = {}; _cleanup_set_free_free_ Set *remove_symlinks_to = NULL; + bool dry_run = !!(flags & UNIT_FILE_DRY_RUN); const char *config_path; char **i; int r; @@ -2514,10 +2511,6 @@ int unit_file_disable( if (r < 0) return r; - config_path = (flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; - if (!config_path) - return -ENXIO; - STRV_FOREACH(i, files) { if (!unit_name_is_valid(*i, UNIT_NAME_ANY)) return -EINVAL; @@ -2527,11 +2520,17 @@ int unit_file_disable( return r; } - r = install_context_mark_for_removal(scope, &c, &paths, &remove_symlinks_to, config_path, changes, n_changes); + r = install_context_mark_for_removal(scope, &c, &paths, &remove_symlinks_to, changes, n_changes); if (r < 0) return r; - return remove_marked_symlinks(remove_symlinks_to, config_path, &paths, !!(flags & UNIT_FILE_DRY_RUN), changes, n_changes); + FOREACH_STRING(config_path, paths.runtime_config, paths.persistent_config) { + r = remove_marked_symlinks(remove_symlinks_to, config_path, &paths, dry_run, changes, n_changes); + if (r < 0) + return r; + } + + return 0; } int unit_file_reenable( @@ -2915,45 +2914,45 @@ int unit_file_query_preset(UnitFileScope scope, const char *root_dir, const char static int execute_preset( UnitFileScope scope, + UnitFileFlags flags, InstallContext *plus, InstallContext *minus, const LookupPaths *paths, - const char *config_path, char **files, UnitFilePresetMode mode, - bool force, UnitFileChange **changes, size_t *n_changes) { - int r; + const char *config_path; + bool force = !!(flags & UNIT_FILE_FORCE); + bool runtime = !!(flags & UNIT_FILE_RUNTIME); + int r = 0, q; assert(plus); assert(minus); assert(paths); - assert(config_path); if (mode != UNIT_FILE_PRESET_ENABLE_ONLY) { _cleanup_set_free_free_ Set *remove_symlinks_to = NULL; - r = install_context_mark_for_removal(scope, minus, paths, &remove_symlinks_to, config_path, changes, n_changes); - if (r < 0) - return r; + q = install_context_mark_for_removal(scope, minus, paths, &remove_symlinks_to, changes, n_changes); + if (q < 0) + return q; - r = remove_marked_symlinks(remove_symlinks_to, config_path, paths, false, changes, n_changes); - } else - r = 0; + FOREACH_STRING(config_path, paths->runtime_config, paths->persistent_config) { + q = remove_marked_symlinks(remove_symlinks_to, config_path, paths, false, changes, n_changes); + if (r == 0) + r = q; + } + } if (mode != UNIT_FILE_PRESET_DISABLE_ONLY) { - int q; - /* Returns number of symlinks that where supposed to be installed. */ - q = install_context_apply(scope, plus, paths, config_path, force, SEARCH_LOAD, changes, n_changes); - if (r >= 0) { - if (q < 0) - r = q; - else - r += q; - } + q = install_context_apply(scope, plus, paths, + runtime ? paths->runtime_config : paths->persistent_config, + force, SEARCH_LOAD, changes, n_changes); + if (r == 0) + r = q; } return r; @@ -3017,7 +3016,6 @@ int unit_file_preset( _cleanup_(install_context_done) InstallContext plus = {}, minus = {}; _cleanup_(lookup_paths_free) LookupPaths paths = {}; _cleanup_(presets_freep) Presets presets = {}; - const char *config_path; char **i; int r; @@ -3029,10 +3027,6 @@ int unit_file_preset( if (r < 0) return r; - config_path = (flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; - if (!config_path) - return -ENXIO; - r = read_presets(scope, root_dir, &presets); if (r < 0) return r; @@ -3043,7 +3037,7 @@ int unit_file_preset( return r; } - return execute_preset(scope, &plus, &minus, &paths, config_path, files, mode, !!(flags & UNIT_FILE_FORCE), changes, n_changes); + return execute_preset(scope, flags, &plus, &minus, &paths, files, mode, changes, n_changes); } int unit_file_preset_all( @@ -3057,7 +3051,6 @@ int unit_file_preset_all( _cleanup_(install_context_done) InstallContext plus = {}, minus = {}; _cleanup_(lookup_paths_free) LookupPaths paths = {}; _cleanup_(presets_freep) Presets presets = {}; - const char *config_path = NULL; char **i; int r; @@ -3069,10 +3062,6 @@ int unit_file_preset_all( if (r < 0) return r; - config_path = (flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; - if (!config_path) - return -ENXIO; - r = read_presets(scope, root_dir, &presets); if (r < 0) return r; @@ -3090,7 +3079,6 @@ int unit_file_preset_all( } FOREACH_DIRENT(de, d, return -errno) { - if (!unit_name_is_valid(de->d_name, UNIT_NAME_ANY)) continue; @@ -3114,7 +3102,7 @@ int unit_file_preset_all( } } - return execute_preset(scope, &plus, &minus, &paths, config_path, NULL, mode, !!(flags & UNIT_FILE_FORCE), changes, n_changes); + return execute_preset(scope, flags, &plus, &minus, &paths, NULL, mode, changes, n_changes); } static void unit_file_list_free_one(UnitFileList *f) { diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index fbaa464991..11762c7c41 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -7685,6 +7685,11 @@ static int systemctl_parse_argv(int argc, char *argv[]) { return -EINVAL; } + if (arg_runtime && STRPTR_IN_SET(argv[optind], "disable", "unmask", "preset", "preset-all")) { + log_error("--runtime cannot be used with %s", argv[optind]); + return -EINVAL; + } + return 1; }