From d9c1c43e678f93e1bda7478870c615f54a01e7b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 13 Dec 2019 18:42:54 +0100 Subject: [PATCH 1/4] shared/install: remove duplicated check install_info_add() does the exact same check. --- src/shared/install.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 5680660928..53446611be 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1138,9 +1138,6 @@ static int config_parse_also( if (r < 0) return r; - if (!unit_name_is_valid(printed, UNIT_NAME_ANY)) - return -EINVAL; - r = install_info_add(c, printed, NULL, true, &alsoinfo); if (r < 0) return r; From d7ceaf72618aa2466fc4465ce8fcebf0cba68cc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 13 Dec 2019 18:36:29 +0100 Subject: [PATCH 2/4] shared/install: provide a nicer error message for invalid WantedBy=/Required= values $ build/systemctl --user cat badinstall # /home/zbyszek/.config/systemd/user/badinstall.service [Service] ExecStart=true [Install] WantedBy=asdf $ build/systemctl --user enable badinstall Failed to enable unit: "asdf" is not a valid unit name. Fixes #4209. --- src/core/dbus-manager.c | 6 ++++++ src/shared/install.c | 9 ++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 64a612a7f0..c751e84253 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1902,6 +1902,12 @@ static int install_error( "Unit %s is transient or generated.", changes[i].path); goto found; + case -EUCLEAN: + r = sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, + "\"%s\" is not a valid unit name.", + changes[i].path); + goto found; + case -ELOOP: r = sd_bus_error_setf(error, BUS_ERROR_UNIT_LINKED, "Refusing to operate on alias name or linked unit file: %s", diff --git a/src/shared/install.c b/src/shared/install.c index 53446611be..d291eb6f21 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -362,6 +362,12 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang verb, changes[i].path); logged = true; break; + case -EUCLEAN: + log_error_errno(changes[i].type, + "Failed to %s unit, \"%s\" is not a valid unit name.", + verb, changes[i].path); + logged = true; + break; case -ELOOP: log_error_errno(changes[i].type, "Failed to %s unit, refusing to operate on linked unit file %s", verb, changes[i].path); @@ -1794,7 +1800,8 @@ static int install_info_symlink_wants( return q; if (!unit_name_is_valid(dst, UNIT_NAME_ANY)) { - r = -EINVAL; + unit_file_changes_add(changes, n_changes, -EUCLEAN, dst, NULL); + r = -EUCLEAN; continue; } From e51712963b817995781534975470f1b54171702b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 13 Dec 2019 19:25:50 +0100 Subject: [PATCH 3/4] shared/install: log syntax error for invalid DefaultInstance= Ideally, we would want to report this over back over dbus. But that is pretty hard, because the unitfile parsing logic doesn't provide any feedback. systemd-analyze verify also doesn't notice the issue, because it doesn't look at the [Install] section at all. Let's print a message in the logs at least. --- TODO | 1 + src/shared/install.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/TODO b/TODO index aded50fee6..de560d3f15 100644 --- a/TODO +++ b/TODO @@ -1039,6 +1039,7 @@ Features: - introduce mix of BindTo and Requisite - add a concept of RemainAfterExit= to scope units - Allow multiple ExecStart= for all Type= settings, so that we can cover rescue.service nicely + - add verification of [Install] section to systemd-analyze verify * udev-link-config: - Make sure ID_PATH is always exported and complete for diff --git a/src/shared/install.c b/src/shared/install.c index d291eb6f21..0e673b3358 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1197,7 +1197,8 @@ static int config_parse_default_instance( } if (!unit_instance_is_valid(printed)) - return -EINVAL; + return log_syntax(unit, LOG_WARNING, filename, line, SYNTHETIC_ERRNO(EINVAL), + "Invalid DefaultInstance= value \"%s\".", printed); return free_and_replace(i->default_instance, printed); } From b742942edf60307c58c7cad2c4e4e5143051d95c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 13 Dec 2019 19:30:00 +0100 Subject: [PATCH 4/4] TODO: drop entry Implemented in 7d1e91d1a9504ab1bc03894038f90a8e87a4e982. --- TODO | 2 -- 1 file changed, 2 deletions(-) diff --git a/TODO b/TODO index de560d3f15..3306c7814c 100644 --- a/TODO +++ b/TODO @@ -1033,8 +1033,6 @@ Features: - allow Type=simple with PIDFile= https://bugzilla.redhat.com/show_bug.cgi?id=723942 - allow writing multiple conditions in unit files on one line - - load-fragment: when loading a unit file via a chain of symlinks - verify that it is not masked via any of the names traversed. - introduce Type=pid-file - introduce mix of BindTo and Requisite - add a concept of RemainAfterExit= to scope units