From 5125e76243c56662d9d0d91385a01ae8cb271e71 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 18:56:25 +0100 Subject: [PATCH 01/11] core: move specifier expansion out of service.c/socket.c This monopolizes unit file specifier expansion in load-fragment.c, and removes it from socket.c + service.c. This way expansion becomes an operation done exclusively at time of loading unit files. Previously specifiers were resolved for all settings during loading of unit files with the exception of ExecStart= and friends which were resolved in socket.c and service.c. With this change the latter is also moved to the loading of unit files. Fixes: #3061 --- src/core/load-fragment.c | 64 +++++++++++++++++++++++----------------- src/core/service.c | 9 ++---- src/core/socket.c | 8 +---- 3 files changed, 40 insertions(+), 41 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 970eed27c1..c844ec2566 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -579,6 +579,7 @@ int config_parse_exec( void *userdata) { ExecCommand **e = data; + Unit *u = userdata; const char *p; bool semicolon; int r; @@ -604,7 +605,7 @@ int config_parse_exec( _cleanup_free_ ExecCommand *nce = NULL; _cleanup_strv_free_ char **n = NULL; size_t nlen = 0, nbufsize = 0; - char *f; + const char *f; int i; semicolon = false; @@ -631,47 +632,47 @@ int config_parse_exec( f++; } - if (isempty(f)) { + r = unit_full_printf(u, f, &path); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers on %s, ignoring: %m", f); + return 0; + } + + if (isempty(path)) { /* First word is either "-" or "@" with no command. */ log_syntax(unit, LOG_ERR, filename, line, 0, "Empty path in command line, ignoring: \"%s\"", rvalue); return 0; } - if (!string_is_safe(f)) { + if (!string_is_safe(path)) { log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path contains special characters, ignoring: %s", rvalue); return 0; } - if (!path_is_absolute(f)) { + if (!path_is_absolute(path)) { log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path is not absolute, ignoring: %s", rvalue); return 0; } - if (endswith(f, "/")) { + if (endswith(path, "/")) { log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path specifies a directory, ignoring: %s", rvalue); return 0; } - if (f == firstword) { - path = firstword; - firstword = NULL; - } else { - path = strdup(f); - if (!path) - return log_oom(); - } - if (!separate_argv0) { + char *w = NULL; + if (!GREEDY_REALLOC(n, nbufsize, nlen + 2)) return log_oom(); - f = strdup(path); - if (!f) + + w = strdup(path); + if (!w) return log_oom(); - n[nlen++] = f; + n[nlen++] = w; n[nlen] = NULL; } path_kill_slashes(path); while (!isempty(p)) { - _cleanup_free_ char *word = NULL; + _cleanup_free_ char *word = NULL, *resolved = NULL; /* Check explicitly for an unquoted semicolon as * command separator token. */ @@ -682,18 +683,21 @@ int config_parse_exec( break; } - /* Check for \; explicitly, to not confuse it with \\; - * or "\;" or "\\;" etc. extract_first_word would - * return the same for all of those. */ + /* Check for \; explicitly, to not confuse it with \\; or "\;" or "\\;" etc. + * extract_first_word() would return the same for all of those. */ if (p[0] == '\\' && p[1] == ';' && (!p[2] || strchr(WHITESPACE, p[2]))) { + char *w; + p += 2; p += strspn(p, WHITESPACE); + if (!GREEDY_REALLOC(n, nbufsize, nlen + 2)) return log_oom(); - f = strdup(";"); - if (!f) + + w = strdup(";"); + if (!w) return log_oom(); - n[nlen++] = f; + n[nlen++] = w; n[nlen] = NULL; continue; } @@ -701,14 +705,20 @@ int config_parse_exec( r = extract_first_word_and_warn(&p, &word, NULL, EXTRACT_QUOTES|EXTRACT_CUNESCAPE, unit, filename, line, rvalue); if (r == 0) break; - else if (r < 0) + if (r < 0) return 0; + r = unit_full_printf(u, word, &resolved); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, 0, "Failed to resolve unit specifiers on %s, ignoring: %m", word); + return 0; + } + if (!GREEDY_REALLOC(n, nbufsize, nlen + 2)) return log_oom(); - n[nlen++] = word; + n[nlen++] = resolved; n[nlen] = NULL; - word = NULL; + resolved = NULL; } if (!n || !n[0]) { diff --git a/src/core/service.c b/src/core/service.c index c68a7122b6..d5b8decc20 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -49,7 +49,6 @@ #include "string-util.h" #include "strv.h" #include "unit-name.h" -#include "unit-printf.h" #include "unit.h" #include "utf8.h" #include "util.h" @@ -1205,7 +1204,7 @@ static int service_spawn( ExecFlags flags, pid_t *_pid) { - _cleanup_strv_free_ char **argv = NULL, **final_env = NULL, **our_env = NULL, **fd_names = NULL; + _cleanup_strv_free_ char **final_env = NULL, **our_env = NULL, **fd_names = NULL; _cleanup_free_ int *fds = NULL; unsigned n_fds = 0, n_env = 0; const char *path; @@ -1263,10 +1262,6 @@ static int service_spawn( if (r < 0) return r; - r = unit_full_printf_strv(UNIT(s), c->argv, &argv); - if (r < 0) - return r; - our_env = new0(char*, 9); if (!our_env) return -ENOMEM; @@ -1349,7 +1344,7 @@ static int service_spawn( } else path = UNIT(s)->cgroup_path; - exec_params.argv = argv; + exec_params.argv = c->argv; exec_params.environment = final_env; exec_params.fds = fds; exec_params.fd_names = fd_names; diff --git a/src/core/socket.c b/src/core/socket.c index 1a53d47f21..fee9b702e6 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -54,7 +54,6 @@ #include "string-util.h" #include "strv.h" #include "unit-name.h" -#include "unit-printf.h" #include "unit.h" #include "user-util.h" #include "in-addr-util.h" @@ -1740,7 +1739,6 @@ static int socket_coldplug(Unit *u) { } static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { - _cleanup_free_ char **argv = NULL; pid_t pid; int r; ExecParameters exec_params = { @@ -1772,11 +1770,7 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - r = unit_full_printf_strv(UNIT(s), c->argv, &argv); - if (r < 0) - return r; - - exec_params.argv = argv; + exec_params.argv = c->argv; exec_params.environment = UNIT(s)->manager->environment; exec_params.confirm_spawn = manager_get_confirm_spawn(UNIT(s)->manager); exec_params.cgroup_supported = UNIT(s)->manager->cgroup_supported; From 13e40f5a4cd2cbecd3d35e0d6b277749b1d21272 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 19:10:44 +0100 Subject: [PATCH 02/11] man: drop reference to %U being useless This paragraph was a missed left-over from 79413b673b45adc98dfeaec882bbdda2343cb2f9. Drop it now. --- man/systemd.unit.xml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 40c4cfd854..a767b2e80a 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1314,13 +1314,6 @@ - Please note that specifiers %U, - %h, %s are mostly useless - when systemd is running in system mode. PID 1 cannot query the - user account database for information, so the specifiers only work - as shortcuts for things which are already specified in a different - way in the unit file. They are fully functional when systemd is - running in mode. From b1801e6433c30cb0ab7d7c823c98c637edfe0720 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 19:12:54 +0100 Subject: [PATCH 03/11] core: resolve more specifiers in unit_name_printf() unit_name_printf() is usually what we use when the resulting string shall qualify as unit name, and it hence avoids resolving specifiers that almost certainly won't result in valid unit names. Add a couple of more specifiers that unit_full_printf() resolves also to the list unit_name_printf() resolves, as they are likely to be useful in valid unit names too. (Note that there might be cases where this doesn't hold, but we should still permit this, as more often than not they are safe, and if people want to use them that way, they should be able to.) --- src/core/unit-printf.c | 47 ++++++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index 1f5dc6fd88..4c95127c80 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -194,13 +194,20 @@ static int specifier_user_shell(char specifier, void *data, void *userdata, char int unit_name_printf(Unit *u, const char* format, char **ret) { /* - * This will use the passed string as format string and - * replace the following specifiers: + * This will use the passed string as format string and replace the following specifiers (which should all be + * safe for inclusion in unit names): * * %n: the full id of the unit (foo@bar.waldo) * %N: the id of the unit without the suffix (foo@bar) * %p: the prefix (foo) * %i: the instance (bar) + * + * %U: the UID of the running user + * %u: the username of the running user + * + * %m: the machine ID of the running system + * %H: the host name of the running system + * %b: the boot ID of the running system */ const Specifier table[] = { @@ -208,7 +215,14 @@ int unit_name_printf(Unit *u, const char* format, char **ret) { { 'N', specifier_prefix_and_instance, NULL }, { 'p', specifier_prefix, NULL }, { 'i', specifier_string, u->instance }, - { 0, NULL, NULL } + + { 'U', specifier_user_id, NULL }, + { 'u', specifier_user_name, NULL }, + + { 'm', specifier_machine_id, NULL }, + { 'H', specifier_host_name, NULL }, + { 'b', specifier_boot_id, NULL }, + {} }; assert(u); @@ -220,22 +234,19 @@ int unit_name_printf(Unit *u, const char* format, char **ret) { int unit_full_printf(Unit *u, const char *format, char **ret) { - /* This is similar to unit_name_printf() but also supports - * unescaping. Also, adds a couple of additional codes: + /* This is similar to unit_name_printf() but also supports unescaping. Also, adds a couple of additional codes + * (which are likely not suitable for unescaped inclusion in unit names): * - * %f the instance if set, otherwise the id - * %c cgroup path of unit - * %r where units in this slice are placed in the cgroup tree - * %R the root of this systemd's instance tree - * %t the runtime directory to place sockets in (e.g. "/run" or $XDG_RUNTIME_DIR) - * %U the UID of the running user - * %u the username of the running user - * %h the homedir of the running user - * %s the shell of the running user - * %m the machine ID of the running system - * %H the host name of the running system - * %b the boot ID of the running system - * %v `uname -r` of the running system + * %f: the unescaped instance if set, otherwise the id unescaped as path + * %c: cgroup path of unit + * %r: where units in this slice are placed in the cgroup tree + * %R: the root of this systemd's instance tree + * %t: the runtime directory to place sockets in (e.g. "/run" or $XDG_RUNTIME_DIR) + * + * %h: the homedir of the running user + * %s: the shell of the running user + * + * %v: `uname -r` of the running system */ const Specifier table[] = { From 18913df9a2aa5ee53a1dfb6f3cf8cdddcc7f11a3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 19:25:44 +0100 Subject: [PATCH 04/11] core: use unit_full_printf() at a couple of locations we used unit_name_printf() before For settings that are not taking unit names there's no reason to use unit_name_printf(). Use unit_full_printf() instead, as the names are validated anyway in one form or another after expansion. --- src/core/load-fragment.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index c844ec2566..3ffb417f97 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -1336,7 +1336,7 @@ int config_parse_exec_selinux_context( } else ignore = false; - r = unit_name_printf(u, rvalue, &k); + r = unit_full_printf(u, rvalue, &k); if (r < 0) { log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m"); return 0; @@ -1384,7 +1384,7 @@ int config_parse_exec_apparmor_profile( } else ignore = false; - r = unit_name_printf(u, rvalue, &k); + r = unit_full_printf(u, rvalue, &k); if (r < 0) { log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m"); return 0; @@ -1432,7 +1432,7 @@ int config_parse_exec_smack_process_label( } else ignore = false; - r = unit_name_printf(u, rvalue, &k); + r = unit_full_printf(u, rvalue, &k); if (r < 0) { log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m"); return 0; @@ -1699,7 +1699,7 @@ int config_parse_fdname( return 0; } - r = unit_name_printf(UNIT(s), rvalue, &p); + r = unit_full_printf(UNIT(s), rvalue, &p); if (r < 0) { log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %s", rvalue); return 0; @@ -3720,7 +3720,7 @@ int config_parse_runtime_directory( return 0; } - r = unit_name_printf(u, word, &k); + r = unit_full_printf(u, word, &k); if (r < 0) { log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers in \"%s\", ignoring: %m", word); From d107589cd2767c95fb1c794d4f90832301a297f5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 19:38:39 +0100 Subject: [PATCH 05/11] core: turn on specifier expansion for more unit file settings Let's permit specifier expansion at a numbre of additional fields, where arbitrary strings might be passed where this might be useful one day. (Or at least where there's no clear reason where it wouldn't make sense to have.) --- src/core/load-fragment-gperf.gperf.m4 | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index f4ef5a0140..2610442b91 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -191,13 +191,13 @@ Unit.IgnoreOnIsolate, config_parse_bool, 0, Unit.IgnoreOnSnapshot, config_parse_warn_compat, DISABLED_LEGACY, 0 Unit.JobTimeoutSec, config_parse_sec_fix_0, 0, offsetof(Unit, job_timeout) Unit.JobTimeoutAction, config_parse_emergency_action, 0, offsetof(Unit, job_timeout_action) -Unit.JobTimeoutRebootArgument, config_parse_string, 0, offsetof(Unit, job_timeout_reboot_arg) +Unit.JobTimeoutRebootArgument, config_parse_unit_string_printf, 0, offsetof(Unit, job_timeout_reboot_arg) Unit.StartLimitIntervalSec, config_parse_sec, 0, offsetof(Unit, start_limit.interval) m4_dnl The following is a legacy alias name for compatibility Unit.StartLimitInterval, config_parse_sec, 0, offsetof(Unit, start_limit.interval) Unit.StartLimitBurst, config_parse_unsigned, 0, offsetof(Unit, start_limit.burst) Unit.StartLimitAction, config_parse_emergency_action, 0, offsetof(Unit, start_limit_action) -Unit.RebootArgument, config_parse_string, 0, offsetof(Unit, reboot_arg) +Unit.RebootArgument, config_parse_unit_string_printf, 0, offsetof(Unit, reboot_arg) Unit.ConditionPathExists, config_parse_unit_condition_path, CONDITION_PATH_EXISTS, offsetof(Unit, conditions) Unit.ConditionPathExistsGlob, config_parse_unit_condition_path, CONDITION_PATH_EXISTS_GLOB, offsetof(Unit, conditions) Unit.ConditionPathIsDirectory, config_parse_unit_condition_path, CONDITION_PATH_IS_DIRECTORY, offsetof(Unit, conditions) @@ -254,7 +254,7 @@ m4_dnl The following three only exist for compatibility, they moved into Unit, s Service.StartLimitInterval, config_parse_sec, 0, offsetof(Unit, start_limit.interval) Service.StartLimitBurst, config_parse_unsigned, 0, offsetof(Unit, start_limit.burst) Service.StartLimitAction, config_parse_emergency_action, 0, offsetof(Unit, start_limit_action) -Service.RebootArgument, config_parse_string, 0, offsetof(Unit, reboot_arg) +Service.RebootArgument, config_parse_unit_path_printf, 0, offsetof(Unit, reboot_arg) Service.FailureAction, config_parse_emergency_action, 0, offsetof(Service, emergency_action) Service.Type, config_parse_service_type, 0, offsetof(Service, type) Service.Restart, config_parse_service_restart, 0, offsetof(Service, restart) @@ -272,8 +272,8 @@ Service.FileDescriptorStoreMax, config_parse_unsigned, 0, Service.NotifyAccess, config_parse_notify_access, 0, offsetof(Service, notify_access) Service.Sockets, config_parse_service_sockets, 0, 0 Service.BusPolicy, config_parse_warn_compat, DISABLED_LEGACY, 0 -Service.USBFunctionDescriptors, config_parse_path, 0, offsetof(Service, usb_function_descriptors) -Service.USBFunctionStrings, config_parse_path, 0, offsetof(Service, usb_function_strings) +Service.USBFunctionDescriptors, config_parse_unit_path_printf, 0, offsetof(Service, usb_function_descriptors) +Service.USBFunctionStrings, config_parse_unit_path_printf, 0, offsetof(Service, usb_function_strings) EXEC_CONTEXT_CONFIG_ITEMS(Service)m4_dnl CGROUP_CONTEXT_CONFIG_ITEMS(Service)m4_dnl KILL_CONTEXT_CONFIG_ITEMS(Service)m4_dnl @@ -332,9 +332,9 @@ Socket.Service, config_parse_socket_service, 0, Socket.TriggerLimitIntervalSec, config_parse_sec, 0, offsetof(Socket, trigger_limit.interval) Socket.TriggerLimitBurst, config_parse_unsigned, 0, offsetof(Socket, trigger_limit.burst) m4_ifdef(`HAVE_SMACK', -`Socket.SmackLabel, config_parse_string, 0, offsetof(Socket, smack) -Socket.SmackLabelIPIn, config_parse_string, 0, offsetof(Socket, smack_ip_in) -Socket.SmackLabelIPOut, config_parse_string, 0, offsetof(Socket, smack_ip_out)', +`Socket.SmackLabel, config_parse_unit_string_printf, 0, offsetof(Socket, smack) +Socket.SmackLabelIPIn, config_parse_unit_string_printf, 0, offsetof(Socket, smack_ip_in) +Socket.SmackLabelIPOut, config_parse_unit_string_printf, 0, offsetof(Socket, smack_ip_out)', `Socket.SmackLabel, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 Socket.SmackLabelIPIn, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 Socket.SmackLabelIPOut, config_parse_warn_compat, DISABLED_CONFIGURATION, 0') @@ -354,9 +354,9 @@ BusName.AllowWorld, config_parse_bus_policy_world, 0, BusName.SELinuxContext, config_parse_exec_selinux_context, 0, 0 BusName.AcceptFileDescriptors, config_parse_bool, 0, offsetof(BusName, accept_fd) m4_dnl -Mount.What, config_parse_string, 0, offsetof(Mount, parameters_fragment.what) +Mount.What, config_parse_unit_string_printf, 0, offsetof(Mount, parameters_fragment.what) Mount.Where, config_parse_path, 0, offsetof(Mount, where) -Mount.Options, config_parse_string, 0, offsetof(Mount, parameters_fragment.options) +Mount.Options, config_parse_unit_string_printf, 0, offsetof(Mount, parameters_fragment.options) Mount.Type, config_parse_string, 0, offsetof(Mount, parameters_fragment.fstype) Mount.TimeoutSec, config_parse_sec, 0, offsetof(Mount, timeout_usec) Mount.DirectoryMode, config_parse_mode, 0, offsetof(Mount, directory_mode) @@ -373,7 +373,7 @@ Automount.TimeoutIdleSec, config_parse_sec, 0, m4_dnl Swap.What, config_parse_path, 0, offsetof(Swap, parameters_fragment.what) Swap.Priority, config_parse_int, 0, offsetof(Swap, parameters_fragment.priority) -Swap.Options, config_parse_string, 0, offsetof(Swap, parameters_fragment.options) +Swap.Options, config_parse_unit_string_printf, 0, offsetof(Swap, parameters_fragment.options) Swap.TimeoutSec, config_parse_sec, 0, offsetof(Swap, timeout_usec) EXEC_CONTEXT_CONFIG_ITEMS(Swap)m4_dnl CGROUP_CONTEXT_CONFIG_ITEMS(Swap)m4_dnl From 744bb5b1bea4d04363f7894e86701efdd75b8acb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 19:40:13 +0100 Subject: [PATCH 06/11] core: add specifier expansion to RequiresMountsFor= This might be useful for some people, for example to pull in mounts for paths including the machine ID or hostname. --- src/core/load-fragment.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 3ffb417f97..85bac9ea5b 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2565,7 +2565,7 @@ int config_parse_unit_requires_mounts_for( assert(data); for (p = rvalue;; ) { - _cleanup_free_ char *word = NULL; + _cleanup_free_ char *word = NULL, *resolved = NULL; r = extract_first_word(&p, &word, NULL, EXTRACT_QUOTES); if (r == 0) @@ -2583,9 +2583,15 @@ int config_parse_unit_requires_mounts_for( continue; } - r = unit_require_mounts_for(u, word); + r = unit_full_printf(u, word, &resolved); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to add required mount \"%s\", ignoring: %m", word); + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit name \"%s\", ignoring: %m", word); + continue; + } + + r = unit_require_mounts_for(u, resolved); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to add required mount \"%s\", ignoring: %m", resolved); continue; } } From 7b07e99320586fa3baf3e6cbb374f06c6ddc47d8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 19:42:25 +0100 Subject: [PATCH 07/11] core: add specifier expansion to ReadOnlyPaths= and friends Expanding specifiers here definitely makes sense. Also simplifies the loop a bit, as there's no reason to keep "prev" around... --- src/core/load-fragment.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 85bac9ea5b..6b36b2fc3a 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3828,8 +3828,8 @@ int config_parse_namespace_path_strv( void *data, void *userdata) { + Unit *u = userdata; char*** sv = data; - const char *prev; const char *cur; int r; @@ -3844,10 +3844,10 @@ int config_parse_namespace_path_strv( return 0; } - prev = cur = rvalue; + cur = rvalue; for (;;) { - _cleanup_free_ char *word = NULL; - int offset; + _cleanup_free_ char *word = NULL, *resolved = NULL, *joined = NULL; + bool ignore_enoent; r = extract_first_word(&cur, &word, NULL, EXTRACT_QUOTES); if (r == 0) @@ -3855,31 +3855,37 @@ int config_parse_namespace_path_strv( if (r == -ENOMEM) return log_oom(); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Trailing garbage, ignoring: %s", prev); + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to extract first word, ignoring: %s", rvalue); return 0; } if (!utf8_is_valid(word)) { log_syntax_invalid_utf8(unit, LOG_ERR, filename, line, word); - prev = cur; continue; } - offset = word[0] == '-'; - if (!path_is_absolute(word + offset)) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Not an absolute path, ignoring: %s", word); - prev = cur; + ignore_enoent = word[0] == '-'; + + r = unit_full_printf(u, word + ignore_enoent, &resolved); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers in %s: %m", word); continue; } - path_kill_slashes(word + offset); + if (!path_is_absolute(resolved)) { + log_syntax(unit, LOG_ERR, filename, line, 0, "Not an absolute path, ignoring: %s", resolved); + continue; + } - r = strv_push(sv, word); + path_kill_slashes(resolved); + + joined = strjoin(ignore_enoent ? "-" : "", resolved); + + r = strv_push(sv, joined); if (r < 0) return log_oom(); - prev = cur; - word = NULL; + joined = NULL; } return 0; From ea9cfad1d7bb7a87c0cf625e24ad93fde95e1aeb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 20:12:46 +0100 Subject: [PATCH 08/11] tests: let's make function tables static/const --- src/test/test-execute.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/test-execute.c b/src/test/test-execute.c index b2ea358b8c..4670458ffb 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -409,8 +409,8 @@ static void test_exec_spec_interpolation(Manager *m) { test(m, "exec-spec-interpolation.service", 0, CLD_EXITED); } -static int run_tests(UnitFileScope scope, test_function_t *tests) { - test_function_t *test = NULL; +static int run_tests(UnitFileScope scope, const test_function_t *tests) { + const test_function_t *test = NULL; Manager *m = NULL; int r; @@ -433,7 +433,7 @@ static int run_tests(UnitFileScope scope, test_function_t *tests) { } int main(int argc, char *argv[]) { - test_function_t user_tests[] = { + static const test_function_t user_tests[] = { test_exec_workingdirectory, test_exec_personality, test_exec_ignoresigpipe, @@ -464,7 +464,7 @@ int main(int argc, char *argv[]) { test_exec_spec_interpolation, NULL, }; - test_function_t system_tests[] = { + static const test_function_t system_tests[] = { test_exec_systemcall_system_mode_with_user, NULL, }; From 6a9cd374e02bcec5adf1918ba07e881cf87e9236 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Dec 2016 20:14:13 +0100 Subject: [PATCH 09/11] update TODO --- TODO | 2 -- 1 file changed, 2 deletions(-) diff --git a/TODO b/TODO index 74183b5f47..d89242ec6e 100644 --- a/TODO +++ b/TODO @@ -118,8 +118,6 @@ Features: * journald: sigbus API via a signal-handler safe function that people may call from the SIGBUS handler -* move specifier expansion from service_spawn() into load-fragment.c - * optionally, also require WATCHDOG=1 notifications during service start-up and shutdown * resolved: when routing queries, make sure only look for the *longest* suffix... From 1b89b0c499cd4bf0ff389caab4ecaae6e75f9d4e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 7 Dec 2016 18:58:09 +0100 Subject: [PATCH 10/11] core: deprecate %c, %r, %R specifiers %c and %r rely on settings made in the unit files themselves and hence resolve to different values depending on whether they are used before or after Slice=. Let's simply deprecate them and drop them from the documentation, as that's not really possible to fix. Moreover they are actually redundant, as the same information may always be queried from /proc/self/cgroup and /proc/1/cgroup. (Accurately speaking, %R is actually not broken like this as it is constant. However, let's remove all cgroup-related specifiers at once, as it is also redundant, and doesn't really make much sense alone.) --- man/systemd.unit.xml | 15 --------------- src/core/unit-printf.c | 16 +++++++++++++--- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index a767b2e80a..dbb0dc7bd7 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -1246,21 +1246,6 @@ This is either the unescaped instance name (if applicable) with / prepended (if applicable), or the unescaped prefix name prepended with /. - %c - Control group path of the unit - This path does not include the /sys/fs/cgroup/systemd/ prefix. - - - %r - Control group path of the slice the unit is placed in - This usually maps to the parent control group path of %c. - - - %R - Root control group path below which slices and units are placed - For system instances, this resolves to /, except in containers, where this maps to the container's root control group path. - - %t Runtime directory This is either /run (for the system manager) or the path $XDG_RUNTIME_DIR resolves to (for user managers). diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index 4c95127c80..8f7eb84c61 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -78,12 +78,18 @@ static int specifier_filename(char specifier, void *data, void *userdata, char * return unit_name_to_path(u->id, ret); } +static void bad_specifier(Unit *u, char specifier) { + log_unit_warning(u, "Specifier '%%%c' used in unit configuration, which is deprecated. Please update your unit file, as it does not work as intended.", specifier); +} + static int specifier_cgroup(char specifier, void *data, void *userdata, char **ret) { Unit *u = userdata; char *n; assert(u); + bad_specifier(u, specifier); + if (u->cgroup_path) n = strdup(u->cgroup_path); else @@ -101,6 +107,8 @@ static int specifier_cgroup_root(char specifier, void *data, void *userdata, cha assert(u); + bad_specifier(u, specifier); + n = strdup(u->manager->cgroup_root); if (!n) return -ENOMEM; @@ -115,6 +123,8 @@ static int specifier_cgroup_slice(char specifier, void *data, void *userdata, ch assert(u); + bad_specifier(u, specifier); + if (UNIT_ISSET(u->slice)) { Unit *slice; @@ -238,9 +248,9 @@ int unit_full_printf(Unit *u, const char *format, char **ret) { * (which are likely not suitable for unescaped inclusion in unit names): * * %f: the unescaped instance if set, otherwise the id unescaped as path - * %c: cgroup path of unit - * %r: where units in this slice are placed in the cgroup tree - * %R: the root of this systemd's instance tree + * %c: cgroup path of unit (deprecated) + * %r: where units in this slice are placed in the cgroup tree (deprecated) + * %R: the root of this systemd's instance tree (deprecated) * %t: the runtime directory to place sockets in (e.g. "/run" or $XDG_RUNTIME_DIR) * * %h: the homedir of the running user From 03fc9c723cfc59467a7fccc305f34273f8564b25 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 7 Dec 2016 19:03:22 +0100 Subject: [PATCH 11/11] core: add a note clarifying that we should be careful when adding new specifiers --- src/core/unit-printf.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index 8f7eb84c61..746e1a46ef 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -257,6 +257,10 @@ int unit_full_printf(Unit *u, const char *format, char **ret) { * %s: the shell of the running user * * %v: `uname -r` of the running system + * + * NOTICE: When you add new entries here, please be careful: specifiers which depend on settings of the unit + * file itself are broken by design, as they would resolve differently depending on whether they are used + * before or after the relevant configuration setting. Hence: don't add them. */ const Specifier table[] = {