From 89711996b3f561522508306e0b5ecf34f6016638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 12:35:08 -0500 Subject: [PATCH 01/29] basic/util: move execute_directory() to separate file It's a fairly specialized function. Let's make new files for it and the tests. --- .gitignore | 1 + Makefile.am | 9 ++ src/basic/exec-util.c | 181 ++++++++++++++++++++++++++++++++++++++ src/basic/exec-util.h | 22 +++++ src/basic/util.c | 143 ------------------------------ src/basic/util.h | 2 - src/core/manager.c | 1 + src/core/shutdown.c | 1 + src/sleep/sleep.c | 1 + src/test/test-exec-util.c | 87 ++++++++++++++++++ src/test/test-util.c | 45 ---------- 11 files changed, 303 insertions(+), 190 deletions(-) create mode 100644 src/basic/exec-util.c create mode 100644 src/basic/exec-util.h create mode 100644 src/test/test-exec-util.c diff --git a/.gitignore b/.gitignore index fe7859c265..7b5bb41259 100644 --- a/.gitignore +++ b/.gitignore @@ -194,6 +194,7 @@ /test-env-util /test-escape /test-event +/test-exec-util /test-execute /test-extract-word /test-fd-util diff --git a/Makefile.am b/Makefile.am index 2d913bd7d7..003ec9bfb7 100644 --- a/Makefile.am +++ b/Makefile.am @@ -873,6 +873,8 @@ libbasic_la_SOURCES = \ src/basic/bus-label.h \ src/basic/ratelimit.h \ src/basic/ratelimit.c \ + src/basic/exec-util.c \ + src/basic/exec-util.h \ src/basic/exit-status.c \ src/basic/exit-status.h \ src/basic/virt.c \ @@ -1528,6 +1530,7 @@ tests += \ test-ellipsize \ test-util \ test-mount-util \ + test-exec-util \ test-cpu-set-util \ test-hexdecoct \ test-escape \ @@ -1912,6 +1915,12 @@ test_mount_util_SOURCES = \ test_mount_util_LDADD = \ libsystemd-shared.la +test_exec_util_SOURCES = \ + src/test/test-exec-util.c + +test_exec_util_LDADD = \ + libsystemd-shared.la + test_hexdecoct_SOURCES = \ src/test/test-hexdecoct.c diff --git a/src/basic/exec-util.c b/src/basic/exec-util.c new file mode 100644 index 0000000000..757cd3b4ff --- /dev/null +++ b/src/basic/exec-util.c @@ -0,0 +1,181 @@ +/*** + This file is part of systemd. + + Copyright 2010 Lennart Poettering + + systemd is free software; you can redistribute it and/or modify it + under the terms of the GNU Lesser General Public License as published by + the Free Software Foundation; either version 2.1 of the License, or + (at your option) any later version. + + systemd is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with systemd; If not, see . +***/ + +#include +#include +#include +#include +#include + +#include "alloc-util.h" +#include "dirent-util.h" +#include "exec-util.h" +#include "fd-util.h" +#include "hashmap.h" +#include "macro.h" +#include "process-util.h" +#include "set.h" +#include "signal-util.h" +#include "stat-util.h" +#include "string-util.h" +#include "strv.h" +#include "util.h" + +/* Put this test here for a lack of better place */ +assert_cc(EAGAIN == EWOULDBLOCK); + +static int do_execute(char **directories, usec_t timeout, char *argv[]) { + _cleanup_hashmap_free_free_ Hashmap *pids = NULL; + _cleanup_set_free_free_ Set *seen = NULL; + char **directory; + + /* We fork this all off from a child process so that we can + * somewhat cleanly make use of SIGALRM to set a time limit */ + + (void) reset_all_signal_handlers(); + (void) reset_signal_mask(); + + assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0); + + pids = hashmap_new(NULL); + if (!pids) + return log_oom(); + + seen = set_new(&string_hash_ops); + if (!seen) + return log_oom(); + + STRV_FOREACH(directory, directories) { + _cleanup_closedir_ DIR *d; + struct dirent *de; + + d = opendir(*directory); + if (!d) { + if (errno == ENOENT) + continue; + + return log_error_errno(errno, "Failed to open directory %s: %m", *directory); + } + + FOREACH_DIRENT(de, d, break) { + _cleanup_free_ char *path = NULL; + pid_t pid; + int r; + + if (!dirent_is_file(de)) + continue; + + if (set_contains(seen, de->d_name)) { + log_debug("%1$s/%2$s skipped (%2$s was already seen).", *directory, de->d_name); + continue; + } + + r = set_put_strdup(seen, de->d_name); + if (r < 0) + return log_oom(); + + path = strjoin(*directory, "/", de->d_name); + if (!path) + return log_oom(); + + if (null_or_empty_path(path)) { + log_debug("%s is empty (a mask).", path); + continue; + } + + pid = fork(); + if (pid < 0) { + log_error_errno(errno, "Failed to fork: %m"); + continue; + } else if (pid == 0) { + char *_argv[2]; + + assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0); + + if (!argv) { + _argv[0] = path; + _argv[1] = NULL; + argv = _argv; + } else + argv[0] = path; + + execv(path, argv); + return log_error_errno(errno, "Failed to execute %s: %m", path); + } + + log_debug("Spawned %s as " PID_FMT ".", path, pid); + + r = hashmap_put(pids, PID_TO_PTR(pid), path); + if (r < 0) + return log_oom(); + path = NULL; + } + } + + /* Abort execution of this process after the timout. We simply + * rely on SIGALRM as default action terminating the process, + * and turn on alarm(). */ + + if (timeout != USEC_INFINITY) + alarm((timeout + USEC_PER_SEC - 1) / USEC_PER_SEC); + + while (!hashmap_isempty(pids)) { + _cleanup_free_ char *path = NULL; + pid_t pid; + + pid = PTR_TO_PID(hashmap_first_key(pids)); + assert(pid > 0); + + path = hashmap_remove(pids, PID_TO_PTR(pid)); + assert(path); + + wait_for_terminate_and_warn(path, pid, true); + } + + return 0; +} + +void execute_directories(const char* const* directories, usec_t timeout, char *argv[]) { + pid_t executor_pid; + int r; + char *name; + char **dirs = (char**) directories; + + assert(!strv_isempty(dirs)); + + name = basename(dirs[0]); + assert(!isempty(name)); + + /* Executes all binaries in the directories in parallel and waits + * for them to finish. Optionally a timeout is applied. If a file + * with the same name exists in more than one directory, the + * earliest one wins. */ + + executor_pid = fork(); + if (executor_pid < 0) { + log_error_errno(errno, "Failed to fork: %m"); + return; + + } else if (executor_pid == 0) { + r = do_execute(dirs, timeout, argv); + _exit(r < 0 ? EXIT_FAILURE : EXIT_SUCCESS); + } + + wait_for_terminate_and_warn(name, executor_pid, true); +} diff --git a/src/basic/exec-util.h b/src/basic/exec-util.h new file mode 100644 index 0000000000..9f8daa9fc8 --- /dev/null +++ b/src/basic/exec-util.h @@ -0,0 +1,22 @@ +/*** + This file is part of systemd. + + Copyright 2017 Zbigniew Jędrzejewski-Szmek + + systemd is free software; you can redistribute it and/or modify it + under the terms of the GNU Lesser General Public License as published by + the Free Software Foundation; either version 2.1 of the License, or + (at your option) any later version. + + systemd is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with systemd; If not, see . +***/ + +#include "time-util.h" + +void execute_directories(const char* const* directories, usec_t timeout, char *argv[]); diff --git a/src/basic/util.c b/src/basic/util.c index 6204906f37..3dce0ea92e 100644 --- a/src/basic/util.c +++ b/src/basic/util.c @@ -59,9 +59,6 @@ #include "user-util.h" #include "util.h" -/* Put this test here for a lack of better place */ -assert_cc(EAGAIN == EWOULDBLOCK); - int saved_argc = 0; char **saved_argv = NULL; static int saved_in_initrd = -1; @@ -80,146 +77,6 @@ size_t page_size(void) { return pgsz; } -static int do_execute(char **directories, usec_t timeout, char *argv[]) { - _cleanup_hashmap_free_free_ Hashmap *pids = NULL; - _cleanup_set_free_free_ Set *seen = NULL; - char **directory; - - /* We fork this all off from a child process so that we can - * somewhat cleanly make use of SIGALRM to set a time limit */ - - (void) reset_all_signal_handlers(); - (void) reset_signal_mask(); - - assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0); - - pids = hashmap_new(NULL); - if (!pids) - return log_oom(); - - seen = set_new(&string_hash_ops); - if (!seen) - return log_oom(); - - STRV_FOREACH(directory, directories) { - _cleanup_closedir_ DIR *d; - struct dirent *de; - - d = opendir(*directory); - if (!d) { - if (errno == ENOENT) - continue; - - return log_error_errno(errno, "Failed to open directory %s: %m", *directory); - } - - FOREACH_DIRENT(de, d, break) { - _cleanup_free_ char *path = NULL; - pid_t pid; - int r; - - if (!dirent_is_file(de)) - continue; - - if (set_contains(seen, de->d_name)) { - log_debug("%1$s/%2$s skipped (%2$s was already seen).", *directory, de->d_name); - continue; - } - - r = set_put_strdup(seen, de->d_name); - if (r < 0) - return log_oom(); - - path = strjoin(*directory, "/", de->d_name); - if (!path) - return log_oom(); - - if (null_or_empty_path(path)) { - log_debug("%s is empty (a mask).", path); - continue; - } - - pid = fork(); - if (pid < 0) { - log_error_errno(errno, "Failed to fork: %m"); - continue; - } else if (pid == 0) { - char *_argv[2]; - - assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0); - - if (!argv) { - _argv[0] = path; - _argv[1] = NULL; - argv = _argv; - } else - argv[0] = path; - - execv(path, argv); - return log_error_errno(errno, "Failed to execute %s: %m", path); - } - - log_debug("Spawned %s as " PID_FMT ".", path, pid); - - r = hashmap_put(pids, PID_TO_PTR(pid), path); - if (r < 0) - return log_oom(); - path = NULL; - } - } - - /* Abort execution of this process after the timout. We simply - * rely on SIGALRM as default action terminating the process, - * and turn on alarm(). */ - - if (timeout != USEC_INFINITY) - alarm((timeout + USEC_PER_SEC - 1) / USEC_PER_SEC); - - while (!hashmap_isempty(pids)) { - _cleanup_free_ char *path = NULL; - pid_t pid; - - pid = PTR_TO_PID(hashmap_first_key(pids)); - assert(pid > 0); - - path = hashmap_remove(pids, PID_TO_PTR(pid)); - assert(path); - - wait_for_terminate_and_warn(path, pid, true); - } - - return 0; -} - -void execute_directories(const char* const* directories, usec_t timeout, char *argv[]) { - pid_t executor_pid; - int r; - char *name; - char **dirs = (char**) directories; - - assert(!strv_isempty(dirs)); - - name = basename(dirs[0]); - assert(!isempty(name)); - - /* Executes all binaries in the directories in parallel and waits - * for them to finish. Optionally a timeout is applied. If a file - * with the same name exists in more than one directory, the - * earliest one wins. */ - - executor_pid = fork(); - if (executor_pid < 0) { - log_error_errno(errno, "Failed to fork: %m"); - return; - - } else if (executor_pid == 0) { - r = do_execute(dirs, timeout, argv); - _exit(r < 0 ? EXIT_FAILURE : EXIT_SUCCESS); - } - - wait_for_terminate_and_warn(name, executor_pid, true); -} - bool plymouth_running(void) { return access("/run/plymouth/pid", F_OK) >= 0; } diff --git a/src/basic/util.h b/src/basic/util.h index c3802a811c..c7da6c39bf 100644 --- a/src/basic/util.h +++ b/src/basic/util.h @@ -65,8 +65,6 @@ static inline const char* enable_disable(bool b) { return b ? "enable" : "disable"; } -void execute_directories(const char* const* directories, usec_t timeout, char *argv[]); - bool plymouth_running(void); bool display_is_local(const char *display) _pure_; diff --git a/src/core/manager.c b/src/core/manager.c index e4da945777..0884534cc4 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -52,6 +52,7 @@ #include "dirent-util.h" #include "env-util.h" #include "escape.h" +#include "exec-util.h" #include "exit-status.h" #include "fd-util.h" #include "fileio.h" diff --git a/src/core/shutdown.c b/src/core/shutdown.c index a795d875bb..56a035e234 100644 --- a/src/core/shutdown.c +++ b/src/core/shutdown.c @@ -32,6 +32,7 @@ #include "alloc-util.h" #include "cgroup-util.h" #include "def.h" +#include "exec-util.h" #include "fileio.h" #include "killall.h" #include "log.h" diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index c8f0742183..b0f992fc9c 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -25,6 +25,7 @@ #include "sd-messages.h" #include "def.h" +#include "exec-util.h" #include "fd-util.h" #include "fileio.h" #include "log.h" diff --git a/src/test/test-exec-util.c b/src/test/test-exec-util.c new file mode 100644 index 0000000000..26533f0bf6 --- /dev/null +++ b/src/test/test-exec-util.c @@ -0,0 +1,87 @@ +/*** + This file is part of systemd. + + Copyright 2010 Lennart Poettering + Copyright 2013 Thomas H.P. Andersen + + systemd is free software; you can redistribute it and/or modify it + under the terms of the GNU Lesser General Public License as published by + the Free Software Foundation; either version 2.1 of the License, or + (at your option) any later version. + + systemd is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with systemd; If not, see . +***/ + +#include +#include +#include +#include +#include + +#include "def.h" +#include "exec-util.h" +#include "fileio.h" +#include "fs-util.h" +#include "log.h" +#include "macro.h" +#include "rm-rf.h" +#include "string-util.h" + +static void test_execute_directory(void) { + char template_lo[] = "/tmp/test-readlink_and_make_absolute-lo.XXXXXXX"; + char template_hi[] = "/tmp/test-readlink_and_make_absolute-hi.XXXXXXX"; + const char * dirs[] = {template_hi, template_lo, NULL}; + const char *name, *name2, *name3, *overridden, *override, *masked, *mask; + + assert_se(mkdtemp(template_lo)); + assert_se(mkdtemp(template_hi)); + + name = strjoina(template_lo, "/script"); + name2 = strjoina(template_hi, "/script2"); + name3 = strjoina(template_lo, "/useless"); + overridden = strjoina(template_lo, "/overridden"); + override = strjoina(template_hi, "/overridden"); + masked = strjoina(template_lo, "/masked"); + mask = strjoina(template_hi, "/masked"); + + assert_se(write_string_file(name, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/it_works", WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(name2, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/it_works2", WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(overridden, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/failed", WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(override, "#!/bin/sh\necho 'Executing '$0", WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(masked, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/failed", WRITE_STRING_FILE_CREATE) == 0); + assert_se(symlink("/dev/null", mask) == 0); + assert_se(chmod(name, 0755) == 0); + assert_se(chmod(name2, 0755) == 0); + assert_se(chmod(overridden, 0755) == 0); + assert_se(chmod(override, 0755) == 0); + assert_se(chmod(masked, 0755) == 0); + assert_se(touch(name3) >= 0); + + execute_directories(dirs, DEFAULT_TIMEOUT_USEC, NULL); + + assert_se(chdir(template_lo) == 0); + assert_se(access("it_works", F_OK) >= 0); + assert_se(access("failed", F_OK) < 0); + + assert_se(chdir(template_hi) == 0); + assert_se(access("it_works2", F_OK) >= 0); + assert_se(access("failed", F_OK) < 0); + + (void) rm_rf(template_lo, REMOVE_ROOT|REMOVE_PHYSICAL); + (void) rm_rf(template_hi, REMOVE_ROOT|REMOVE_PHYSICAL); +} + +int main(int argc, char *argv[]) { + log_parse_environment(); + log_open(); + + test_execute_directory(); + + return 0; +} diff --git a/src/test/test-util.c b/src/test/test-util.c index 1b5cba86c1..f8bf0cb875 100644 --- a/src/test/test-util.c +++ b/src/test/test-util.c @@ -195,50 +195,6 @@ static void test_log2i(void) { assert_se(log2i(INT_MAX) == sizeof(int)*8-2); } -static void test_execute_directory(void) { - char template_lo[] = "/tmp/test-readlink_and_make_absolute-lo.XXXXXXX"; - char template_hi[] = "/tmp/test-readlink_and_make_absolute-hi.XXXXXXX"; - const char * dirs[] = {template_hi, template_lo, NULL}; - const char *name, *name2, *name3, *overridden, *override, *masked, *mask; - - assert_se(mkdtemp(template_lo)); - assert_se(mkdtemp(template_hi)); - - name = strjoina(template_lo, "/script"); - name2 = strjoina(template_hi, "/script2"); - name3 = strjoina(template_lo, "/useless"); - overridden = strjoina(template_lo, "/overridden"); - override = strjoina(template_hi, "/overridden"); - masked = strjoina(template_lo, "/masked"); - mask = strjoina(template_hi, "/masked"); - - assert_se(write_string_file(name, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/it_works", WRITE_STRING_FILE_CREATE) == 0); - assert_se(write_string_file(name2, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/it_works2", WRITE_STRING_FILE_CREATE) == 0); - assert_se(write_string_file(overridden, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/failed", WRITE_STRING_FILE_CREATE) == 0); - assert_se(write_string_file(override, "#!/bin/sh\necho 'Executing '$0", WRITE_STRING_FILE_CREATE) == 0); - assert_se(write_string_file(masked, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/failed", WRITE_STRING_FILE_CREATE) == 0); - assert_se(symlink("/dev/null", mask) == 0); - assert_se(chmod(name, 0755) == 0); - assert_se(chmod(name2, 0755) == 0); - assert_se(chmod(overridden, 0755) == 0); - assert_se(chmod(override, 0755) == 0); - assert_se(chmod(masked, 0755) == 0); - assert_se(touch(name3) >= 0); - - execute_directories(dirs, DEFAULT_TIMEOUT_USEC, NULL); - - assert_se(chdir(template_lo) == 0); - assert_se(access("it_works", F_OK) >= 0); - assert_se(access("failed", F_OK) < 0); - - assert_se(chdir(template_hi) == 0); - assert_se(access("it_works2", F_OK) >= 0); - assert_se(access("failed", F_OK) < 0); - - (void) rm_rf(template_lo, REMOVE_ROOT|REMOVE_PHYSICAL); - (void) rm_rf(template_hi, REMOVE_ROOT|REMOVE_PHYSICAL); -} - static void test_raw_clone(void) { pid_t parent, pid, pid2; @@ -359,7 +315,6 @@ int main(int argc, char *argv[]) { test_protect_errno(); test_in_set(); test_log2i(); - test_execute_directory(); test_raw_clone(); test_physical_memory(); test_physical_memory_scale(); From cf55fc180783e5350ec243daf281200c6f6a3191 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 13:06:27 -0500 Subject: [PATCH 02/29] basic/exec-util: split out actual execution to a different function This corrects an error in error handling: if execution fails, we should never use return, but immediately _exit(). --- src/basic/exec-util.c | 60 +++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/src/basic/exec-util.c b/src/basic/exec-util.c index 757cd3b4ff..23dcac3274 100644 --- a/src/basic/exec-util.c +++ b/src/basic/exec-util.c @@ -40,6 +40,39 @@ /* Put this test here for a lack of better place */ assert_cc(EAGAIN == EWOULDBLOCK); +static int do_spawn(const char *path, char *argv[], pid_t *pid) { + pid_t _pid; + + if (null_or_empty_path(path)) { + log_debug("%s is empty (a mask).", path); + return 0; + } + + _pid = fork(); + if (_pid < 0) + return log_error_errno(errno, "Failed to fork: %m"); + if (_pid == 0) { + char *_argv[2]; + + assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0); + + if (!argv) { + _argv[0] = (char*) path; + _argv[1] = NULL; + argv = _argv; + } else + argv[0] = (char*) path; + + execv(path, argv); + log_error_errno(errno, "Failed to execute %s: %m", path); + _exit(EXIT_FAILURE); + } + + log_debug("Spawned %s as " PID_FMT ".", path, _pid); + *pid = _pid; + return 1; +} + static int do_execute(char **directories, usec_t timeout, char *argv[]) { _cleanup_hashmap_free_free_ Hashmap *pids = NULL; _cleanup_set_free_free_ Set *seen = NULL; @@ -94,32 +127,9 @@ static int do_execute(char **directories, usec_t timeout, char *argv[]) { if (!path) return log_oom(); - if (null_or_empty_path(path)) { - log_debug("%s is empty (a mask).", path); + r = do_spawn(path, argv, &pid); + if (r <= 0) continue; - } - - pid = fork(); - if (pid < 0) { - log_error_errno(errno, "Failed to fork: %m"); - continue; - } else if (pid == 0) { - char *_argv[2]; - - assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0); - - if (!argv) { - _argv[0] = path; - _argv[1] = NULL; - argv = _argv; - } else - argv[0] = path; - - execv(path, argv); - return log_error_errno(errno, "Failed to execute %s: %m", path); - } - - log_debug("Spawned %s as " PID_FMT ".", path, pid); r = hashmap_put(pids, PID_TO_PTR(pid), path); if (r < 0) From 4e281f68eaec5592c4de9b0974f42ece71cd6f80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 14:16:19 -0500 Subject: [PATCH 03/29] basic/conf-files: extend conf_files_list() to list unsuffixed files 5dd11ab5f36ce71138005 did a similar change for conf_files_list_strv(). Here we do the same for conf_files_list() and conf_files_list_nulstr(). No change for existing users. Tests are added. --- src/basic/conf-files.c | 2 -- src/test/test-conf-files.c | 24 ++++++++++++++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/basic/conf-files.c b/src/basic/conf-files.c index b5780194df..b8f0f5d03d 100644 --- a/src/basic/conf-files.c +++ b/src/basic/conf-files.c @@ -137,7 +137,6 @@ int conf_files_list(char ***strv, const char *suffix, const char *root, const ch va_list ap; assert(strv); - assert(suffix); va_start(ap, dir); dirs = strv_new_ap(dir, ap); @@ -153,7 +152,6 @@ int conf_files_list_nulstr(char ***strv, const char *suffix, const char *root, c _cleanup_strv_free_ char **dirs = NULL; assert(strv); - assert(suffix); dirs = strv_split_nulstr(d); if (!dirs) diff --git a/src/test/test-conf-files.c b/src/test/test-conf-files.c index 03b3a9fa5c..22b7c61204 100644 --- a/src/test/test-conf-files.c +++ b/src/test/test-conf-files.c @@ -47,13 +47,16 @@ static void setup_test_dir(char *tmp_dir, const char *files, ...) { static void test_conf_files_list(bool use_root) { char tmp_dir[] = "/tmp/test-conf-files-XXXXXX"; - _cleanup_strv_free_ char **found_files = NULL; - const char *root_dir, *search_1, *search_2, *expect_a, *expect_b; + _cleanup_strv_free_ char **found_files = NULL, **found_files2 = NULL; + const char *root_dir, *search_1, *search_2, *expect_a, *expect_b, *expect_c; + + log_debug("/* %s */", __func__); setup_test_dir(tmp_dir, "/dir1/a.conf", "/dir2/a.conf", "/dir2/b.conf", + "/dir2/c.foo", NULL); if (use_root) { @@ -68,6 +71,9 @@ static void test_conf_files_list(bool use_root) { expect_a = strjoina(tmp_dir, "/dir1/a.conf"); expect_b = strjoina(tmp_dir, "/dir2/b.conf"); + expect_c = strjoina(tmp_dir, "/dir2/c.foo"); + + log_debug("/* Check when filtered by suffix */"); assert_se(conf_files_list(&found_files, ".conf", root_dir, search_1, search_2, NULL) == 0); strv_print(found_files); @@ -77,10 +83,24 @@ static void test_conf_files_list(bool use_root) { assert_se(streq_ptr(found_files[1], expect_b)); assert_se(found_files[2] == NULL); + log_debug("/* Check when unfiltered */"); + assert_se(conf_files_list(&found_files2, NULL, root_dir, search_1, search_2, NULL) == 0); + strv_print(found_files2); + + assert_se(found_files2); + assert_se(streq_ptr(found_files2[0], expect_a)); + assert_se(streq_ptr(found_files2[1], expect_b)); + assert_se(streq_ptr(found_files2[2], expect_c)); + assert_se(found_files2[3] == NULL); + assert_se(rm_rf(tmp_dir, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); } int main(int argc, char **argv) { + log_set_max_level(LOG_DEBUG); + log_parse_environment(); + log_open(); + test_conf_files_list(false); test_conf_files_list(true); return 0; From 2e4cfe65b87075d79f261e0fb3dd6bac1290b06d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 14:44:34 -0500 Subject: [PATCH 04/29] basic/exec-util: use conf_files_list_strv to list executables Essentially the same logic as in conf_files_list() was independently implemented in do_execute(). With previous commit, do_execute() can just call conf_files_list() to get a list of executable paths. --- src/basic/exec-util.c | 75 +++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 50 deletions(-) diff --git a/src/basic/exec-util.c b/src/basic/exec-util.c index 23dcac3274..46eafc7b90 100644 --- a/src/basic/exec-util.c +++ b/src/basic/exec-util.c @@ -24,9 +24,8 @@ #include #include "alloc-util.h" -#include "dirent-util.h" +#include "conf-files.h" #include "exec-util.h" -#include "fd-util.h" #include "hashmap.h" #include "macro.h" #include "process-util.h" @@ -75,8 +74,9 @@ static int do_spawn(const char *path, char *argv[], pid_t *pid) { static int do_execute(char **directories, usec_t timeout, char *argv[]) { _cleanup_hashmap_free_free_ Hashmap *pids = NULL; - _cleanup_set_free_free_ Set *seen = NULL; - char **directory; + _cleanup_strv_free_ char **paths = NULL; + char **path; + int r; /* We fork this all off from a child process so that we can * somewhat cleanly make use of SIGALRM to set a time limit */ @@ -86,56 +86,31 @@ static int do_execute(char **directories, usec_t timeout, char *argv[]) { assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0); + r = conf_files_list_strv(&paths, NULL, NULL, (const char* const*) directories); + if (r < 0) + return r; + pids = hashmap_new(NULL); if (!pids) return log_oom(); - seen = set_new(&string_hash_ops); - if (!seen) - return log_oom(); + STRV_FOREACH(path, paths) { + _cleanup_free_ char *t = NULL; + pid_t pid; - STRV_FOREACH(directory, directories) { - _cleanup_closedir_ DIR *d; - struct dirent *de; + t = strdup(*path); + if (!t) + return log_oom(); - d = opendir(*directory); - if (!d) { - if (errno == ENOENT) - continue; + r = do_spawn(t, argv, &pid); + if (r <= 0) + continue; - return log_error_errno(errno, "Failed to open directory %s: %m", *directory); - } + r = hashmap_put(pids, PID_TO_PTR(pid), t); + if (r < 0) + return log_oom(); - FOREACH_DIRENT(de, d, break) { - _cleanup_free_ char *path = NULL; - pid_t pid; - int r; - - if (!dirent_is_file(de)) - continue; - - if (set_contains(seen, de->d_name)) { - log_debug("%1$s/%2$s skipped (%2$s was already seen).", *directory, de->d_name); - continue; - } - - r = set_put_strdup(seen, de->d_name); - if (r < 0) - return log_oom(); - - path = strjoin(*directory, "/", de->d_name); - if (!path) - return log_oom(); - - r = do_spawn(path, argv, &pid); - if (r <= 0) - continue; - - r = hashmap_put(pids, PID_TO_PTR(pid), path); - if (r < 0) - return log_oom(); - path = NULL; - } + t = NULL; } /* Abort execution of this process after the timout. We simply @@ -146,16 +121,16 @@ static int do_execute(char **directories, usec_t timeout, char *argv[]) { alarm((timeout + USEC_PER_SEC - 1) / USEC_PER_SEC); while (!hashmap_isempty(pids)) { - _cleanup_free_ char *path = NULL; + _cleanup_free_ char *t = NULL; pid_t pid; pid = PTR_TO_PID(hashmap_first_key(pids)); assert(pid > 0); - path = hashmap_remove(pids, PID_TO_PTR(pid)); - assert(path); + t = hashmap_remove(pids, PID_TO_PTR(pid)); + assert(t); - wait_for_terminate_and_warn(path, pid, true); + wait_for_terminate_and_warn(t, pid, true); } return 0; From 3a7928957bc730d9b7edb5161e3f17060debdb63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 16:38:01 -0500 Subject: [PATCH 05/29] basic/def: indentation --- src/basic/def.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/basic/def.h b/src/basic/def.h index 2266eff650..10d776ec8e 100644 --- a/src/basic/def.h +++ b/src/basic/def.h @@ -73,18 +73,18 @@ #define NOTIFY_BUFFER_MAX PIPE_BUF #ifdef HAVE_SPLIT_USR -#define _CONF_PATHS_SPLIT_USR(n) "/lib/" n "\0" +# define _CONF_PATHS_SPLIT_USR(n) "/lib/" n "\0" #else -#define _CONF_PATHS_SPLIT_USR(n) +# define _CONF_PATHS_SPLIT_USR(n) #endif /* Return a nulstr for a standard cascade of configuration paths, * suitable to pass to conf_files_list_nulstr() or config_parse_many_nulstr() * to implement drop-in directories for extending configuration * files. */ -#define CONF_PATHS_NULSTR(n) \ - "/etc/" n "\0" \ - "/run/" n "\0" \ - "/usr/local/lib/" n "\0" \ - "/usr/lib/" n "\0" \ +#define CONF_PATHS_NULSTR(n) \ + "/etc/" n "\0" \ + "/run/" n "\0" \ + "/usr/local/lib/" n "\0" \ + "/usr/lib/" n "\0" \ _CONF_PATHS_SPLIT_USR(n) From 58f88d929f2b46e9470eb468f4890c1d4e8f51b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 01:35:33 -0500 Subject: [PATCH 06/29] manager: fix handling of failure in initialization We would warn and continue after failure in manager_startup, but there's no way we can continue. We must fail. --- src/core/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/main.c b/src/core/main.c index ad2ce1330e..3c6b18229c 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1830,8 +1830,10 @@ int main(int argc, char *argv[]) { before_startup = now(CLOCK_MONOTONIC); r = manager_startup(m, arg_serialization, fds); - if (r < 0) + if (r < 0) { log_error_errno(r, "Failed to fully start up daemon: %m"); + goto finish; + } /* This will close all file descriptors that were opened, but * not claimed by any unit. */ From 18f71a3c8174774c5386c4aba94d54f3b5c36a84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 16:23:24 -0500 Subject: [PATCH 07/29] basic/strv: allow NULLs to be inserted into strv All callers of this function insert non-empty strings, so there's no functional change. --- src/basic/strv.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/basic/strv.c b/src/basic/strv.c index 0eec868eed..60f92e6373 100644 --- a/src/basic/strv.c +++ b/src/basic/strv.c @@ -564,9 +564,6 @@ int strv_extend_front(char ***l, const char *value) { /* Like strv_extend(), but prepends rather than appends the new entry */ - if (!value) - return 0; - n = strv_length(*l); /* Increase and overflow check. */ @@ -574,9 +571,12 @@ int strv_extend_front(char ***l, const char *value) { if (m < n) return -ENOMEM; - v = strdup(value); - if (!v) - return -ENOMEM; + if (value) { + v = strdup(value); + if (!v) + return -ENOMEM; + } else + v = NULL; c = realloc_multiply(*l, sizeof(char*), m); if (!c) { From 504afd7c34e00eb84589e94e59cd14f2fffa2807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 11 Feb 2017 18:33:16 -0500 Subject: [PATCH 08/29] core/manager: split out creation of serialization fd out to a helper There is a slight change in behaviour: the user manager for root will create a temporary file in /run/systemd, not /tmp. I don't think this matters, but simplifies implementation. --- src/basic/fileio.c | 19 +++++++++++++++++++ src/basic/fileio.h | 1 + src/core/manager.c | 17 ++++------------- src/test/test-fd-util.c | 10 ++++++++++ 4 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index c43b0583a4..ac65fada35 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -1342,6 +1342,25 @@ int open_tmpfile_linkable(const char *target, int flags, char **ret_path) { return fd; } +int open_serialization_fd(const char *ident) { + int fd = -1; + + fd = memfd_create(ident, MFD_CLOEXEC); + if (fd < 0) { + const char *path; + + path = getpid() == 1 ? "/run/systemd" : "/tmp"; + fd = open_tmpfile_unlinkable(path, O_RDWR|O_CLOEXEC); + if (fd < 0) + return fd; + + log_debug("Serializing %s to %s.", ident, path); + } else + log_debug("Serializing %s to memfd.", ident); + + return fd; +} + int link_tmpfile(int fd, const char *path, const char *target) { assert(fd >= 0); diff --git a/src/basic/fileio.h b/src/basic/fileio.h index 17b38a5d60..64852b15a8 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -84,6 +84,7 @@ int fputs_with_space(FILE *f, const char *s, const char *separator, bool *space) int open_tmpfile_unlinkable(const char *directory, int flags); int open_tmpfile_linkable(const char *target, int flags, char **ret_path); +int open_serialization_fd(const char *ident); int link_tmpfile(int fd, const char *path, const char *target); diff --git a/src/core/manager.c b/src/core/manager.c index 0884534cc4..d432512a59 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2437,22 +2437,14 @@ void manager_send_unit_plymouth(Manager *m, Unit *u) { } int manager_open_serialization(Manager *m, FILE **_f) { - int fd = -1; + int fd; FILE *f; assert(_f); - fd = memfd_create("systemd-serialization", MFD_CLOEXEC); - if (fd < 0) { - const char *path; - - path = MANAGER_IS_SYSTEM(m) ? "/run/systemd" : "/tmp"; - fd = open_tmpfile_unlinkable(path, O_RDWR|O_CLOEXEC); - if (fd < 0) - return -errno; - log_debug("Serializing state to %s.", path); - } else - log_debug("Serializing state to memfd."); + fd = open_serialization_fd("systemd-state"); + if (fd < 0) + return fd; f = fdopen(fd, "w+"); if (!f) { @@ -2461,7 +2453,6 @@ int manager_open_serialization(Manager *m, FILE **_f) { } *_f = f; - return 0; } diff --git a/src/test/test-fd-util.c b/src/test/test-fd-util.c index f555bb976c..4425b5fe5f 100644 --- a/src/test/test-fd-util.c +++ b/src/test/test-fd-util.c @@ -94,10 +94,20 @@ static void test_same_fd(void) { assert_se(same_fd(b, a) == 0); } +static void test_open_serialization_fd(void) { + _cleanup_close_ int fd = -1; + + fd = open_serialization_fd("test"); + assert_se(fd >= 0); + + write(fd, "test\n", 5); +} + int main(int argc, char *argv[]) { test_close_many(); test_close_nointr(); test_same_fd(); + test_open_serialization_fd(); return 0; } From c6e47247a745f9245eb3dbfb29fc066ef72d3886 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 15:22:37 -0500 Subject: [PATCH 09/29] basic/exec-util: add support for synchronous (ordered) execution The output of processes can be gathered, and passed back to the callee. (This commit just implements the basic functionality and tests.) After the preparation in previous commits, the change in functionality is relatively simple. For coding convenience, alarm is prepared *before* any children are executed, and not before. This shouldn't matter usually, since just forking of the children should be pretty quick. One could also argue that this is more correct, because we will also catch the case when (for whatever reason), forking itself is slow. Three callback functions and three levels of serialization are used: - from individual generator processes to the generator forker - from the forker back to the main process - deserialization in the main process v2: - replace an structure with an indexed array of callbacks --- src/basic/exec-util.c | 147 ++++++++++++++++++++----- src/basic/exec-util.h | 18 ++- src/core/manager.c | 3 +- src/core/shutdown.c | 2 +- src/sleep/sleep.c | 4 +- src/test/test-exec-util.c | 226 +++++++++++++++++++++++++++++++++++--- 6 files changed, 351 insertions(+), 49 deletions(-) diff --git a/src/basic/exec-util.c b/src/basic/exec-util.c index 46eafc7b90..d69039c489 100644 --- a/src/basic/exec-util.c +++ b/src/basic/exec-util.c @@ -22,10 +22,14 @@ #include #include #include +#include #include "alloc-util.h" #include "conf-files.h" +#include "env-util.h" #include "exec-util.h" +#include "fd-util.h" +#include "fileio.h" #include "hashmap.h" #include "macro.h" #include "process-util.h" @@ -34,12 +38,14 @@ #include "stat-util.h" #include "string-util.h" #include "strv.h" +#include "terminal-util.h" #include "util.h" /* Put this test here for a lack of better place */ assert_cc(EAGAIN == EWOULDBLOCK); -static int do_spawn(const char *path, char *argv[], pid_t *pid) { +static int do_spawn(const char *path, char *argv[], int stdout_fd, pid_t *pid) { + pid_t _pid; if (null_or_empty_path(path)) { @@ -55,6 +61,15 @@ static int do_spawn(const char *path, char *argv[], pid_t *pid) { assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0); + if (stdout_fd >= 0) { + /* If the fd happens to be in the right place, go along with that */ + if (stdout_fd != STDOUT_FILENO && + dup2(stdout_fd, STDOUT_FILENO) < 0) + return -errno; + + fd_cloexec(STDOUT_FILENO, false); + } + if (!argv) { _argv[0] = (char*) path; _argv[1] = NULL; @@ -72,14 +87,24 @@ static int do_spawn(const char *path, char *argv[], pid_t *pid) { return 1; } -static int do_execute(char **directories, usec_t timeout, char *argv[]) { +static int do_execute( + char **directories, + usec_t timeout, + gather_stdout_callback_t const callbacks[_STDOUT_CONSUME_MAX], + void* const callback_args[_STDOUT_CONSUME_MAX], + int output_fd, + char *argv[]) { + _cleanup_hashmap_free_free_ Hashmap *pids = NULL; _cleanup_strv_free_ char **paths = NULL; char **path; int r; - /* We fork this all off from a child process so that we can - * somewhat cleanly make use of SIGALRM to set a time limit */ + /* We fork this all off from a child process so that we can somewhat cleanly make + * use of SIGALRM to set a time limit. + * + * If callbacks is nonnull, execution is serial. Otherwise, we default to parallel. + */ (void) reset_all_signal_handlers(); (void) reset_signal_mask(); @@ -90,35 +115,62 @@ static int do_execute(char **directories, usec_t timeout, char *argv[]) { if (r < 0) return r; - pids = hashmap_new(NULL); - if (!pids) - return log_oom(); + if (!callbacks) { + pids = hashmap_new(NULL); + if (!pids) + return log_oom(); + } + + /* Abort execution of this process after the timout. We simply rely on SIGALRM as + * default action terminating the process, and turn on alarm(). */ + + if (timeout != USEC_INFINITY) + alarm((timeout + USEC_PER_SEC - 1) / USEC_PER_SEC); STRV_FOREACH(path, paths) { _cleanup_free_ char *t = NULL; + _cleanup_close_ int fd = -1; pid_t pid; t = strdup(*path); if (!t) return log_oom(); - r = do_spawn(t, argv, &pid); + if (callbacks) { + fd = open_serialization_fd(basename(*path)); + if (fd < 0) + return log_error_errno(fd, "Failed to open serialization file: %m"); + } + + r = do_spawn(t, argv, fd, &pid); if (r <= 0) continue; - r = hashmap_put(pids, PID_TO_PTR(pid), t); - if (r < 0) - return log_oom(); + if (pids) { + r = hashmap_put(pids, PID_TO_PTR(pid), t); + if (r < 0) + return log_oom(); + t = NULL; + } else { + r = wait_for_terminate_and_warn(t, pid, true); + if (r < 0) + continue; - t = NULL; + if (lseek(fd, 0, SEEK_SET) < 0) + return log_error_errno(errno, "Failed to seek on serialization fd: %m"); + + r = callbacks[STDOUT_GENERATE](fd, callback_args[STDOUT_GENERATE]); + fd = -1; + if (r < 0) + return log_error_errno(r, "Failed to process output from %s: %m", *path); + } } - /* Abort execution of this process after the timout. We simply - * rely on SIGALRM as default action terminating the process, - * and turn on alarm(). */ - - if (timeout != USEC_INFINITY) - alarm((timeout + USEC_PER_SEC - 1) / USEC_PER_SEC); + if (callbacks) { + r = callbacks[STDOUT_COLLECT](output_fd, callback_args[STDOUT_COLLECT]); + if (r < 0) + return log_error_errno(r, "Callback two failed: %m"); + } while (!hashmap_isempty(pids)) { _cleanup_free_ char *t = NULL; @@ -136,31 +188,66 @@ static int do_execute(char **directories, usec_t timeout, char *argv[]) { return 0; } -void execute_directories(const char* const* directories, usec_t timeout, char *argv[]) { +int execute_directories( + const char* const* directories, + usec_t timeout, + gather_stdout_callback_t const callbacks[_STDOUT_CONSUME_MAX], + void* const callback_args[_STDOUT_CONSUME_MAX], + char *argv[]) { + pid_t executor_pid; - int r; char *name; char **dirs = (char**) directories; + _cleanup_close_ int fd = -1; + int r; assert(!strv_isempty(dirs)); name = basename(dirs[0]); assert(!isempty(name)); - /* Executes all binaries in the directories in parallel and waits - * for them to finish. Optionally a timeout is applied. If a file - * with the same name exists in more than one directory, the - * earliest one wins. */ + if (callbacks) { + assert(callback_args); + assert(callbacks[STDOUT_GENERATE]); + assert(callbacks[STDOUT_COLLECT]); + assert(callbacks[STDOUT_CONSUME]); + + fd = open_serialization_fd(name); + if (fd < 0) + return log_error_errno(fd, "Failed to open serialization file: %m"); + } + + /* Executes all binaries in the directories serially or in parallel and waits for + * them to finish. Optionally a timeout is applied. If a file with the same name + * exists in more than one directory, the earliest one wins. */ executor_pid = fork(); - if (executor_pid < 0) { - log_error_errno(errno, "Failed to fork: %m"); - return; + if (executor_pid < 0) + return log_error_errno(errno, "Failed to fork: %m"); - } else if (executor_pid == 0) { - r = do_execute(dirs, timeout, argv); + if (executor_pid == 0) { + r = do_execute(dirs, timeout, callbacks, callback_args, fd, argv); _exit(r < 0 ? EXIT_FAILURE : EXIT_SUCCESS); } - wait_for_terminate_and_warn(name, executor_pid, true); + r = wait_for_terminate_and_warn(name, executor_pid, true); + if (r < 0) + return log_error_errno(r, "Execution failed: %m"); + if (r > 0) { + /* non-zero return code from child */ + log_error("Forker process failed."); + return -EREMOTEIO; + } + + if (!callbacks) + return 0; + + if (lseek(fd, 0, SEEK_SET) < 0) + return log_error_errno(errno, "Failed to rewind serialization fd: %m"); + + r = callbacks[STDOUT_CONSUME](fd, callback_args[STDOUT_CONSUME]); + fd = -1; + if (r < 0) + return log_error_errno(r, "Failed to parse returned data: %m"); + return 0; } diff --git a/src/basic/exec-util.h b/src/basic/exec-util.h index 9f8daa9fc8..2c58e4bd5c 100644 --- a/src/basic/exec-util.h +++ b/src/basic/exec-util.h @@ -17,6 +17,22 @@ along with systemd; If not, see . ***/ +#include + #include "time-util.h" -void execute_directories(const char* const* directories, usec_t timeout, char *argv[]); +typedef int (*gather_stdout_callback_t) (int fd, void *arg); + +enum { + STDOUT_GENERATE, /* from generators to helper process */ + STDOUT_COLLECT, /* from helper process to main process */ + STDOUT_CONSUME, /* process data in main process */ + _STDOUT_CONSUME_MAX, +}; + +int execute_directories( + const char* const* directories, + usec_t timeout, + gather_stdout_callback_t const callbacks[_STDOUT_CONSUME_MAX], + void* const callback_args[_STDOUT_CONSUME_MAX], + char *argv[]); diff --git a/src/core/manager.c b/src/core/manager.c index d432512a59..73ac7499bd 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3047,7 +3047,8 @@ static int manager_run_generators(Manager *m) { argv[4] = NULL; RUN_WITH_UMASK(0022) - execute_directories((const char* const*) paths, DEFAULT_TIMEOUT_USEC, (char**) argv); + execute_directories((const char* const*) paths, DEFAULT_TIMEOUT_USEC, + NULL, NULL, (char**) argv); finish: lookup_paths_trim_generator(&m->lookup_paths); diff --git a/src/core/shutdown.c b/src/core/shutdown.c index 56a035e234..a2309b7726 100644 --- a/src/core/shutdown.c +++ b/src/core/shutdown.c @@ -322,7 +322,7 @@ int main(int argc, char *argv[]) { arguments[0] = NULL; arguments[1] = arg_verb; arguments[2] = NULL; - execute_directories(dirs, DEFAULT_TIMEOUT_USEC, arguments); + execute_directories(dirs, DEFAULT_TIMEOUT_USEC, NULL, NULL, arguments); if (!in_container && !in_initrd() && access("/run/initramfs/shutdown", X_OK) == 0) { diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index b0f992fc9c..a6978b6273 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -107,7 +107,7 @@ static int execute(char **modes, char **states) { if (r < 0) return r; - execute_directories(dirs, DEFAULT_TIMEOUT_USEC, arguments); + execute_directories(dirs, DEFAULT_TIMEOUT_USEC, NULL, NULL, arguments); log_struct(LOG_INFO, LOG_MESSAGE_ID(SD_MESSAGE_SLEEP_START), @@ -126,7 +126,7 @@ static int execute(char **modes, char **states) { NULL); arguments[1] = (char*) "post"; - execute_directories(dirs, DEFAULT_TIMEOUT_USEC, arguments); + execute_directories(dirs, DEFAULT_TIMEOUT_USEC, NULL, NULL, arguments); return r; } diff --git a/src/test/test-exec-util.c b/src/test/test-exec-util.c index 26533f0bf6..41cbef74b1 100644 --- a/src/test/test-exec-util.c +++ b/src/test/test-exec-util.c @@ -24,21 +24,59 @@ #include #include +#include "alloc-util.h" +#include "copy.h" #include "def.h" #include "exec-util.h" +#include "fd-util.h" #include "fileio.h" #include "fs-util.h" #include "log.h" #include "macro.h" #include "rm-rf.h" #include "string-util.h" +#include "strv.h" -static void test_execute_directory(void) { - char template_lo[] = "/tmp/test-readlink_and_make_absolute-lo.XXXXXXX"; - char template_hi[] = "/tmp/test-readlink_and_make_absolute-hi.XXXXXXX"; +static int here = 0, here2 = 0, here3 = 0; +void *ignore_stdout_args[] = {&here, &here2, &here3}; + +/* noop handlers, just check that arguments are passed correctly */ +static int ignore_stdout_func(int fd, void *arg) { + assert(fd >= 0); + assert(arg == &here); + safe_close(fd); + + return 0; +} +static int ignore_stdout_func2(int fd, void *arg) { + assert(fd >= 0); + assert(arg == &here2); + safe_close(fd); + + return 0; +} +static int ignore_stdout_func3(int fd, void *arg) { + assert(fd >= 0); + assert(arg == &here3); + safe_close(fd); + + return 0; +} + +static const gather_stdout_callback_t ignore_stdout[] = { + ignore_stdout_func, + ignore_stdout_func2, + ignore_stdout_func3, +}; + +static void test_execute_directory(bool gather_stdout) { + char template_lo[] = "/tmp/test-exec-util.XXXXXXX"; + char template_hi[] = "/tmp/test-exec-util.XXXXXXX"; const char * dirs[] = {template_hi, template_lo, NULL}; const char *name, *name2, *name3, *overridden, *override, *masked, *mask; + log_info("/* %s (%s) */", __func__, gather_stdout ? "gathering stdout" : "asynchronous"); + assert_se(mkdtemp(template_lo)); assert_se(mkdtemp(template_hi)); @@ -50,20 +88,34 @@ static void test_execute_directory(void) { masked = strjoina(template_lo, "/masked"); mask = strjoina(template_hi, "/masked"); - assert_se(write_string_file(name, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/it_works", WRITE_STRING_FILE_CREATE) == 0); - assert_se(write_string_file(name2, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/it_works2", WRITE_STRING_FILE_CREATE) == 0); - assert_se(write_string_file(overridden, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/failed", WRITE_STRING_FILE_CREATE) == 0); - assert_se(write_string_file(override, "#!/bin/sh\necho 'Executing '$0", WRITE_STRING_FILE_CREATE) == 0); - assert_se(write_string_file(masked, "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/failed", WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(name, + "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/it_works", + WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(name2, + "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/it_works2", + WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(overridden, + "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/failed", + WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(override, + "#!/bin/sh\necho 'Executing '$0", + WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(masked, + "#!/bin/sh\necho 'Executing '$0\ntouch $(dirname $0)/failed", + WRITE_STRING_FILE_CREATE) == 0); assert_se(symlink("/dev/null", mask) == 0); + assert_se(touch(name3) >= 0); + assert_se(chmod(name, 0755) == 0); assert_se(chmod(name2, 0755) == 0); assert_se(chmod(overridden, 0755) == 0); assert_se(chmod(override, 0755) == 0); assert_se(chmod(masked, 0755) == 0); - assert_se(touch(name3) >= 0); - execute_directories(dirs, DEFAULT_TIMEOUT_USEC, NULL); + if (gather_stdout) + execute_directories(dirs, DEFAULT_TIMEOUT_USEC, ignore_stdout, ignore_stdout_args, NULL); + else + execute_directories(dirs, DEFAULT_TIMEOUT_USEC, NULL, NULL, NULL); assert_se(chdir(template_lo) == 0); assert_se(access("it_works", F_OK) >= 0); @@ -77,11 +129,157 @@ static void test_execute_directory(void) { (void) rm_rf(template_hi, REMOVE_ROOT|REMOVE_PHYSICAL); } -int main(int argc, char *argv[]) { - log_parse_environment(); - log_open(); +static void test_execution_order(void) { + char template_lo[] = "/tmp/test-exec-util-lo.XXXXXXX"; + char template_hi[] = "/tmp/test-exec-util-hi.XXXXXXX"; + const char *dirs[] = {template_hi, template_lo, NULL}; + const char *name, *name2, *name3, *overridden, *override, *masked, *mask; + const char *output, *t; + _cleanup_free_ char *contents = NULL; - test_execute_directory(); + assert_se(mkdtemp(template_lo)); + assert_se(mkdtemp(template_hi)); + + output = strjoina(template_hi, "/output"); + + log_info("/* %s >>%s */", __func__, output); + + /* write files in "random" order */ + name2 = strjoina(template_lo, "/90-bar"); + name = strjoina(template_hi, "/80-foo"); + name3 = strjoina(template_lo, "/last"); + overridden = strjoina(template_lo, "/30-override"); + override = strjoina(template_hi, "/30-override"); + masked = strjoina(template_lo, "/10-masked"); + mask = strjoina(template_hi, "/10-masked"); + + t = strjoina("#!/bin/sh\necho $(basename $0) >>", output); + assert_se(write_string_file(name, t, WRITE_STRING_FILE_CREATE) == 0); + + t = strjoina("#!/bin/sh\necho $(basename $0) >>", output); + assert_se(write_string_file(name2, t, WRITE_STRING_FILE_CREATE) == 0); + + t = strjoina("#!/bin/sh\necho $(basename $0) >>", output); + assert_se(write_string_file(name3, t, WRITE_STRING_FILE_CREATE) == 0); + + t = strjoina("#!/bin/sh\necho OVERRIDDEN >>", output); + assert_se(write_string_file(overridden, t, WRITE_STRING_FILE_CREATE) == 0); + + t = strjoina("#!/bin/sh\necho $(basename $0) >>", output); + assert_se(write_string_file(override, t, WRITE_STRING_FILE_CREATE) == 0); + + t = strjoina("#!/bin/sh\necho MASKED >>", output); + assert_se(write_string_file(masked, t, WRITE_STRING_FILE_CREATE) == 0); + + assert_se(symlink("/dev/null", mask) == 0); + + assert_se(chmod(name, 0755) == 0); + assert_se(chmod(name2, 0755) == 0); + assert_se(chmod(name3, 0755) == 0); + assert_se(chmod(overridden, 0755) == 0); + assert_se(chmod(override, 0755) == 0); + assert_se(chmod(masked, 0755) == 0); + + execute_directories(dirs, DEFAULT_TIMEOUT_USEC, ignore_stdout, ignore_stdout_args, NULL); + + assert_se(read_full_file(output, &contents, NULL) >= 0); + assert_se(streq(contents, "30-override\n80-foo\n90-bar\nlast\n")); + + (void) rm_rf(template_lo, REMOVE_ROOT|REMOVE_PHYSICAL); + (void) rm_rf(template_hi, REMOVE_ROOT|REMOVE_PHYSICAL); +} + +static int gather_stdout_one(int fd, void *arg) { + char ***s = arg, *t; + char buf[128] = {}; + + assert_se(s); + assert_se(read(fd, buf, sizeof buf) >= 0); + safe_close(fd); + + assert_se(t = strndup(buf, sizeof buf)); + assert_se(strv_push(s, t) >= 0); + + return 0; +} +static int gather_stdout_two(int fd, void *arg) { + char ***s = arg, **t; + + STRV_FOREACH(t, *s) + assert_se(write(fd, *t, strlen(*t)) == (ssize_t) strlen(*t)); + safe_close(fd); + + return 0; +} +static int gather_stdout_three(int fd, void *arg) { + char **s = arg; + char buf[128] = {}; + + assert_se(read(fd, buf, sizeof buf - 1) > 0); + safe_close(fd); + assert_se(*s = strndup(buf, sizeof buf)); + + return 0; +} + +const gather_stdout_callback_t const gather_stdout[] = { + gather_stdout_one, + gather_stdout_two, + gather_stdout_three, +}; + + +static void test_stdout_gathering(void) { + char template[] = "/tmp/test-exec-util.XXXXXXX"; + const char *dirs[] = {template, NULL}; + const char *name, *name2, *name3; + int r; + + char **tmp = NULL; /* this is only used in the forked process, no cleanup here */ + _cleanup_free_ char *output = NULL; + + void* args[] = {&tmp, &tmp, &output}; + + assert_se(mkdtemp(template)); + + log_info("/* %s */", __func__); + + /* write files */ + name = strjoina(template, "/10-foo"); + name2 = strjoina(template, "/20-bar"); + name3 = strjoina(template, "/30-last"); + + assert_se(write_string_file(name, + "#!/bin/sh\necho a\necho b\necho c\n", + WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(name2, + "#!/bin/sh\necho d\n", + WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(name3, + "#!/bin/sh\nsleep 1", + WRITE_STRING_FILE_CREATE) == 0); + + assert_se(chmod(name, 0755) == 0); + assert_se(chmod(name2, 0755) == 0); + assert_se(chmod(name3, 0755) == 0); + + r = execute_directories(dirs, DEFAULT_TIMEOUT_USEC, gather_stdout, args, NULL); + assert_se(r >= 0); + + log_info("got: %s", output); + + assert_se(streq(output, "a\nb\nc\nd\n")); +} + +int main(int argc, char *argv[]) { + log_set_max_level(LOG_DEBUG); + log_parse_environment(); + log_open(); + + test_execute_directory(true); + test_execute_directory(false); + test_execution_order(); + test_stdout_gathering(); return 0; } From 71cb7d306ab42712f274337c457ac5cbdeff363c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 10 Feb 2017 15:41:42 -0500 Subject: [PATCH 10/29] core/manager: fix grammar in comment --- src/core/manager.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 73ac7499bd..eb9c4e65c6 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -531,9 +531,9 @@ static int manager_default_environment(Manager *m) { if (MANAGER_IS_SYSTEM(m)) { /* The system manager always starts with a clean * environment for its children. It does not import - * the kernel or the parents exported variables. + * the kernel's or the parents' exported variables. * - * The initial passed environ is untouched to keep + * The initial passed environment is untouched to keep * /proc/self/environ valid; it is used for tagging * the init process inside containers. */ m->environment = strv_new("PATH=" DEFAULT_PATH, @@ -541,11 +541,10 @@ static int manager_default_environment(Manager *m) { /* Import locale variables LC_*= from configuration */ locale_setup(&m->environment); - } else { + } else /* The user manager passes its own environment * along to its children. */ m->environment = strv_copy(environ); - } if (!m->environment) return -ENOMEM; From fe902fa496abb4583c5befaf671a2402b650cd14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 10 Feb 2017 21:44:21 -0500 Subject: [PATCH 11/29] core/manager: move environment serialization out to basic/env-util.c This protocol is generally useful, we might just as well reuse it for the env. generators. The implementation is changed a bit: instead of making a new strv and freeing the old one, just mutate the original. This is much faster with larger arrays, while in fact atomicity is preserved, since we only either insert the new entry or not, without being in inconsistent state. v2: - fix confusion with return value --- src/basic/env-util.c | 34 ++++++++++++++++++++++++++++++++++ src/basic/env-util.h | 4 ++++ src/core/manager.c | 30 ++++-------------------------- 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 96da38d45e..4645ec781e 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -26,6 +26,7 @@ #include "alloc-util.h" #include "env-util.h" +#include "escape.h" #include "extract-word.h" #include "macro.h" #include "parse-util.h" @@ -644,3 +645,36 @@ int getenv_bool(const char *p) { return parse_boolean(e); } + +int serialize_environment(FILE *f, char **environment) { + char **e; + + STRV_FOREACH(e, environment) { + _cleanup_free_ char *ce; + + ce = cescape(*e); + if (!ce) + return -ENOMEM; + + fprintf(f, "env=%s\n", *e); + } + + /* caller should call ferror() */ + + return 0; +} + +int deserialize_environment(char ***environment, const char *line) { + char *uce = NULL; + int r; + + assert(line); + assert(environment); + + assert(startswith(line, "env=")); + r = cunescape(line + 4, UNESCAPE_RELAX, &uce); + if (r < 0) + return r; + + return strv_env_replace(environment, uce); +} diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 8cb0fc2131..90df5b1cd9 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -21,6 +21,7 @@ #include #include +#include #include "macro.h" @@ -50,3 +51,6 @@ char *strv_env_get_n(char **l, const char *name, size_t k) _pure_; char *strv_env_get(char **x, const char *n) _pure_; int getenv_bool(const char *p); + +int serialize_environment(FILE *f, char **environment); +int deserialize_environment(char ***environment, const char *line); diff --git a/src/core/manager.c b/src/core/manager.c index eb9c4e65c6..6bbda1af0d 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2459,7 +2459,6 @@ int manager_serialize(Manager *m, FILE *f, FDSet *fds, bool switching_root) { Iterator i; Unit *u; const char *t; - char **e; int r; assert(m); @@ -2489,17 +2488,8 @@ int manager_serialize(Manager *m, FILE *f, FDSet *fds, bool switching_root) { dual_timestamp_serialize(f, "units-load-finish-timestamp", &m->units_load_finish_timestamp); } - if (!switching_root) { - STRV_FOREACH(e, m->environment) { - _cleanup_free_ char *ce; - - ce = cescape(*e); - if (!ce) - return -ENOMEM; - - fprintf(f, "env=%s\n", *e); - } - } + if (!switching_root) + (void) serialize_environment(f, m->environment); if (m->notify_fd >= 0) { int copy; @@ -2662,21 +2652,9 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { else if ((val = startswith(l, "units-load-finish-timestamp="))) dual_timestamp_deserialize(val, &m->units_load_finish_timestamp); else if (startswith(l, "env=")) { - _cleanup_free_ char *uce = NULL; - char **e; - - r = cunescape(l + 4, UNESCAPE_RELAX, &uce); + r = deserialize_environment(&m->environment, l); if (r < 0) - goto finish; - - e = strv_env_set(m->environment, uce); - if (!e) { - r = -ENOMEM; - goto finish; - } - - strv_free(m->environment); - m->environment = e; + return r; } else if ((val = startswith(l, "notify-fd="))) { int fd; From ac466818814f347b74ece4463e6f1ec662257e19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 10 Feb 2017 22:14:03 -0500 Subject: [PATCH 12/29] basic/fileio: add helper function for a set of two common checks --- src/basic/fileio.c | 58 ++++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 33 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index ac65fada35..fb41431ec9 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -586,14 +586,9 @@ fail: return r; } -static int parse_env_file_push( +static int check_utf8ness_and_warn( const char *filename, unsigned line, - const char *key, char *value, - void *userdata, - int *n_pushed) { - - const char *k; - va_list aq, *ap = userdata; + const char *key, char *value) { if (!utf8_is_valid(key)) { _cleanup_free_ char *p = NULL; @@ -611,6 +606,23 @@ static int parse_env_file_push( return -EINVAL; } + return 0; +} + +static int parse_env_file_push( + const char *filename, unsigned line, + const char *key, char *value, + void *userdata, + int *n_pushed) { + + const char *k; + va_list aq, *ap = userdata; + int r; + + r = check_utf8ness_and_warn(filename, line, key, value); + if (r < 0) + return r; + va_copy(aq, *ap); while ((k = va_arg(aq, const char *))) { @@ -662,19 +674,9 @@ static int load_env_file_push( char *p; int r; - if (!utf8_is_valid(key)) { - _cleanup_free_ char *t = utf8_escape_invalid(key); - - log_error("%s:%u: invalid UTF-8 for key '%s', ignoring.", strna(filename), line, t); - return -EINVAL; - } - - if (value && !utf8_is_valid(value)) { - _cleanup_free_ char *t = utf8_escape_invalid(value); - - log_error("%s:%u: invalid UTF-8 value for key %s: '%s', ignoring.", strna(filename), line, key, t); - return -EINVAL; - } + r = check_utf8ness_and_warn(filename, line, key, value); + if (r < 0) + return r; p = strjoin(key, "=", strempty(value)); if (!p) @@ -716,19 +718,9 @@ static int load_env_file_push_pairs( char ***m = userdata; int r; - if (!utf8_is_valid(key)) { - _cleanup_free_ char *t = utf8_escape_invalid(key); - - log_error("%s:%u: invalid UTF-8 for key '%s', ignoring.", strna(filename), line, t); - return -EINVAL; - } - - if (value && !utf8_is_valid(value)) { - _cleanup_free_ char *t = utf8_escape_invalid(value); - - log_error("%s:%u: invalid UTF-8 value for key %s: '%s', ignoring.", strna(filename), line, key, t); - return -EINVAL; - } + r = check_utf8ness_and_warn(filename, line, key, value); + if (r < 0) + return r; r = strv_extend(m, key); if (r < 0) From 99003e01b6f7171f884ee6da663326aa1aa44e2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 10 Feb 2017 23:08:53 -0500 Subject: [PATCH 13/29] env-util,fileio: immediately replace variables in load_env_file_push() strv_env_replace was calling env_match(), which in effect allowed multiple values for the same key to be inserted into the environment block. That's pointless, because APIs to access variables only return a single value (the latest entry), so it's better to keep the block clean, i.e. with just a single entry for each key. Add a new helper function that simply tests if the part before '=' is equal in two strings and use that in strv_env_replace. In load_env_file_push, use strv_env_replace to immediately replace the previous assignment with a matching name. Afaict, none of the callers are materially affected by this change, but it seems like some pointless work was being done, if the same value was set multiple times. We'd go through parsing and assigning the value for each entry. With this change, we handle just the last one. --- src/basic/env-util.c | 30 ++++++++++++++++++++++++++---- src/basic/fileio.c | 9 ++++++--- src/test/test-fileio.c | 2 ++ 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 4645ec781e..05b90e499e 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -274,6 +274,19 @@ _pure_ static bool env_match(const char *t, const char *pattern) { return false; } +static bool env_entry_has_name(const char *entry, const char *name) { + const char *t; + + assert(entry); + assert(name); + + t = startswith(entry, name); + if (!t) + return false; + + return *t == '='; +} + char **strv_env_delete(char **x, unsigned n_lists, ...) { size_t n, i = 0; char **k, **r; @@ -387,18 +400,24 @@ char **strv_env_unset_many(char **l, ...) { int strv_env_replace(char ***l, char *p) { char **f; + const char *t, *name; assert(p); /* Replace first occurrence of the env var or add a new one in the * string list. Drop other occurences. Edits in-place. Does not copy p. + * p must be a valid key=value assignment. */ + t = strchr(p, '='); + assert(t); + + name = strndupa(p, t - p); + for (f = *l; f && *f; f++) - if (env_match(*f, p)) { - free(*f); - *f = p; - strv_env_unset(f + 1, p); + if (env_entry_has_name(*f, name)) { + free_and_replace(*f, p); + strv_env_unset(f + 1, *f); return 0; } @@ -676,5 +695,8 @@ int deserialize_environment(char ***environment, const char *line) { if (r < 0) return r; + if (!env_assignment_is_valid(uce)) + return -EINVAL; + return strv_env_replace(environment, uce); } diff --git a/src/basic/fileio.c b/src/basic/fileio.c index fb41431ec9..a1e4978125 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -30,6 +30,7 @@ #include "alloc-util.h" #include "ctype.h" +#include "env-util.h" #include "escape.h" #include "fd-util.h" #include "fileio.h" @@ -678,13 +679,15 @@ static int load_env_file_push( if (r < 0) return r; - p = strjoin(key, "=", strempty(value)); + p = strjoin(key, "=", value); if (!p) return -ENOMEM; - r = strv_consume(m, p); - if (r < 0) + r = strv_env_replace(m, p); + if (r < 0) { + free(p); return r; + } if (n_pushed) (*n_pushed)++; diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index 56316904a3..a38bb874a9 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -71,6 +71,8 @@ static void test_parse_env_file(void) { "seven=\"sevenval\" #nocomment\n" "eight=eightval #nocomment\n" "export nine=nineval\n" + "ten=ignored\n" + "ten=ignored\n" "ten=", f); fflush(f); From c8cebc36b02f6386faccce7d84b444155f3765ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 18 Feb 2017 16:23:03 -0500 Subject: [PATCH 14/29] basic/env-util: drop _pure_ from static function --- src/basic/env-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 05b90e499e..a28707fb49 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -248,7 +248,7 @@ fail: return NULL; } -_pure_ static bool env_match(const char *t, const char *pattern) { +static bool env_match(const char *t, const char *pattern) { assert(t); assert(pattern); From 3303d1b2dc3f9ac88927c23abce020fc77c1e92c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 10 Feb 2017 21:49:01 -0500 Subject: [PATCH 15/29] exec-util: implement a set of callbacks to pass variables around Only tests are added, otherwise the new code is unused. --- src/basic/exec-util.c | 102 ++++++++++++++++++++++++++++++++++++++ src/basic/exec-util.h | 2 + src/test/test-exec-util.c | 55 ++++++++++++++++++++ 3 files changed, 159 insertions(+) diff --git a/src/basic/exec-util.c b/src/basic/exec-util.c index d69039c489..207fac8dc1 100644 --- a/src/basic/exec-util.c +++ b/src/basic/exec-util.c @@ -251,3 +251,105 @@ int execute_directories( return log_error_errno(r, "Failed to parse returned data: %m"); return 0; } + +static int gather_environment_generate(int fd, void *arg) { + char ***env = arg, **x, **y; + _cleanup_fclose_ FILE *f = NULL; + _cleanup_strv_free_ char **new; + int r; + + /* Read a series of VAR=value assignments from fd, use them to update the list of + * variables in env. Also update the exported environment. + * + * fd is always consumed, even on error. + */ + + assert(env); + + f = fdopen(fd, "r"); + if (!f) { + safe_close(fd); + return -errno; + } + + r = load_env_file_pairs(f, NULL, NULL, &new); + if (r < 0) + return r; + + STRV_FOREACH_PAIR(x, y, new) { + char *p; + + p = strjoin(*x, "=", *y); + if (!p) + return -ENOMEM; + + r = strv_env_replace(env, p); + if (r < 0) + return r; + + if (setenv(*x, *y, true) < 0) + return -errno; + } + + return r; +} + +static int gather_environment_collect(int fd, void *arg) { + char ***env = arg; + _cleanup_fclose_ FILE *f = NULL; + int r; + + /* Write out a series of env=cescape(VAR=value) assignments to fd. */ + + assert(env); + + f = fdopen(fd, "w"); + if (!f) { + safe_close(fd); + return -errno; + } + + r = serialize_environment(f, *env); + if (r < 0) + return r; + + if (ferror(f)) + return errno > 0 ? -errno : -EIO; + + return 0; +} + +static int gather_environment_consume(int fd, void *arg) { + char ***env = arg; + _cleanup_fclose_ FILE *f = NULL; + char line[LINE_MAX]; + int r = 0, k; + + /* Read a series of env=cescape(VAR=value) assignments from fd into env. */ + + assert(env); + + f = fdopen(fd, "r"); + if (!f) { + safe_close(fd); + return -errno; + } + + FOREACH_LINE(line, f, return -EIO) { + truncate_nl(line); + + k = deserialize_environment(env, line); + if (k < 0) + log_error_errno(k, "Invalid line \"%s\": %m", line); + if (k < 0 && r == 0) + r = k; + } + + return r; +} + +const gather_stdout_callback_t gather_environment[] = { + gather_environment_generate, + gather_environment_collect, + gather_environment_consume, +}; diff --git a/src/basic/exec-util.h b/src/basic/exec-util.h index 2c58e4bd5c..72009799b2 100644 --- a/src/basic/exec-util.h +++ b/src/basic/exec-util.h @@ -36,3 +36,5 @@ int execute_directories( gather_stdout_callback_t const callbacks[_STDOUT_CONSUME_MAX], void* const callback_args[_STDOUT_CONSUME_MAX], char *argv[]); + +extern const gather_stdout_callback_t gather_environment[_STDOUT_CONSUME_MAX]; diff --git a/src/test/test-exec-util.c b/src/test/test-exec-util.c index 41cbef74b1..94fe042aa9 100644 --- a/src/test/test-exec-util.c +++ b/src/test/test-exec-util.c @@ -27,6 +27,7 @@ #include "alloc-util.h" #include "copy.h" #include "def.h" +#include "env-util.h" #include "exec-util.h" #include "fd-util.h" #include "fileio.h" @@ -271,6 +272,59 @@ static void test_stdout_gathering(void) { assert_se(streq(output, "a\nb\nc\nd\n")); } +static void test_environment_gathering(void) { + char template[] = "/tmp/test-exec-util.XXXXXXX", **p; + const char *dirs[] = {template, NULL}; + const char *name, *name2, *name3; + int r; + + char **tmp = NULL; /* this is only used in the forked process, no cleanup here */ + _cleanup_strv_free_ char **env = NULL; + + void* const args[] = { &tmp, &tmp, &env }; + + assert_se(mkdtemp(template)); + + log_info("/* %s */", __func__); + + /* write files */ + name = strjoina(template, "/10-foo"); + name2 = strjoina(template, "/20-bar"); + name3 = strjoina(template, "/30-last"); + + assert_se(write_string_file(name, + "#!/bin/sh\n" + "echo A=23\n", + WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(name2, + "#!/bin/sh\n" + "echo A=22:$A\n\n\n", /* substitution from previous generator */ + WRITE_STRING_FILE_CREATE) == 0); + assert_se(write_string_file(name3, + "#!/bin/sh\n" + "echo A=$A:24\n" + "echo B=12\n" + "echo C=000\n" + "echo C=001\n" /* variable overwriting */ + "echo PATH=$PATH:/no/such/file", /* variable from manager */ + WRITE_STRING_FILE_CREATE) == 0); + + assert_se(chmod(name, 0755) == 0); + assert_se(chmod(name2, 0755) == 0); + assert_se(chmod(name3, 0755) == 0); + + r = execute_directories(dirs, DEFAULT_TIMEOUT_USEC, gather_environment, args, NULL); + assert_se(r >= 0); + + STRV_FOREACH(p, env) + log_info("got env: \"%s\"", *p); + + assert_se(streq(strv_env_get(env, "A"), "22:23:24")); + assert_se(streq(strv_env_get(env, "B"), "12")); + assert_se(streq(strv_env_get(env, "C"), "001")); + assert_se(endswith(strv_env_get(env, "PATH"), ":/no/such/file")); +} + int main(int argc, char *argv[]) { log_set_max_level(LOG_DEBUG); log_parse_environment(); @@ -280,6 +334,7 @@ int main(int argc, char *argv[]) { test_execute_directory(false); test_execution_order(); test_stdout_gathering(); + test_environment_gathering(); return 0; } From 64691d2024d3bd202c85c52069f185afee2b8e43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 22 Jan 2017 01:13:47 -0500 Subject: [PATCH 16/29] manager: run environment generators Environment file generators are a lot like unit file generators, but not exactly: 1. environment file generators are run for each manager instance, and their output is (or at least can be) individualized. The generators themselves are system-wide, the same for all users. 2. environment file generators are run sequentially, in priority order. Thus, the lifetime of those files is tied to lifecycle of the manager instance. Because generators are run sequentially, later generators can use or modify the output of earlier generators. Each generator is run with no arguments, and the whole state is stored in the environment variables. The generator can echo a set of variable assignments to standard output: VAR_A=something VAR_B=something else This output is parsed, and the next and subsequent generators run with those updated variables in the environment. After the last generator is done, the environment that the manager itself exports is updated. Each generator must return 0, otherwise the output is ignored. The generators in */user-env-generator are for the user session managers, including root, and the ones in */system-env-generator are for pid1. --- Makefile.am | 6 ++++ src/core/macros.systemd.in | 2 ++ src/core/manager.c | 70 +++++++++++++++++++++++++++++++------- 3 files changed, 66 insertions(+), 12 deletions(-) diff --git a/Makefile.am b/Makefile.am index 003ec9bfb7..f6699252d1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -80,6 +80,8 @@ networkdir=$(rootprefix)/lib/systemd/network pkgincludedir=$(includedir)/systemd systemgeneratordir=$(rootlibexecdir)/system-generators usergeneratordir=$(prefix)/lib/systemd/user-generators +systemenvgeneratordir=$(prefix)/lib/systemd/system-environment-generators +userenvgeneratordir=$(prefix)/lib/systemd/user-environment-generators systemshutdowndir=$(rootlibexecdir)/system-shutdown systemsleepdir=$(rootlibexecdir)/system-sleep systemunitdir=$(rootprefix)/lib/systemd/system @@ -205,6 +207,8 @@ AM_CPPFLAGS = \ -DSYSTEMD_CRYPTSETUP_PATH=\"$(rootlibexecdir)/systemd-cryptsetup\" \ -DSYSTEM_GENERATOR_PATH=\"$(systemgeneratordir)\" \ -DUSER_GENERATOR_PATH=\"$(usergeneratordir)\" \ + -DSYSTEM_ENV_GENERATOR_PATH=\"$(systemenvgeneratordir)\" \ + -DUSER_ENV_GENERATOR_PATH=\"$(userenvgeneratordir)\" \ -DSYSTEM_SHUTDOWN_PATH=\"$(systemshutdowndir)\" \ -DSYSTEM_SLEEP_PATH=\"$(systemsleepdir)\" \ -DSYSTEMD_KBD_MODEL_MAP=\"$(pkgdatadir)/kbd-model-map\" \ @@ -6210,6 +6214,8 @@ substitutions = \ '|sysctldir=$(sysctldir)|' \ '|systemgeneratordir=$(systemgeneratordir)|' \ '|usergeneratordir=$(usergeneratordir)|' \ + '|systemenvgeneratordir=$(systemenvgeneratordir)|' \ + '|userenvgeneratordir=$(userenvgeneratordir)|' \ '|CERTIFICATEROOT=$(CERTIFICATEROOT)|' \ '|PACKAGE_VERSION=$(PACKAGE_VERSION)|' \ '|PACKAGE_NAME=$(PACKAGE_NAME)|' \ diff --git a/src/core/macros.systemd.in b/src/core/macros.systemd.in index 8d7ce1c238..a2a7edd1ee 100644 --- a/src/core/macros.systemd.in +++ b/src/core/macros.systemd.in @@ -31,6 +31,8 @@ %_binfmtdir @binfmtdir@ %_systemdgeneratordir @systemgeneratordir@ %_systemdusergeneratordir @usergeneratordir@ +%_systemd_system_env_generator_dir @systemenvgeneratordir@ +%_systemd_user_env_generator_dir @userenvgeneratordir@ %systemd_requires \ Requires(post): systemd \ diff --git a/src/core/manager.c b/src/core/manager.c index 6bbda1af0d..aefe4ca780 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -103,6 +103,7 @@ static int manager_dispatch_idle_pipe_fd(sd_event_source *source, int fd, uint32 static int manager_dispatch_user_lookup_fd(sd_event_source *source, int fd, uint32_t revents, void *userdata); static int manager_dispatch_jobs_in_progress(sd_event_source *source, usec_t usec, void *userdata); static int manager_dispatch_run_queue(sd_event_source *source, void *userdata); +static int manager_run_environment_generators(Manager *m); static int manager_run_generators(Manager *m); static void manager_watch_jobs_in_progress(Manager *m) { @@ -1262,6 +1263,10 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { if (r < 0) return r; + r = manager_run_environment_generators(m); + if (r < 0) + return r; + /* Make sure the transient directory always exists, so that it remains in the search path */ if (!m->test_run) { r = mkdir_p_label(m->lookup_paths.transient, 0755); @@ -2795,6 +2800,10 @@ int manager_reload(Manager *m) { if (q < 0 && r >= 0) r = q; + q = manager_run_environment_generators(m); + if (q < 0 && r >= 0) + r = q; + /* Find new unit paths */ q = manager_run_generators(m); if (q < 0 && r >= 0) @@ -2986,10 +2995,56 @@ void manager_check_finished(Manager *m) { manager_invalidate_startup_units(m); } +static bool generator_path_any(const char* const* paths) { + char **path; + bool found = false; + + /* Optimize by skipping the whole process by not creating output directories + * if no generators are found. */ + STRV_FOREACH(path, (char**) paths) + if (access(*path, F_OK) == 0) + found = true; + else if (errno != ENOENT) + log_warning_errno(errno, "Failed to open generator directory %s: %m", *path); + + return found; +} + +static const char* system_env_generator_binary_paths[] = { + "/run/systemd/system-environment-generators", + "/etc/systemd/system-environment-generators", + "/usr/local/lib/systemd/system-environment-generators", + SYSTEM_ENV_GENERATOR_PATH, + NULL +}; + +static const char* user_env_generator_binary_paths[] = { + "/run/systemd/user-environment-generators", + "/etc/systemd/user-environment-generators", + "/usr/local/lib/systemd/user-environment-generators", + USER_ENV_GENERATOR_PATH, + NULL +}; + +static int manager_run_environment_generators(Manager *m) { + char **tmp = NULL; /* this is only used in the forked process, no cleanup here */ + const char **paths; + void* args[] = {&tmp, &tmp, &m->environment}; + + if (m->test_run) + return 0; + + paths = MANAGER_IS_SYSTEM(m) ? system_env_generator_binary_paths : user_env_generator_binary_paths; + + if (!generator_path_any(paths)) + return 0; + + return execute_directories(paths, DEFAULT_TIMEOUT_USEC, gather_environment, args, NULL); +} + static int manager_run_generators(Manager *m) { _cleanup_strv_free_ char **paths = NULL; const char *argv[5]; - char **path; int r; assert(m); @@ -3001,18 +3056,9 @@ static int manager_run_generators(Manager *m) { if (!paths) return log_oom(); - /* Optimize by skipping the whole process by not creating output directories - * if no generators are found. */ - STRV_FOREACH(path, paths) { - if (access(*path, F_OK) >= 0) - goto found; - if (errno != ENOENT) - log_warning_errno(errno, "Failed to open generator directory %s: %m", *path); - } + if (!generator_path_any((const char* const*) paths)) + return 0; - return 0; - - found: r = lookup_paths_mkdir_generator(&m->lookup_paths); if (r < 0) goto finish; From 1bd2d4e31b739586cf91ddfe65349f79298af013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 23 Jan 2017 01:11:45 -0500 Subject: [PATCH 17/29] man: add systemd.environment-generator(7) with two examples v2: - add example files to EXTRA_DIST v3: - rework for the new scheme where nothing is written to disk v4: - use separate dirs for system and user env generators --- Makefile-man.am | 2 + Makefile.am | 4 +- man/50-xdg-data-dirs.sh | 12 ++ man/90-rearrange-path.py | 40 +++++++ man/systemd.environment-generator.xml | 159 ++++++++++++++++++++++++++ man/systemd.generator.xml | 3 +- 6 files changed, 218 insertions(+), 2 deletions(-) create mode 100755 man/50-xdg-data-dirs.sh create mode 100755 man/90-rearrange-path.py create mode 100644 man/systemd.environment-generator.xml diff --git a/Makefile-man.am b/Makefile-man.am index 6f59658445..86ea6afbe7 100644 --- a/Makefile-man.am +++ b/Makefile-man.am @@ -146,6 +146,7 @@ MANPAGES += \ man/systemd.1 \ man/systemd.automount.5 \ man/systemd.device.5 \ + man/systemd.environment-generator.7 \ man/systemd.exec.5 \ man/systemd.generator.7 \ man/systemd.journal-fields.7 \ @@ -2827,6 +2828,7 @@ EXTRA_DIST += \ man/systemd-volatile-root.service.xml \ man/systemd.automount.xml \ man/systemd.device.xml \ + man/systemd.environment-generator.xml \ man/systemd.exec.xml \ man/systemd.generator.xml \ man/systemd.journal-fields.xml \ diff --git a/Makefile.am b/Makefile.am index f6699252d1..67c433b43a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -765,7 +765,9 @@ EXTRA_DIST += \ tools/make-man-rules.py \ tools/make-directive-index.py \ tools/xml_helper.py \ - man/glib-event-glue.c + man/glib-event-glue.c \ + man/50-xdg-data-dirs.sh \ + man/90-rearrange-path.py # ------------------------------------------------------------------------------ noinst_LTLIBRARIES += \ diff --git a/man/50-xdg-data-dirs.sh b/man/50-xdg-data-dirs.sh new file mode 100755 index 0000000000..073174cb40 --- /dev/null +++ b/man/50-xdg-data-dirs.sh @@ -0,0 +1,12 @@ +#!/bin/bash + +# set the default value +XDG_DATA_DIRS="${XDG_DATA_DIRS:-/usr/local/share/:/usr/share}" + +# add a directory if it exists +if [[ -d /opt/foo/share ]]; then + XDG_DATA_DIRS=/opt/foo/share:${XDG_DATA_DIRS} +fi + +# write our output +echo XDG_DATA_DIRS=$XDG_DATA_DIRS diff --git a/man/90-rearrange-path.py b/man/90-rearrange-path.py new file mode 100755 index 0000000000..c6ff32210f --- /dev/null +++ b/man/90-rearrange-path.py @@ -0,0 +1,40 @@ +#!/usr/bin/python3 + +""" + +Proof-of-concept systemd environment generator that makes sure that bin dirs +are always after matching sbin dirs in the path. +(Changes /sbin:/bin:/foo/bar to /bin:/sbin:/foo/bar.) + +This generator shows how to override the configuration possibly created by +earlier generators. It would be easier to write in bash, but let's have it +in Python just to prove that we can, and to serve as a template for more +interesting generators. + +""" + +import os +import pathlib + +def rearrange_bin_sbin(path): + """Make sure any pair of …/bin, …/sbin directories is in this order + + >>> rearrange_bin_sbin('/bin:/sbin:/usr/sbin:/usr/bin') + '/bin:/sbin:/usr/bin:/usr/sbin' + """ + items = [pathlib.Path(p) for p in path.split(':')] + for i in range(len(items)): + if 'sbin' in items[i].parts: + ind = items[i].parts.index('sbin') + bin = pathlib.Path(*items[i].parts[:ind], 'bin', *items[i].parts[ind+1:]) + if bin in items[i+1:]: + j = i + 1 + items[i+1:].index(bin) + items[i], items[j] = items[j], items[i] + return ':'.join(p.as_posix() for p in items) + +if __name__ == '__main__': + path = os.environ['PATH'] # This should be always set. + # If it's not, we'll just crash, we is OK too. + new = rearrange_bin_sbin(path) + if new != path: + print('PATH={}'.format(new)) diff --git a/man/systemd.environment-generator.xml b/man/systemd.environment-generator.xml new file mode 100644 index 0000000000..e162dfcbae --- /dev/null +++ b/man/systemd.environment-generator.xml @@ -0,0 +1,159 @@ + + +%entities; +]> + + + + + + systemd.environment-generator + systemd + + + + Developer + Zbigniew + Jędrzejewski-Szmek + zbyszek@in.waw.pl + + + + + + systemd.environment-generator + 7 + + + + systemd.environment-generator + Systemd environment file generators + + + + + &systemenvgeneratordir;/some-generator + + + &userenvgeneratordir;/some-generator + + + + /run/systemd/system-environment-generators/* +/etc/systemd/system-environment-generators/* +/usr/local/lib/systemd/system-environment-generators/* +&systemenvgeneratordir;/* + + + + /run/systemd/user-environment-generators/* +/etc/systemd/user-environment-generators/* +/usr/local/lib/systemd/user-environment-generators/* +&userenvgeneratordir;/* + + + + + Description + Generators are small executables that live in + &systemenvgeneratordir;/ and other directories listed above. + systemd1 will + execute those binaries very early at the startup of each manager and at configuration + reload time, before running the generators described in + systemd.generator7 + and before starting any units. Environment generators can override the environment that the + manager exports to services and other processes. + + Generators are loaded from a set of paths determined during compilation, as listed + above. System and user environment generators are loaded from directories with names ending in + system-environment-generators/ and + user-environment-generators/, respectively. Generators found in directories + listed earlier override the ones with the same name in directories lower in the list. A symlink + to /dev/null or an empty file can be used to mask a generator, thereby + preventing it from running. Please note that the order of the two directories with the highest + priority is reversed with respect to the unit load path, and generators in + /run overwrite those in /etc. + + After installing new generators or updating the configuration, systemctl + daemon-reload may be executed. This will re-run all generators, updating environment + configuration. It will be used for any services that are started subsequently. + + Environment file generators are executed similarly to unit file generators described + in + systemd.generator7, + with the following differences: + + + + Generators are executed sequentially in the alphanumerical order of the final + component of their name. The output of each generator output is immediately parsed and used + to update the environment for generators that run after that. Thus, later generators can use + and/or modify the output of earlier generators. + + + + Generators are run by every manager instance, their output can be different for each + user. + + + + It is recommended to use numerical prefixes for generator names to simplify ordering. + + + + Examples + + + A simple generator that extends an environment variable if a directory exists in the file system + + # 50-xdg-data-dirs.sh + + + + + + A more complicated generator which reads existing configuration and mutates one variable + + # 90-rearrange-path.py + + + + + + Debugging a generator + + SYSTEMD_LOG_LEVEL=debug VAR_A=something VAR_B="something else" \ +&systemenvgeneratordir;/path-to-generator + + + + + + See also + + + systemd.generator7, + systemd1, + systemctl1 + + + diff --git a/man/systemd.generator.xml b/man/systemd.generator.xml index b268104c9d..fb0f0c4da8 100644 --- a/man/systemd.generator.xml +++ b/man/systemd.generator.xml @@ -342,7 +342,8 @@ find $dir systemd-system-update-generator8, systemd-sysv-generator8, systemd.unit5, - systemctl1 + systemctl1, + systemd.environment-generator7 From 6162512cde54244127da547917d0e6d5d0a378c3 Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Wed, 3 Aug 2016 14:35:50 -0400 Subject: [PATCH 18/29] basic: fix strv_env_get_n for unclean arrays If an environment array has duplicates, strv_env_get_n returns the results for the first match. This is wrong, because later entries in the environment are supposed to replace earlier entries. --- src/basic/env-util.c | 2 +- src/test/test-env-util.c | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index a28707fb49..f9208d1475 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -462,7 +462,7 @@ char *strv_env_get_n(char **l, const char *name, size_t k) { if (k <= 0) return NULL; - STRV_FOREACH(i, l) + STRV_FOREACH_BACKWARDS(i, l) if (strneq(*i, name, k) && (*i)[k] == '=') return *i + k + 1; diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 35bb62906e..e004c518fb 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -45,6 +45,16 @@ static void test_strv_env_delete(void) { assert_se(strv_length(d) == 2); } +static void test_strv_env_get(void) { + char **l; + + l = STRV_MAKE("ONE_OR_TWO=1", "THREE=3", "ONE_OR_TWO=2", "FOUR=4"); + + assert_se(streq(strv_env_get(l, "ONE_OR_TWO"), "2")); + assert_se(streq(strv_env_get(l, "THREE"), "3")); + assert_se(streq(strv_env_get(l, "FOUR"), "4")); +} + static void test_strv_env_unset(void) { _cleanup_strv_free_ char **l = NULL; @@ -211,6 +221,7 @@ static void test_env_assignment_is_valid(void) { int main(int argc, char *argv[]) { test_strv_env_delete(); + test_strv_env_get(); test_strv_env_unset(); test_strv_env_set(); test_strv_env_merge(); From d8ad241f54b8c4ac76aafd960d89b47b0ed87fb6 Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Tue, 9 Aug 2016 10:39:15 -0400 Subject: [PATCH 19/29] basic: drop unnecessary strempty() call in replace_env strempty() converts a NULL value to empty string, so that it can be passed on to functions that don't support NULL. replace_env calls strempty before passing its value on to strappend. strappend supports NULL just fine, though, so this commit drops the strempty call. --- src/basic/env-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index f9208d1475..86ac07e1b6 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -563,7 +563,7 @@ char *replace_env(const char *format, char **env) { if (*e == '}') { const char *t; - t = strempty(strv_env_get_n(env, word+2, e-word-2)); + t = strv_env_get_n(env, word+2, e-word-2); k = strappend(r, t); if (!k) From 37f3ffca273e5238794019caede7b7cd33a5de3a Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Thu, 4 Aug 2016 12:00:00 -0400 Subject: [PATCH 20/29] basic: add new merge_env_file function merge_env_file is a new function, that's like load_env_file, but takes a pre-existing environment as an input argument. New environment entries are merged. Variable expansion is performed. Falling back to the process environment is supported (when a flag is set). Alternatively this could be implemented as passing an additional fallback environment array, but later on we're adding another flag to allow braceless expansion, and the two flags can be combined in one arg, so there's less stuff to pass around. --- src/basic/env-util.c | 17 +++++++++---- src/basic/env-util.h | 8 +++++-- src/basic/fileio.c | 28 ++++++++++++++++++++++ src/basic/fileio.h | 2 ++ src/test/test-env-util.c | 31 ++++++++++++++++++++++++ src/test/test-fileio.c | 52 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 131 insertions(+), 7 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 86ac07e1b6..99a130008b 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -454,7 +454,7 @@ fail: return NULL; } -char *strv_env_get_n(char **l, const char *name, size_t k) { +char *strv_env_get_n(char **l, const char *name, size_t k, unsigned flags) { char **i; assert(name); @@ -467,13 +467,20 @@ char *strv_env_get_n(char **l, const char *name, size_t k) { (*i)[k] == '=') return *i + k + 1; + if (flags & REPLACE_ENV_USE_ENVIRONMENT) { + const char *t; + + t = strndupa(name, k); + return getenv(t); + }; + return NULL; } char *strv_env_get(char **l, const char *name) { assert(name); - return strv_env_get_n(l, name, strlen(name)); + return strv_env_get_n(l, name, strlen(name), 0); } char **strv_env_clean_with_callback(char **e, void (*invalid_callback)(const char *p, void *userdata), void *userdata) { @@ -512,7 +519,7 @@ char **strv_env_clean_with_callback(char **e, void (*invalid_callback)(const cha return e; } -char *replace_env(const char *format, char **env) { +char *replace_env(const char *format, char **env, unsigned flags) { enum { WORD, CURLY, @@ -563,7 +570,7 @@ char *replace_env(const char *format, char **env) { if (*e == '}') { const char *t; - t = strv_env_get_n(env, word+2, e-word-2); + t = strv_env_get_n(env, word+2, e-word-2, flags); k = strappend(r, t); if (!k) @@ -643,7 +650,7 @@ char **replace_env_argv(char **argv, char **env) { } /* If ${FOO} appears as part of a word, replace it by the variable as-is */ - ret[k] = replace_env(*i, env); + ret[k] = replace_env(*i, env, 0); if (!ret[k]) { strv_free(ret); return NULL; diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 90df5b1cd9..4e83dcb43a 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -29,7 +29,11 @@ bool env_name_is_valid(const char *e); bool env_value_is_valid(const char *e); bool env_assignment_is_valid(const char *e); -char *replace_env(const char *format, char **env); +enum { + REPLACE_ENV_USE_ENVIRONMENT = 1u, +}; + +char *replace_env(const char *format, char **env, unsigned flags); char **replace_env_argv(char **argv, char **env); bool strv_env_is_valid(char **e); @@ -47,7 +51,7 @@ char **strv_env_unset(char **l, const char *p); /* In place ... */ char **strv_env_unset_many(char **l, ...) _sentinel_; int strv_env_replace(char ***l, char *p); /* In place ... */ -char *strv_env_get_n(char **l, const char *name, size_t k) _pure_; +char *strv_env_get_n(char **l, const char *name, size_t k, unsigned flags) _pure_; char *strv_env_get(char **x, const char *n) _pure_; int getenv_bool(const char *p); diff --git a/src/basic/fileio.c b/src/basic/fileio.c index a1e4978125..49dd52bfd9 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -762,6 +762,34 @@ int load_env_file_pairs(FILE *f, const char *fname, const char *newline, char ** return 0; } +static int merge_env_file_push( + const char *filename, unsigned line, + const char *key, char *value, + void *userdata, + int *n_pushed) { + + char ***env = userdata; + char *expanded_value; + + assert(env); + + expanded_value = replace_env(value, *env, REPLACE_ENV_USE_ENVIRONMENT); + if (!expanded_value) + return -ENOMEM; + + free_and_replace(value, expanded_value); + + return load_env_file_push(filename, line, key, value, env, n_pushed); +} + +int merge_env_file( + char ***env, + FILE *f, + const char *fname) { + + return parse_env_file_internal(f, fname, NEWLINE, merge_env_file_push, env, NULL); +} + static void write_env_var(FILE *f, const char *v) { const char *p; diff --git a/src/basic/fileio.h b/src/basic/fileio.h index 64852b15a8..e547614cc4 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -48,6 +48,8 @@ int parse_env_file(const char *fname, const char *separator, ...) _sentinel_; int load_env_file(FILE *f, const char *fname, const char *separator, char ***l); int load_env_file_pairs(FILE *f, const char *fname, const char *separator, char ***l); +int merge_env_file(char ***env, FILE *f, const char *fname); + int write_env_file(const char *fname, char **l); int executable_is_script(const char *path, char **interpreter); diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index e004c518fb..f44cb3d57b 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -112,6 +112,36 @@ static void test_strv_env_merge(void) { assert_se(strv_length(r) == 5); } +static void test_env_strv_get_n(void) { + const char *_env[] = { + "FOO=NO NO NO", + "FOO=BAR BAR", + "BAR=waldo", + "PATH=unset", + NULL + }; + char **env = (char**) _env; + + assert_se(streq(strv_env_get_n(env, "FOO__", 3, 0), "BAR BAR")); + assert_se(streq(strv_env_get_n(env, "FOO__", 3, REPLACE_ENV_USE_ENVIRONMENT), "BAR BAR")); + assert_se(streq(strv_env_get_n(env, "FOO", 3, 0), "BAR BAR")); + assert_se(streq(strv_env_get_n(env, "FOO", 3, REPLACE_ENV_USE_ENVIRONMENT), "BAR BAR")); + + assert_se(streq(strv_env_get_n(env, "PATH__", 4, 0), "unset")); + assert_se(streq(strv_env_get_n(env, "PATH", 4, 0), "unset")); + assert_se(streq(strv_env_get_n(env, "PATH__", 4, REPLACE_ENV_USE_ENVIRONMENT), "unset")); + assert_se(streq(strv_env_get_n(env, "PATH", 4, REPLACE_ENV_USE_ENVIRONMENT), "unset")); + + env[3] = NULL; /* kill our $PATH */ + + assert_se(!strv_env_get_n(env, "PATH__", 4, 0)); + assert_se(!strv_env_get_n(env, "PATH", 4, 0)); + assert_se(streq(strv_env_get_n(env, "PATH__", 4, REPLACE_ENV_USE_ENVIRONMENT), + getenv("PATH"))); + assert_se(streq(strv_env_get_n(env, "PATH", 4, REPLACE_ENV_USE_ENVIRONMENT), + getenv("PATH"))); +} + static void test_replace_env_arg(void) { const char *env[] = { "FOO=BAR BAR", @@ -225,6 +255,7 @@ int main(int argc, char *argv[]) { test_strv_env_unset(); test_strv_env_set(); test_strv_env_merge(); + test_env_strv_get_n(); test_replace_env_arg(); test_env_clean(); test_env_name_is_valid(); diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index a38bb874a9..84f394a713 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -206,6 +206,56 @@ static void test_parse_multiline_env_file(void) { unlink(p); } +static void test_merge_env_file(void) { + char t[] = "/tmp/test-fileio-XXXXXX"; + int fd, r; + FILE *f; + _cleanup_strv_free_ char **a = NULL; + char **i; + + fd = mkostemp_safe(t); + assert_se(fd >= 0); + + log_info("/* %s (%s) */", __func__, t); + + f = fdopen(fd, "w"); + assert_se(f); + + r = write_string_stream(f, + "one=1 \n" + "twelve=${one}2\n" + "twentyone=2${one}\n" + "one=2\n" + "twentytwo=2${one}\n", false); + assert(r >= 0); + + r = merge_env_file(&a, NULL, t); + assert_se(r >= 0); + strv_sort(a); + + STRV_FOREACH(i, a) + log_info("Got: <%s>", *i); + + assert_se(streq(a[0], "one=2")); + assert_se(streq(a[1], "twelve=12")); + assert_se(streq(a[2], "twentyone=21")); + assert_se(streq(a[3], "twentytwo=22")); + assert_se(a[4] == NULL); + + + r = merge_env_file(&a, NULL, t); + assert_se(r >= 0); + strv_sort(a); + + STRV_FOREACH(i, a) + log_info("Got2: <%s>", *i); + + assert_se(streq(a[0], "one=2")); + assert_se(streq(a[1], "twelve=12")); + assert_se(streq(a[2], "twentyone=21")); + assert_se(streq(a[3], "twentytwo=22")); + assert_se(a[4] == NULL); +} static void test_executable_is_script(void) { char t[] = "/tmp/test-executable-XXXXXX"; @@ -557,11 +607,13 @@ static void test_tempfn(void) { } int main(int argc, char *argv[]) { + log_set_max_level(LOG_DEBUG); log_parse_environment(); log_open(); test_parse_env_file(); test_parse_multiline_env_file(); + test_merge_env_file(); test_executable_is_script(); test_status_field(); test_capeff(); From f63c4aabb2f127ce4acdaec59e3f00e3579f8a75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 11 Feb 2017 11:32:17 -0500 Subject: [PATCH 21/29] environment-generator: new generator to peruse environment.d Why the strange name: the prefix is necessary to follow our own advice that environment generators should have numerical prefixes. I also put -d- in the name because otherwise the name was very easy to mistake with systemd.environment-generator. This additional letter clarifies that this on special generator that supports environment.d files. --- .gitignore | 1 + Makefile-man.am | 7 ++ Makefile.am | 10 ++ man/systemd-environment-d-generator.xml | 80 +++++++++++++ man/systemd.environment-generator.xml | 1 + src/environment-d-generator/Makefile | 1 + .../environment-d-generator.c | 107 ++++++++++++++++++ src/shared/path-lookup.c | 3 +- 8 files changed, 208 insertions(+), 2 deletions(-) create mode 100644 man/systemd-environment-d-generator.xml create mode 120000 src/environment-d-generator/Makefile create mode 100644 src/environment-d-generator/environment-d-generator.c diff --git a/.gitignore b/.gitignore index 7b5bb41259..e54c8fa591 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,7 @@ /*.tar.bz2 /*.tar.gz /*.tar.xz +/30-systemd-environment-d-generator /GPATH /GRTAGS /GSYMS diff --git a/Makefile-man.am b/Makefile-man.am index 86ea6afbe7..2413d5b6c8 100644 --- a/Makefile-man.am +++ b/Makefile-man.am @@ -110,6 +110,7 @@ MANPAGES += \ man/systemd-debug-generator.8 \ man/systemd-delta.1 \ man/systemd-detect-virt.1 \ + man/systemd-environment-d-generator.8 \ man/systemd-escape.1 \ man/systemd-fsck@.service.8 \ man/systemd-fstab-generator.8 \ @@ -185,6 +186,7 @@ MANPAGES += \ man/udev_new.3 \ man/udevadm.8 MANPAGES_ALIAS += \ + man/30-systemd-environment-d-generator.8 \ man/SD_ALERT.3 \ man/SD_BUS_ERROR_ACCESS_DENIED.3 \ man/SD_BUS_ERROR_ADDRESS_IN_USE.3 \ @@ -542,6 +544,7 @@ MANPAGES_ALIAS += \ man/udev_ref.3 \ man/udev_unref.3 \ man/user.conf.d.5 +man/30-systemd-environment-d-generator.8: man/systemd-environment-d-generator.8 man/SD_ALERT.3: man/sd-daemon.3 man/SD_BUS_ERROR_ACCESS_DENIED.3: man/sd-bus-errors.3 man/SD_BUS_ERROR_ADDRESS_IN_USE.3: man/sd-bus-errors.3 @@ -899,6 +902,9 @@ man/udev_monitor_unref.3: man/udev_monitor_new_from_netlink.3 man/udev_ref.3: man/udev_new.3 man/udev_unref.3: man/udev_new.3 man/user.conf.d.5: man/systemd-system.conf.5 +man/30-systemd-environment-d-generator.html: man/systemd-environment-d-generator.html + $(html-alias) + man/SD_ALERT.html: man/sd-daemon.html $(html-alias) @@ -2768,6 +2774,7 @@ EXTRA_DIST += \ man/systemd-debug-generator.xml \ man/systemd-delta.xml \ man/systemd-detect-virt.xml \ + man/systemd-environment-d-generator.xml \ man/systemd-escape.xml \ man/systemd-firstboot.xml \ man/systemd-fsck@.service.xml \ diff --git a/Makefile.am b/Makefile.am index 67c433b43a..70bdcf7076 100644 --- a/Makefile.am +++ b/Makefile.am @@ -426,6 +426,9 @@ systemgenerator_PROGRAMS = \ systemd-system-update-generator \ systemd-debug-generator +userenvgenerator_PROGRAMS = \ + 30-systemd-environment-d-generator + dist_bashcompletion_data = \ shell-completion/bash/busctl \ shell-completion/bash/journalctl \ @@ -2817,6 +2820,13 @@ systemd_system_update_generator_SOURCES = \ systemd_system_update_generator_LDADD = \ libsystemd-shared.la +# ------------------------------------------------------------------------------ +30_systemd_environment_d_generator_SOURCES = \ + src/environment-d-generator/environment-d-generator.c + +30_systemd_environment_d_generator_LDADD = \ + libsystemd-shared.la + # ------------------------------------------------------------------------------ if ENABLE_HIBERNATE systemgenerator_PROGRAMS += \ diff --git a/man/systemd-environment-d-generator.xml b/man/systemd-environment-d-generator.xml new file mode 100644 index 0000000000..cc00a5256d --- /dev/null +++ b/man/systemd-environment-d-generator.xml @@ -0,0 +1,80 @@ + + +%entities; +]> + + + + + + systemd-environment-d-generator + systemd + + + + Developer + Zbigniew + Jędrzejewski-Szmek + zbyszek@in.waw.pl + + + + + + systemd-environment-d-generator + 8 + + + + systemd-environment-d-generator + 30-systemd-environment-d-generator + Load variables specified by environment.d + + + + + &userenvgeneratordir;/30-systemd-environment-d-generator + + + + Description + + systemd-environment-d-generator is a + systemd.environment-generator7 + that reads environment configuration specified by + environment.d7 + configuration files and passes it to the + systemd1 + user manager instance. + + + + See Also + + systemd1, + systemctl1, + systemd.environment-generator7, + systemd.generator7 + + + + diff --git a/man/systemd.environment-generator.xml b/man/systemd.environment-generator.xml index e162dfcbae..fedbd60175 100644 --- a/man/systemd.environment-generator.xml +++ b/man/systemd.environment-generator.xml @@ -151,6 +151,7 @@ See also + systemd-environment-d-generator8, systemd.generator7, systemd1, systemctl1 diff --git a/src/environment-d-generator/Makefile b/src/environment-d-generator/Makefile new file mode 120000 index 0000000000..d0b0e8e008 --- /dev/null +++ b/src/environment-d-generator/Makefile @@ -0,0 +1 @@ +../Makefile \ No newline at end of file diff --git a/src/environment-d-generator/environment-d-generator.c b/src/environment-d-generator/environment-d-generator.c new file mode 100644 index 0000000000..2d4c4235e4 --- /dev/null +++ b/src/environment-d-generator/environment-d-generator.c @@ -0,0 +1,107 @@ +/*** + This file is part of systemd. + + Copyright 2017 Zbigniew Jędrzejewski-Szmek + + systemd is free software; you can redistribute it and/or modify it + under the terms of the GNU Lesser General Public License as published by + the Free Software Foundation; either version 2.1 of the License, or + (at your option) any later version. + + systemd is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with systemd; If not, see . +***/ + +#include "sd-path.h" + +#include "conf-files.h" +#include "def.h" +#include "escape.h" +#include "fileio.h" +#include "log.h" +#include "path-lookup.h" + +static int environment_dirs(char ***ret) { + _cleanup_strv_free_ char **dirs = NULL; + _cleanup_free_ char *c = NULL; + int r; + + dirs = strv_split_nulstr(CONF_PATHS_NULSTR("environment.d")); + if (!dirs) + return -ENOMEM; + + /* ~/.config/systemd/environment.d */ + r = sd_path_home(SD_PATH_USER_CONFIGURATION, "environment.d", &c); + if (r < 0) + return r; + + r = strv_extend_front(&dirs, c); + if (r < 0) + return r; + + *ret = dirs; + dirs = NULL; + return 0; +} + +static int load_and_print(void) { + _cleanup_strv_free_ char **dirs = NULL, **files = NULL, **env = NULL; + char **i; + int r; + + r = environment_dirs(&dirs); + if (r < 0) + return r; + + r = conf_files_list_strv(&files, ".conf", NULL, (const char **) dirs); + if (r < 0) + return r; + + /* This will mutate the existing environment, based on the presumption + * that in case of failure, a partial update is better than none. */ + + STRV_FOREACH(i, files) { + r = merge_env_file(&env, NULL, *i); + if (r == -ENOMEM) + return r; + } + + STRV_FOREACH(i, env) { + char *t; + _cleanup_free_ char *q = NULL; + + t = strchr(*i, '='); + assert(t); + + q = shell_maybe_quote(t + 1); + if (!q) + return log_oom(); + + printf("%.*s=%s\n", (int) (t - *i), *i, q); + } + + return 0; +} + +int main(int argc, char *argv[]) { + int r; + + log_parse_environment(); + log_open(); + + if (argc > 1) { + log_error("This program takes no arguments."); + return EXIT_FAILURE; + } + + r = load_and_print(); + if (r < 0) + log_error_errno(r, "Failed to load environment.d: %m"); + + return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS; +} diff --git a/src/shared/path-lookup.c b/src/shared/path-lookup.c index 586ef64e72..fead755f87 100644 --- a/src/shared/path-lookup.c +++ b/src/shared/path-lookup.c @@ -503,8 +503,7 @@ int lookup_paths_init( append = true; } - /* FIXME: empty components in other places should be - * rejected. */ + /* FIXME: empty components in other places should be rejected. */ r = path_split_and_make_absolute(e, &paths); if (r < 0) From 79d615d56c186475089bc617f6d6850d89687e38 Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Fri, 29 Jul 2016 13:52:55 -0400 Subject: [PATCH 22/29] build-sys,man: load /etc/environment and describe the new environment.d syntax Add support for /etc/environment and document the changes to the user manager to automatically import environment *.conf files from: ~/.config/environment.d/ /etc/environment.d/ /run/environment.d/ /usr/local/lib/environment.d/ /usr/lib/environment.d/ /etc/environment --- Makefile-man.am | 2 + Makefile.am | 7 +++ man/environment.d.xml | 111 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+) create mode 100644 man/environment.d.xml diff --git a/Makefile-man.am b/Makefile-man.am index 2413d5b6c8..4249e13ec3 100644 --- a/Makefile-man.am +++ b/Makefile-man.am @@ -11,6 +11,7 @@ MANPAGES += \ man/bootup.7 \ man/busctl.1 \ man/daemon.7 \ + man/environment.d.5 \ man/file-hierarchy.7 \ man/halt.8 \ man/hostname.5 \ @@ -2642,6 +2643,7 @@ EXTRA_DIST += \ man/crypttab.xml \ man/daemon.xml \ man/dnssec-trust-anchors.d.xml \ + man/environment.d.xml \ man/file-hierarchy.xml \ man/halt.xml \ man/hostname.xml \ diff --git a/Makefile.am b/Makefile.am index 70bdcf7076..337a053ead 100644 --- a/Makefile.am +++ b/Makefile.am @@ -68,6 +68,7 @@ catalogstatedir=$(systemdstatedir)/catalog xinitrcdir=$(sysconfdir)/X11/xinit/xinitrc.d # Our own, non-special dirs +environmentdir=$(prefix)/lib/environment.d pkgsysconfdir=$(sysconfdir)/systemd userunitdir=$(prefix)/lib/systemd/user userpresetdir=$(prefix)/lib/systemd/user-preset @@ -308,6 +309,10 @@ endef install-directories-hook: $(MKDIR_P) $(addprefix $(DESTDIR),$(INSTALL_DIRS)) +install-environment-conf-hook: install-directories-hook + $(AM_V_LN)$(LN_S) --relative -f $(DESTDIR)$(sysconfdir)/environment \ + $(DESTDIR)$(environmentdir)/99-environment.conf + install-aliases-hook: set -- $(SYSTEM_UNIT_ALIASES) && \ dir=$(systemunitdir) && $(install-aliases) @@ -340,6 +345,7 @@ install-touch-usr-hook: INSTALL_EXEC_HOOKS += \ install-target-wants-hook \ install-directories-hook \ + install-environment-conf-hook \ install-aliases-hook \ install-touch-usr-hook @@ -6491,6 +6497,7 @@ INSTALL_DIRS += \ endif INSTALL_DIRS += \ + $(environmentdir) \ $(prefix)/lib/modules-load.d \ $(sysconfdir)/modules-load.d \ $(prefix)/lib/systemd/network \ diff --git a/man/environment.d.xml b/man/environment.d.xml new file mode 100644 index 0000000000..4f3e03825a --- /dev/null +++ b/man/environment.d.xml @@ -0,0 +1,111 @@ + + + + + + + + environment.d + systemd + + + + Developer + Ray + Strode + rstrode@redhat.com + + + + + + environment.d + 5 + + + + environment.d + Definition of user session environment + + + + ~/.config/environment.d/*.conf + /etc/environment.d/*.conf + /run/environment.d/*.conf + /usr/lib/environment.d/*.conf + /etc/environment + + + + Description + + The environment.d directories contain a list of "global" environment + variable assignments for the user environment. + systemd-environment-d-generator8 + parses them and updates the environment exported by the systemd user instance to the services it + starts. + + It is recommended to use numerical prefixes for file names to simplify ordering. + + For backwards compatibility, a symlink to /etc/environment is + installed, so this file is also parsed. + + + + + + Configuration Format + + The configuration files contain a list of + KEY=VALUE environment + variable assignments, separated by newlines. The right hand side of these assignments may + reference previously defined environment variables, using the ${OTHER_KEY} + format. No other elements of shell syntax are supported. + + + + Example + + Setup environment to allow access to a program installed in + <filename noindex='true'>/opt/foo</filename> + + /etc/environment.d/60-foo.conf: + + + FOO_DEBUG=force-software-gl,log-verbose + PATH=/opt/foo/bin:${PATH} + LD_LIBRARY_PATH=/opt/foo/lib + XDG_DATA_DIRS=/opt/foo/share:${XDG_DATA_DIRS} + + + + + + + See Also + + systemd1, + systemd-environment-d-generator8, + systemd.environment-generator7 + + + + From cb4499d0056a7c974d7d3695cc355c7e77edc938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 11 Feb 2017 13:22:13 -0500 Subject: [PATCH 23/29] basic/env-util: use _cleanup_ in replace_env() --- src/basic/env-util.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 99a130008b..8774a81531 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -527,7 +527,8 @@ char *replace_env(const char *format, char **env, unsigned flags) { } state = WORD; const char *e, *word = format; - char *r = NULL, *k; + char *k; + _cleanup_free_ char *r = NULL; assert(format); @@ -544,7 +545,7 @@ char *replace_env(const char *format, char **env, unsigned flags) { if (*e == '{') { k = strnappend(r, word, e-word-1); if (!k) - goto fail; + return NULL; free(r); r = k; @@ -555,7 +556,7 @@ char *replace_env(const char *format, char **env, unsigned flags) { } else if (*e == '$') { k = strnappend(r, word, e-word); if (!k) - goto fail; + return NULL; free(r); r = k; @@ -574,7 +575,7 @@ char *replace_env(const char *format, char **env, unsigned flags) { k = strappend(r, t); if (!k) - goto fail; + return NULL; free(r); r = k; @@ -586,15 +587,7 @@ char *replace_env(const char *format, char **env, unsigned flags) { } } - k = strnappend(r, word, e-word); - if (!k) - goto fail; - - free(r); - return k; - -fail: - return mfree(r); + return strnappend(r, word, e-word); } char **replace_env_argv(char **argv, char **env) { From ccad1fd07ce4eb40a2fcf81cfb55d9b41fdcac48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 11 Feb 2017 14:05:10 -0500 Subject: [PATCH 24/29] Allow braceless variables to be expanded (Only in environment.d files.) We have only basic compatibility with shell syntax, but specifying variables without using braces is probably more common, and I think a lot of people would be surprised if this didn't work. --- man/environment.d.xml | 4 ++-- src/basic/env-util.c | 45 ++++++++++++++++++++++++++++++++++++++-- src/basic/env-util.h | 1 + src/basic/fileio.c | 7 ++++++- src/test/test-env-util.c | 31 +++++++++++++++++++++++++-- src/test/test-fileio.c | 14 +++++++++---- 6 files changed, 91 insertions(+), 11 deletions(-) diff --git a/man/environment.d.xml b/man/environment.d.xml index 4f3e03825a..2302992fa5 100644 --- a/man/environment.d.xml +++ b/man/environment.d.xml @@ -78,7 +78,7 @@ KEY=VALUE environment variable assignments, separated by newlines. The right hand side of these assignments may reference previously defined environment variables, using the ${OTHER_KEY} - format. No other elements of shell syntax are supported. + and $OTHER_KEY format. No other elements of shell syntax are supported. @@ -91,7 +91,7 @@ FOO_DEBUG=force-software-gl,log-verbose - PATH=/opt/foo/bin:${PATH} + PATH=/opt/foo/bin:$PATH LD_LIBRARY_PATH=/opt/foo/lib XDG_DATA_DIRS=/opt/foo/share:${XDG_DATA_DIRS} diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 8774a81531..f370854673 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -523,7 +523,8 @@ char *replace_env(const char *format, char **env, unsigned flags) { enum { WORD, CURLY, - VARIABLE + VARIABLE, + VARIABLE_RAW, } state = WORD; const char *e, *word = format; @@ -563,6 +564,18 @@ char *replace_env(const char *format, char **env, unsigned flags) { word = e+1; state = WORD; + + } else if (flags & REPLACE_ENV_ALLOW_BRACELESS && strchr(VALID_CHARS_ENV_NAME, *e)) { + k = strnappend(r, word, e-word-1); + if (!k) + return NULL; + + free(r); + r = k; + + word = e-1; + state = VARIABLE_RAW; + } else state = WORD; break; @@ -584,10 +597,38 @@ char *replace_env(const char *format, char **env, unsigned flags) { state = WORD; } break; + + case VARIABLE_RAW: + assert(flags & REPLACE_ENV_ALLOW_BRACELESS); + + if (!strchr(VALID_CHARS_ENV_NAME, *e)) { + const char *t; + + t = strv_env_get_n(env, word+1, e-word-1, flags); + + k = strappend(r, t); + if (!k) + return NULL; + + free(r); + r = k; + + word = e--; + state = WORD; + } + break; } } - return strnappend(r, word, e-word); + if (state == VARIABLE_RAW) { + const char *t; + + assert(flags & REPLACE_ENV_ALLOW_BRACELESS); + + t = strv_env_get_n(env, word+1, e-word-1, flags); + return strappend(r, t); + } else + return strnappend(r, word, e-word); } char **replace_env_argv(char **argv, char **env) { diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 4e83dcb43a..03bbc6af00 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -31,6 +31,7 @@ bool env_assignment_is_valid(const char *e); enum { REPLACE_ENV_USE_ENVIRONMENT = 1u, + REPLACE_ENV_ALLOW_BRACELESS = 2u, }; char *replace_env(const char *format, char **env, unsigned flags); diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 49dd52bfd9..3c2dab1855 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -773,7 +773,8 @@ static int merge_env_file_push( assert(env); - expanded_value = replace_env(value, *env, REPLACE_ENV_USE_ENVIRONMENT); + expanded_value = replace_env(value, *env, + REPLACE_ENV_USE_ENVIRONMENT|REPLACE_ENV_ALLOW_BRACELESS); if (!expanded_value) return -ENOMEM; @@ -787,6 +788,10 @@ int merge_env_file( FILE *f, const char *fname) { + /* NOTE: this function supports braceful and braceless variable expansions, + * unlike other exported parsing functions. + */ + return parse_env_file_internal(f, fname, NEWLINE, merge_env_file_push, env, NULL); } diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index f44cb3d57b..77a5219d82 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -142,7 +142,32 @@ static void test_env_strv_get_n(void) { getenv("PATH"))); } -static void test_replace_env_arg(void) { +static void test_replace_env(bool braceless) { + const char *env[] = { + "FOO=BAR BAR", + "BAR=waldo", + NULL + }; + _cleanup_free_ char *t = NULL, *s = NULL, *q = NULL, *r = NULL, *p = NULL; + unsigned flags = REPLACE_ENV_ALLOW_BRACELESS*braceless; + + t = replace_env("FOO=$FOO=${FOO}", (char**) env, flags); + assert_se(streq(t, braceless ? "FOO=BAR BAR=BAR BAR" : "FOO=$FOO=BAR BAR")); + + s = replace_env("BAR=$BAR=${BAR}", (char**) env, flags); + assert_se(streq(s, braceless ? "BAR=waldo=waldo" : "BAR=$BAR=waldo")); + + q = replace_env("BARBAR=$BARBAR=${BARBAR}", (char**) env, flags); + assert_se(streq(q, braceless ? "BARBAR==" : "BARBAR=$BARBAR=")); + + q = replace_env("BAR=$BAR$BAR${BAR}${BAR}", (char**) env, flags); + assert_se(streq(q, braceless ? "BAR=waldowaldowaldowaldo" : "BAR=$BAR$BARwaldowaldo")); + + p = replace_env("${BAR}$BAR$BAR", (char**) env, flags); + assert_se(streq(p, braceless ? "waldowaldowaldo" : "waldo$BAR$BAR")); +} + +static void test_replace_env_argv(void) { const char *env[] = { "FOO=BAR BAR", "BAR=waldo", @@ -256,7 +281,9 @@ int main(int argc, char *argv[]) { test_strv_env_set(); test_strv_env_merge(); test_env_strv_get_n(); - test_replace_env_arg(); + test_replace_env(false); + test_replace_env(true); + test_replace_env_argv(); test_env_clean(); test_env_name_is_valid(); test_env_value_is_valid(); diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index 84f394a713..c204cbae22 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -226,7 +226,10 @@ static void test_merge_env_file(void) { "twelve=${one}2\n" "twentyone=2${one}\n" "one=2\n" - "twentytwo=2${one}\n", false); + "twentytwo=2${one}\n" + "xxx_minus_three=$xxx - 3\n" + "xxx=0x$one$one$one\n" + , false); assert(r >= 0); r = merge_env_file(&a, NULL, t); @@ -240,8 +243,9 @@ static void test_merge_env_file(void) { assert_se(streq(a[1], "twelve=12")); assert_se(streq(a[2], "twentyone=21")); assert_se(streq(a[3], "twentytwo=22")); - assert_se(a[4] == NULL); - + assert_se(streq(a[4], "xxx=0x222")); + assert_se(streq(a[5], "xxx_minus_three= - 3")); + assert_se(a[6] == NULL); r = merge_env_file(&a, NULL, t); assert_se(r >= 0); @@ -254,7 +258,9 @@ static void test_merge_env_file(void) { assert_se(streq(a[1], "twelve=12")); assert_se(streq(a[2], "twentyone=21")); assert_se(streq(a[3], "twentytwo=22")); - assert_se(a[4] == NULL); + assert_se(streq(a[4], "xxx=0x222")); + assert_se(streq(a[5], "xxx_minus_three=0x222 - 3")); + assert_se(a[6] == NULL); } static void test_executable_is_script(void) { From 184d19047370652184a44686cf85bf5001bdf413 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 17 Feb 2017 22:56:28 -0500 Subject: [PATCH 25/29] Tighten checking for variable validity In the future we might want to allow additional syntax (for example "unset VAR". But let's check that the data we're getting does not contain anything unexpected. --- man/environment.d.xml | 3 +++ src/basic/exec-util.c | 5 +++++ src/basic/fileio.c | 10 ++++++++++ src/test/test-exec-util.c | 12 ++++++++++-- src/test/test-fileio.c | 40 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 68 insertions(+), 2 deletions(-) diff --git a/man/environment.d.xml b/man/environment.d.xml index 2302992fa5..4022c25c36 100644 --- a/man/environment.d.xml +++ b/man/environment.d.xml @@ -81,6 +81,9 @@ and $OTHER_KEY format. No other elements of shell syntax are supported. + EachKEY must be a valid variable name. Empty lines + and lines beginning with the comment character # are ignored. + Example diff --git a/src/basic/exec-util.c b/src/basic/exec-util.c index 207fac8dc1..aced9e8e3d 100644 --- a/src/basic/exec-util.c +++ b/src/basic/exec-util.c @@ -279,6 +279,11 @@ static int gather_environment_generate(int fd, void *arg) { STRV_FOREACH_PAIR(x, y, new) { char *p; + if (!env_name_is_valid(*x)) { + log_warning("Invalid variable assignment \"%s=...\", ignoring.", *x); + continue; + } + p = strjoin(*x, "=", *y); if (!p) return -ENOMEM; diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 3c2dab1855..8185f67e00 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -773,6 +773,16 @@ static int merge_env_file_push( assert(env); + if (!value) { + log_error("%s:%u: invalid syntax (around \"%s\"), ignoring.", strna(filename), line, key); + return 0; + } + + if (!env_name_is_valid(key)) { + log_error("%s:%u: invalid variable name \"%s\", ignoring.", strna(filename), line, key); + return 0; + } + expanded_value = replace_env(value, *env, REPLACE_ENV_USE_ENVIRONMENT|REPLACE_ENV_ALLOW_BRACELESS); if (!expanded_value) diff --git a/src/test/test-exec-util.c b/src/test/test-exec-util.c index 94fe042aa9..482b0751b9 100644 --- a/src/test/test-exec-util.c +++ b/src/test/test-exec-util.c @@ -305,8 +305,16 @@ static void test_environment_gathering(void) { "echo A=$A:24\n" "echo B=12\n" "echo C=000\n" - "echo C=001\n" /* variable overwriting */ - "echo PATH=$PATH:/no/such/file", /* variable from manager */ + "echo C=001\n" /* variable overwriting */ + /* various invalid entries */ + "echo unset A\n" + "echo unset A=\n" + "echo unset A=B\n" + "echo unset \n" + "echo A B=C\n" + "echo A\n" + /* test variable assignment without newline */ + "echo PATH=$PATH:/no/such/file", /* no newline */ WRITE_STRING_FILE_CREATE) == 0); assert_se(chmod(name, 0755) == 0); diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index c204cbae22..b117335db8 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -263,6 +263,45 @@ static void test_merge_env_file(void) { assert_se(a[6] == NULL); } +static void test_merge_env_file_invalid(void) { + char t[] = "/tmp/test-fileio-XXXXXX"; + int fd, r; + FILE *f; + _cleanup_strv_free_ char **a = NULL; + char **i; + + fd = mkostemp_safe(t); + assert_se(fd >= 0); + + log_info("/* %s (%s) */", __func__, t); + + f = fdopen(fd, "w"); + assert_se(f); + + r = write_string_stream(f, + "unset one \n" + "unset one= \n" + "unset one=1 \n" + "one \n" + "one = \n" + "one two =\n" + "\x20two=\n" + "#comment=comment\n" + ";comment2=comment2\n" + "#\n" + "\n\n" /* empty line */ + , false); + assert(r >= 0); + + r = merge_env_file(&a, NULL, t); + assert_se(r >= 0); + + STRV_FOREACH(i, a) + log_info("Got: <%s>", *i); + + assert_se(strv_isempty(a)); +} + static void test_executable_is_script(void) { char t[] = "/tmp/test-executable-XXXXXX"; int fd, r; @@ -620,6 +659,7 @@ int main(int argc, char *argv[]) { test_parse_env_file(); test_parse_multiline_env_file(); test_merge_env_file(); + test_merge_env_file_invalid(); test_executable_is_script(); test_status_field(); test_capeff(); From 51e76f7cd137bd57c035f96f447c08991226999d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 18 Feb 2017 11:28:12 -0500 Subject: [PATCH 26/29] build-sys: make environment.d support conditional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have ./configure switches for various parts of non-essential functionality, let's add one for this new stuff too. Support for environment generators is not conditional — if you don't want them, just don't install any. --- Makefile.am | 12 ++++++++---- configure.ac | 9 +++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/Makefile.am b/Makefile.am index 337a053ead..96382baffa 100644 --- a/Makefile.am +++ b/Makefile.am @@ -345,13 +345,15 @@ install-touch-usr-hook: INSTALL_EXEC_HOOKS += \ install-target-wants-hook \ install-directories-hook \ - install-environment-conf-hook \ install-aliases-hook \ - install-touch-usr-hook - -INSTALL_EXEC_HOOKS += \ + install-touch-usr-hook \ install-busnames-target-wants-hook +if ENABLE_ENVIRONMENT_D +INSTALL_EXEC_HOOKS += \ + install-environment-conf-hook +endif + # ------------------------------------------------------------------------------ AM_V_M4 = $(AM_V_M4_$(V)) AM_V_M4_ = $(AM_V_M4_$(AM_DEFAULT_VERBOSITY)) @@ -432,8 +434,10 @@ systemgenerator_PROGRAMS = \ systemd-system-update-generator \ systemd-debug-generator +if ENABLE_ENVIRONMENT_D userenvgenerator_PROGRAMS = \ 30-systemd-environment-d-generator +endif dist_bashcompletion_data = \ shell-completion/bash/busctl \ diff --git a/configure.ac b/configure.ac index ab1d17c531..01bbf99382 100644 --- a/configure.ac +++ b/configure.ac @@ -1040,6 +1040,14 @@ if test "x$enable_tmpfiles" != "xno"; then fi AM_CONDITIONAL(ENABLE_TMPFILES, [test "$have_tmpfiles" = "yes"]) +# ------------------------------------------------------------------------------ +have_environment_d=no +AC_ARG_ENABLE(environment-d, AS_HELP_STRING([--disable-environment-d], [disable environment.d support])) +if test "x$enable_environment_d" != "xno"; then + have_environment_d=yes +fi +AM_CONDITIONAL(ENABLE_ENVIRONMENT_D, [test "$have_environment_d" = "yes"]) + # ------------------------------------------------------------------------------ have_sysusers=no AC_ARG_ENABLE(sysusers, AS_HELP_STRING([--disable-sysusers], [disable sysusers support])) @@ -1655,6 +1663,7 @@ AC_MSG_RESULT([ vconsole: ${have_vconsole} quotacheck: ${have_quotacheck} tmpfiles: ${have_tmpfiles} + environment.d: ${have_environment_d} sysusers: ${have_sysusers} firstboot: ${have_firstboot} randomseed: ${have_randomseed} From 4bed076c5f79ce26451ea3d73950d895f630f9a7 Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Tue, 9 Aug 2016 10:20:22 -0400 Subject: [PATCH 27/29] basic: add replace_env_n function It's like replace_env, but lets you pass in a substring. --- src/basic/env-util.c | 6 ++++-- src/basic/env-util.h | 6 +++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index f370854673..1b955ff1d5 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -519,7 +519,7 @@ char **strv_env_clean_with_callback(char **e, void (*invalid_callback)(const cha return e; } -char *replace_env(const char *format, char **env, unsigned flags) { +char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { enum { WORD, CURLY, @@ -530,10 +530,11 @@ char *replace_env(const char *format, char **env, unsigned flags) { const char *e, *word = format; char *k; _cleanup_free_ char *r = NULL; + size_t i; assert(format); - for (e = format; *e; e ++) { + for (e = format, i = 0; *e && i < n; e ++, i ++) { switch (state) { @@ -614,6 +615,7 @@ char *replace_env(const char *format, char **env, unsigned flags) { r = k; word = e--; + i--; state = WORD; } break; diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 03bbc6af00..43a1371f5e 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -34,9 +34,13 @@ enum { REPLACE_ENV_ALLOW_BRACELESS = 2u, }; -char *replace_env(const char *format, char **env, unsigned flags); +char *replace_env_n(const char *format, size_t n, char **env, unsigned flags); char **replace_env_argv(char **argv, char **env); +static inline char *replace_env(const char *format, char **env, unsigned flags) { + return replace_env_n(format, strlen(format), env, flags); +} + bool strv_env_is_valid(char **e); #define strv_env_clean(l) strv_env_clean_with_callback(l, NULL, NULL) char **strv_env_clean_with_callback(char **l, void (*invalid_callback)(const char *p, void *userdata), void *userdata); From b82f58bfe396b395bce3452bc0ba2f972fb01ab8 Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Tue, 9 Aug 2016 10:20:22 -0400 Subject: [PATCH 28/29] basic: support default and alternate values for env expansion Sometimes it's useful to provide a default value during an environment expansion, if the environment variable isn't already set. For instance $XDG_DATA_DIRS is suppose to default to: /usr/local/share/:/usr/share/ if it's not yet set. That means callers wishing to augment XDG_DATA_DIRS need to manually add those two values. This commit changes replace_env to support the following shell compatible default value syntax: XDG_DATA_DIRS=/foo:${XDG_DATA_DIRS:-/usr/local/share/:/usr/share} Likewise, it's useful to provide an alternate value during an environment expansion, if the environment variable isn't already set. For instance, $LD_LIBRARY_PATH will inadvertently search the current working directory if it starts or ends with a colon, so the following is usually wrong: LD_LIBRARY_PATH=/foo/lib:${LD_LIBRARY_PATH} To address that, this changes replace_env to support the following shell compatible alternate value syntax: LD_LIBRARY_PATH=/foo/lib${LD_LIBRARY_PATH:+:${LD_LIBRARY_PATH}} [zj: gate the new syntax under REPLACE_ENV_ALLOW_EXTENDED switch, so existing callers are not modified.] --- man/environment.d.xml | 16 +++++++--- src/basic/env-util.c | 67 ++++++++++++++++++++++++++++++++++++++-- src/basic/env-util.h | 1 + src/basic/fileio.c | 6 ++-- src/test/test-env-util.c | 14 ++++++++- src/test/test-fileio.c | 16 ++++++++-- 6 files changed, 108 insertions(+), 12 deletions(-) diff --git a/man/environment.d.xml b/man/environment.d.xml index 4022c25c36..be7758a2f9 100644 --- a/man/environment.d.xml +++ b/man/environment.d.xml @@ -78,8 +78,16 @@ KEY=VALUE environment variable assignments, separated by newlines. The right hand side of these assignments may reference previously defined environment variables, using the ${OTHER_KEY} - and $OTHER_KEY format. No other elements of shell syntax are supported. - + and $OTHER_KEY format. It is also possible to use + + ${FOO:-DEFAULT_VALUE} + to expand in the same way as ${FOO} unless the + expansion would be empty, in which case it expands to DEFAULT_VALUE, + and use + ${FOO:+ALTERNATE_VALUE} + to expand to ALTERNATE_VALUE as long as + ${FOO} would have expanded to a non-empty value. + No other elements of shell syntax are supported. EachKEY must be a valid variable name. Empty lines and lines beginning with the comment character # are ignored. @@ -95,8 +103,8 @@ FOO_DEBUG=force-software-gl,log-verbose PATH=/opt/foo/bin:$PATH - LD_LIBRARY_PATH=/opt/foo/lib - XDG_DATA_DIRS=/opt/foo/share:${XDG_DATA_DIRS} + LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}/opt/foo/lib + XDG_DATA_DIRS=/opt/foo/share:${XDG_DATA_DIRS:-/usr/local/share/:/usr/share/} diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 1b955ff1d5..2ca64c3301 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -525,12 +525,16 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { CURLY, VARIABLE, VARIABLE_RAW, + TEST, + DEFAULT_VALUE, + ALTERNATE_VALUE, } state = WORD; - const char *e, *word = format; + const char *e, *word = format, *test_value; char *k; _cleanup_free_ char *r = NULL; - size_t i; + size_t i, len; + int nest = 0; assert(format); @@ -554,7 +558,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { word = e-1; state = VARIABLE; - + nest++; } else if (*e == '$') { k = strnappend(r, word, e-word); if (!k) @@ -594,6 +598,63 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { free(r); r = k; + word = e+1; + state = WORD; + } else if (*e == ':') { + if (!(flags & REPLACE_ENV_ALLOW_EXTENDED)) + /* Treat this as unsupported syntax, i.e. do no replacement */ + state = WORD; + else { + len = e-word-2; + state = TEST; + } + } + break; + + case TEST: + if (*e == '-') + state = DEFAULT_VALUE; + else if (*e == '+') + state = ALTERNATE_VALUE; + else { + state = WORD; + break; + } + + test_value = e+1; + break; + + case DEFAULT_VALUE: /* fall through */ + case ALTERNATE_VALUE: + assert(flags & REPLACE_ENV_ALLOW_EXTENDED); + + if (*e == '{') { + nest++; + break; + } + + if (*e != '}') + break; + + nest--; + if (nest == 0) { // || !strchr(e+1, '}')) { + const char *t; + _cleanup_free_ char *v = NULL; + + t = strv_env_get_n(env, word+2, len, flags); + + if (t && state == ALTERNATE_VALUE) + t = v = replace_env_n(test_value, e-test_value, env, flags); + else if (!t && state == DEFAULT_VALUE) + t = v = replace_env_n(test_value, e-test_value, env, flags); + + k = strappend(r, t); + if (!k) + return NULL; + + free(r); + r = k; + word = e+1; state = WORD; } diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 43a1371f5e..e88fa6aac0 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -32,6 +32,7 @@ bool env_assignment_is_valid(const char *e); enum { REPLACE_ENV_USE_ENVIRONMENT = 1u, REPLACE_ENV_ALLOW_BRACELESS = 2u, + REPLACE_ENV_ALLOW_EXTENDED = 4u, }; char *replace_env_n(const char *format, size_t n, char **env, unsigned flags); diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 8185f67e00..b9a9f74892 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -784,7 +784,9 @@ static int merge_env_file_push( } expanded_value = replace_env(value, *env, - REPLACE_ENV_USE_ENVIRONMENT|REPLACE_ENV_ALLOW_BRACELESS); + REPLACE_ENV_USE_ENVIRONMENT| + REPLACE_ENV_ALLOW_BRACELESS| + REPLACE_ENV_ALLOW_EXTENDED); if (!expanded_value) return -ENOMEM; @@ -799,7 +801,7 @@ int merge_env_file( const char *fname) { /* NOTE: this function supports braceful and braceless variable expansions, - * unlike other exported parsing functions. + * plus "extended" substitutions, unlike other exported parsing functions. */ return parse_env_file_internal(f, fname, NEWLINE, merge_env_file_push, env, NULL); diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 77a5219d82..dfcd9cb724 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -185,6 +185,12 @@ static void test_replace_env_argv(void) { "${FOO", "FOO$$${FOO}", "$$FOO${FOO}", + "${FOO:-${BAR}}", + "${QUUX:-${FOO}}", + "${FOO:+${BAR}}", + "${QUUX:+${BAR}}", + "${FOO:+|${BAR}|}}", + "${FOO:+|${BAR}{|}", NULL }; _cleanup_strv_free_ char **r = NULL; @@ -202,7 +208,13 @@ static void test_replace_env_argv(void) { assert_se(streq(r[8], "${FOO")); assert_se(streq(r[9], "FOO$BAR BAR")); assert_se(streq(r[10], "$FOOBAR BAR")); - assert_se(strv_length(r) == 11); + assert_se(streq(r[11], "${FOO:-waldo}")); + assert_se(streq(r[12], "${QUUX:-BAR BAR}")); + assert_se(streq(r[13], "${FOO:+waldo}")); + assert_se(streq(r[14], "${QUUX:+waldo}")); + assert_se(streq(r[15], "${FOO:+|waldo|}}")); + assert_se(streq(r[16], "${FOO:+|waldo{|}")); + assert_se(strv_length(r) == 17); } static void test_env_clean(void) { diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index b117335db8..b1d688c89e 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -229,6 +229,10 @@ static void test_merge_env_file(void) { "twentytwo=2${one}\n" "xxx_minus_three=$xxx - 3\n" "xxx=0x$one$one$one\n" + "yyy=${one:-fallback}\n" + "zzz=${one:+replacement}\n" + "zzzz=${foobar:-${nothing}}\n" + "zzzzz=${nothing:+${nothing}}\n" , false); assert(r >= 0); @@ -245,7 +249,11 @@ static void test_merge_env_file(void) { assert_se(streq(a[3], "twentytwo=22")); assert_se(streq(a[4], "xxx=0x222")); assert_se(streq(a[5], "xxx_minus_three= - 3")); - assert_se(a[6] == NULL); + assert_se(streq(a[6], "yyy=2")); + assert_se(streq(a[7], "zzz=replacement")); + assert_se(streq(a[8], "zzzz=")); + assert_se(streq(a[9], "zzzzz=")); + assert_se(a[10] == NULL); r = merge_env_file(&a, NULL, t); assert_se(r >= 0); @@ -260,7 +268,11 @@ static void test_merge_env_file(void) { assert_se(streq(a[3], "twentytwo=22")); assert_se(streq(a[4], "xxx=0x222")); assert_se(streq(a[5], "xxx_minus_three=0x222 - 3")); - assert_se(a[6] == NULL); + assert_se(streq(a[6], "yyy=2")); + assert_se(streq(a[7], "zzz=replacement")); + assert_se(streq(a[8], "zzzz=")); + assert_se(streq(a[9], "zzzzz=")); + assert_se(a[10] == NULL); } static void test_merge_env_file_invalid(void) { From f50ce8fc4b2619d73b3118ea202b112035e713c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Feb 2017 23:21:26 -0500 Subject: [PATCH 29/29] test-env-util: add more tests for "extended syntax" This is only the tip of the iceberg. It would be great to test all kinds of nesting, handling of invalid syntax, etc., but I'm leaving that for later. --- src/test/test-env-util.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index dfcd9cb724..4f44cf8711 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -167,6 +167,34 @@ static void test_replace_env(bool braceless) { assert_se(streq(p, braceless ? "waldowaldowaldo" : "waldo$BAR$BAR")); } +static void test_replace_env2(bool extended) { + const char *env[] = { + "FOO=foo", + "BAR=bar", + NULL + }; + _cleanup_free_ char *t = NULL, *s = NULL, *q = NULL, *r = NULL, *p = NULL, *x = NULL; + unsigned flags = REPLACE_ENV_ALLOW_EXTENDED*extended; + + t = replace_env("FOO=${FOO:-${BAR}}", (char**) env, flags); + assert_se(streq(t, extended ? "FOO=foo" : "FOO=${FOO:-bar}")); + + s = replace_env("BAR=${XXX:-${BAR}}", (char**) env, flags); + assert_se(streq(s, extended ? "BAR=bar" : "BAR=${XXX:-bar}")); + + q = replace_env("XXX=${XXX:+${BAR}}", (char**) env, flags); + assert_se(streq(q, extended ? "XXX=" : "XXX=${XXX:+bar}")); + + r = replace_env("FOO=${FOO:+${BAR}}", (char**) env, flags); + assert_se(streq(r, extended ? "FOO=bar" : "FOO=${FOO:+bar}")); + + p = replace_env("FOO=${FOO:-${BAR}post}", (char**) env, flags); + assert_se(streq(p, extended ? "FOO=foo" : "FOO=${FOO:-barpost}")); + + x = replace_env("XXX=${XXX:+${BAR}post}", (char**) env, flags); + assert_se(streq(x, extended ? "XXX=" : "XXX=${XXX:+barpost}")); +} + static void test_replace_env_argv(void) { const char *env[] = { "FOO=BAR BAR", @@ -295,6 +323,8 @@ int main(int argc, char *argv[]) { test_env_strv_get_n(); test_replace_env(false); test_replace_env(true); + test_replace_env2(false); + test_replace_env2(true); test_replace_env_argv(); test_env_clean(); test_env_name_is_valid();