From 7d782f265dda805f9e6a533410e17a94b79012e0 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Thu, 21 Apr 2016 00:57:50 -0400 Subject: shared/install: nicer error message is symlinking chokes on an existing file Fixes #1892. Previously: Failed to enable unit: Invalid argument Now: Failed to enable unit, file /etc/systemd/system/ssh.service already exists. It would be nice to include the unit name in the message too. I looked into this, but it would require major surgery on the whole installation logic, because we first create a list of things to change, and then try to apply them in a loop. To transfer the knowledge which unit was the source of each change, the data structures would have to be extended to carry the unit name over into the second loop. So I'm skipping this for now. --- src/shared/install.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index 71012eafb4..7a98c2d298 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -370,8 +370,14 @@ static int create_symlink( } r = readlink_malloc(new_path, &dest); - if (r < 0) + if (r < 0) { + /* translate EINVAL (non-symlink exists) to EEXIST */ + if (r == -EINVAL) + r = -EEXIST; + + unit_file_changes_add(changes, n_changes, r, new_path, NULL); return r; + } if (path_equal(dest, old_path)) return 0; @@ -382,8 +388,10 @@ static int create_symlink( } r = symlink_atomic(old_path, new_path); - if (r < 0) + if (r < 0) { + unit_file_changes_add(changes, n_changes, r, new_path, NULL); return r; + } unit_file_changes_add(changes, n_changes, UNIT_FILE_UNLINK, new_path, NULL); unit_file_changes_add(changes, n_changes, UNIT_FILE_SYMLINK, new_path, old_path); -- cgit v1.2.3-54-g00ecf From 9a7c402b2a30dbe759f839b8ad88f57f9e74a105 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Thu, 21 Apr 2016 09:23:18 -0400 Subject: core/dbus-manager: drop unused param from installation functions --- src/core/dbus-manager.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index d48b0ca69d..0c86791fe3 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1666,7 +1666,6 @@ static int install_error( static int method_enable_unit_files_generic( sd_bus_message *message, Manager *m, - const char *verb, int (*call)(UnitFileScope scope, bool runtime, const char *root_dir, char *files[], bool force, UnitFileChange **changes, unsigned *n_changes), bool carries_install_info, sd_bus_error *error) { @@ -1701,15 +1700,15 @@ static int method_enable_unit_files_generic( } static int method_enable_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_enable_unit_files_generic(message, userdata, "enable", unit_file_enable, true, error); + return method_enable_unit_files_generic(message, userdata, unit_file_enable, true, error); } static int method_reenable_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_enable_unit_files_generic(message, userdata, "enable", unit_file_reenable, true, error); + return method_enable_unit_files_generic(message, userdata, unit_file_reenable, true, error); } static int method_link_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_enable_unit_files_generic(message, userdata, "enable", unit_file_link, false, error); + return method_enable_unit_files_generic(message, userdata, unit_file_link, false, error); } static int unit_file_preset_without_mode(UnitFileScope scope, bool runtime, const char *root_dir, char **files, bool force, UnitFileChange **changes, unsigned *n_changes) { @@ -1717,11 +1716,11 @@ static int unit_file_preset_without_mode(UnitFileScope scope, bool runtime, cons } static int method_preset_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_enable_unit_files_generic(message, userdata, "enable", unit_file_preset_without_mode, true, error); + return method_enable_unit_files_generic(message, userdata, unit_file_preset_without_mode, true, error); } static int method_mask_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_enable_unit_files_generic(message, userdata, "disable", unit_file_mask, false, error); + return method_enable_unit_files_generic(message, userdata, unit_file_mask, false, error); } static int method_preset_unit_files_with_mode(sd_bus_message *message, void *userdata, sd_bus_error *error) { @@ -1769,7 +1768,6 @@ static int method_preset_unit_files_with_mode(sd_bus_message *message, void *use static int method_disable_unit_files_generic( sd_bus_message *message, Manager *m, - const char *verb, int (*call)(UnitFileScope scope, bool runtime, const char *root_dir, char *files[], UnitFileChange **changes, unsigned *n_changes), sd_bus_error *error) { @@ -1803,11 +1801,11 @@ static int method_disable_unit_files_generic( } static int method_disable_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_disable_unit_files_generic(message, userdata, "disable", unit_file_disable, error); + return method_disable_unit_files_generic(message, userdata, unit_file_disable, error); } static int method_unmask_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_disable_unit_files_generic(message, userdata, "enable", unit_file_unmask, error); + return method_disable_unit_files_generic(message, userdata, unit_file_unmask, error); } static int method_revert_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { -- cgit v1.2.3-54-g00ecf From 12bf0ae4c6c338603bd11bac9002d73339aedae4 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Thu, 21 Apr 2016 09:53:48 -0400 Subject: shared/install: rewrite unit_file_changes_add() path_kill_slashes was applied to the wrong arg... --- src/shared/install.c | 38 ++++++++++++++------------------------ 1 file changed, 14 insertions(+), 24 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index 7a98c2d298..63cb76f21b 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -222,8 +222,8 @@ int unit_file_changes_add( const char *path, const char *source) { + _cleanup_free_ char *p = NULL, *s = NULL; UnitFileChange *c; - unsigned i; assert(path); assert(!changes == !n_changes); @@ -234,29 +234,22 @@ int unit_file_changes_add( c = realloc(*changes, (*n_changes + 1) * sizeof(UnitFileChange)); if (!c) return -ENOMEM; - *changes = c; - i = *n_changes; - - c[i].type = type; - c[i].path = strdup(path); - if (!c[i].path) - return -ENOMEM; - path_kill_slashes(c[i].path); + p = strdup(path); + if (source) + s = strdup(source); - if (source) { - c[i].source = strdup(source); - if (!c[i].source) { - free(c[i].path); - return -ENOMEM; - } + if (!p || (source && !s)) + return -ENOMEM; - path_kill_slashes(c[i].path); - } else - c[i].source = NULL; + path_kill_slashes(p); + if (s) + path_kill_slashes(s); - *n_changes = i+1; + c[*n_changes] = (UnitFileChange) { type, p, s }; + p = s = NULL; + (*n_changes) ++; return 0; } @@ -265,9 +258,6 @@ void unit_file_changes_free(UnitFileChange *changes, unsigned n_changes) { assert(changes || n_changes == 0); - if (!changes) - return; - for (i = 0; i < n_changes; i++) { free(changes[i].path); free(changes[i].source); @@ -529,8 +519,8 @@ static int remove_marked_symlinks_fd( unit_file_changes_add(changes, n_changes, UNIT_FILE_UNLINK, p, NULL); - /* Now, remember the full path (but with the root prefix removed) of the symlink we just - * removed, and remove any symlinks to it, too */ + /* Now, remember the full path (but with the root prefix removed) of + * the symlink we just removed, and remove any symlinks to it, too. */ rp = skip_root(lp, p); q = mark_symlink_for_removal(&remove_symlinks_to, rp ?: p); -- cgit v1.2.3-54-g00ecf From 39207373dd638e548019ddb49929f15795b8b404 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Thu, 21 Apr 2016 20:04:21 -0400 Subject: systemctl,pid1: do not warn about missing install info with "preset" When "preset" was executed for a unit without install info, we'd warn similarly as for "enable" and "disable". But "preset" is usually called for all units, because the preset files are provided by the distribution, and the units are under control of individual programs, and it's reasonable to call "preset" for all units rather then try to do it only for the ones that can be installed. We also don't warn about missing info for "preset-all". Thus it seems reasonable to silently ignore units w/o install info when presetting. (In addition, when more than one unit was specified, we'd issue the warning only if none of them had install info. But this is probably something to fix for enable/disable too.) --- man/systemctl.xml | 26 +++++++++++++------------- src/systemctl/systemctl.c | 7 ++++--- 2 files changed, 17 insertions(+), 16 deletions(-) (limited to 'src') diff --git a/man/systemctl.xml b/man/systemctl.xml index 5f624243f7..991e9bafaf 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -1084,22 +1084,22 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service preset NAME... - Reset one or more unit files, as specified on the - command line, to the defaults configured in the preset - policy files. This has the same effect as - disable or enable, - depending how the unit is listed in the preset files. + Reset the enable/disable status one or more unit files, as specified on + the command line, to the defaults configured in the preset policy files. This + has the same effect as disable or + enable, depending how the unit is listed in the preset + files. - Use to control - whether units shall be enabled and disabled, or only - enabled, or only disabled. + Use to control whether units shall be + enabled and disabled, or only enabled, or only disabled. + + If the unit carries no install information, it will be silently ignored + by this command. - For more information on the preset policy format, - see + For more information on the preset policy format, see systemd.preset5. - For more information on the concept of presets, please - consult the Preset + For more information on the concept of presets, please consult the + Preset document. diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 059e985463..04205411dd 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -5390,6 +5390,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) { UnitFileChange *changes = NULL; unsigned n_changes = 0; int carries_install_info = -1; + bool ignore_carries_install_info = false; int r; if (!argv[1]) @@ -5420,7 +5421,6 @@ static int enable_unit(int argc, char *argv[], void *userdata) { r = unit_file_link(arg_scope, arg_runtime, arg_root, names, arg_force, &changes, &n_changes); else if (streq(verb, "preset")) { r = unit_file_preset(arg_scope, arg_runtime, arg_root, names, arg_preset_mode, arg_force, &changes, &n_changes); - carries_install_info = r; } else if (streq(verb, "mask")) r = unit_file_mask(arg_scope, arg_runtime, arg_root, names, arg_force, &changes, &n_changes); else if (streq(verb, "unmask")) @@ -5437,7 +5437,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) { } else { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL, *m = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - int expect_carries_install_info = false; + bool expect_carries_install_info = false; bool send_runtime = true, send_force = true, send_preset_mode = false; const char *method; sd_bus *bus; @@ -5468,6 +5468,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) { method = "PresetUnitFiles"; expect_carries_install_info = true; + ignore_carries_install_info = true; } else if (streq(verb, "mask")) method = "MaskUnitFiles"; else if (streq(verb, "unmask")) { @@ -5532,7 +5533,7 @@ static int enable_unit(int argc, char *argv[], void *userdata) { r = 0; } - if (carries_install_info == 0) + if (carries_install_info == 0 && !ignore_carries_install_info) log_warning("The unit files have no installation config (WantedBy, RequiredBy, Also, Alias\n" "settings in the [Install] section, and DefaultInstance for template units).\n" "This means they are not meant to be enabled using systemctl.\n" -- cgit v1.2.3-54-g00ecf From 29380daff549b117873f4543a649569be84bd950 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Thu, 21 Apr 2016 21:11:15 -0400 Subject: shared/install: always overwrite symlinks in .wants and .requires Before: $ systemctl preset getty@.service Failed to preset unit, file /etc/systemd/system/getty.target.wants/getty@tty1.service already exists and is a symlink to ../../../../usr/lib/systemd/system/getty@.service. After: $ systemctl preset getty@.service Created symlink /etc/systemd/system/getty.target.wants/getty@tty1.service, pointing to /usr/lib/systemd/system/getty@.service. We don't really care where the symlink points to. For example, it might point to /usr/lib or /etc, and systemd will always load the unit from /etc in preference to /usr/lib. In fact, if we make a symlink like /etc/systemd/system/multi-user.target.wants/b.service -> ../a.service, pid1 will still start b.service. The name of the symlink is the only thing that matters, as far as systemd is concerned. For humans it's confusing when the symlinks points to anything else than the actual unit file. At the very least, the symlink is supposed to point to a file with the same name in some other directory. Since we don't care where the symlink points, we can always replace an existing symlink. Another option I considered would be to simply leave an existing symlink in place. That would work too, but replacing the symlink with the expected value seems more intuitive. Of course those considerations only apply to .wants and .requires. Symlinks created with "link" and "alias" are a separate matter. Fixes #3056. --- src/shared/install.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/shared/install.c b/src/shared/install.c index 63cb76f21b..e97721b79e 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1390,7 +1390,6 @@ static int install_info_symlink_wants( const char *config_path, char **list, const char *suffix, - bool force, UnitFileChange **changes, unsigned *n_changes) { @@ -1438,7 +1437,7 @@ static int install_info_symlink_wants( rp = skip_root(paths, i->path); - q = create_symlink(rp ?: i->path, path, force, changes, n_changes); + q = create_symlink(rp ?: i->path, path, true, changes, n_changes); if (r == 0) r = q; } @@ -1497,11 +1496,11 @@ static int install_info_apply( r = install_info_symlink_alias(i, paths, config_path, force, changes, n_changes); - q = install_info_symlink_wants(i, paths, config_path, i->wanted_by, ".wants/", force, changes, n_changes); + q = install_info_symlink_wants(i, paths, config_path, i->wanted_by, ".wants/", changes, n_changes); if (r == 0) r = q; - q = install_info_symlink_wants(i, paths, config_path, i->required_by, ".requires/", force, changes, n_changes); + q = install_info_symlink_wants(i, paths, config_path, i->required_by, ".requires/", changes, n_changes); if (r == 0) r = q; -- cgit v1.2.3-54-g00ecf