From 75edb0b0d60a949b5804f43a0baa4071251cf4af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 Sep 2020 08:56:28 +0200 Subject: [PATCH 1/9] test-path: more debugging information MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Just to make it easier to grok what happens when test-path fails. Change printf→log_info so that output is interleaved and not split in two independent parts in log files. --- src/test/test-path.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/test/test-path.c b/src/test/test-path.c index cf89d89482f..f0ebf07e4c0 100644 --- a/src/test/test-path.c +++ b/src/test/test-path.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include @@ -78,32 +77,38 @@ static Service *service_for_path(Manager *m, Path *path, const char *service_nam return SERVICE(service_unit); } -static void check_states(Manager *m, Path *path, Service *service, PathState path_state, ServiceState service_state) { +static void _check_states(unsigned line, + Manager *m, Path *path, Service *service, PathState path_state, ServiceState service_state) { assert_se(m); assert_se(service); usec_t end = now(CLOCK_MONOTONIC) + 30 * USEC_PER_SEC; - while (path->result != PATH_SUCCESS || service->result != SERVICE_SUCCESS || - path->state != path_state || service->state != service_state) { + while (path->state != path_state || service->state != service_state || + path->result != PATH_SUCCESS || service->result != SERVICE_SUCCESS) { assert_se(sd_event_run(m->event, 100 * USEC_PER_MSEC) >= 0); - printf("%s: state = %s; result = %s \n", - UNIT(path)->id, - path_state_to_string(path->state), - path_result_to_string(path->result)); - printf("%s: state = %s; result = %s \n", - UNIT(service)->id, - service_state_to_string(service->state), - service_result_to_string(service->result)); + usec_t n = now(CLOCK_MONOTONIC); + log_info("line %u: %s: state = %s; result = %s (left: %" PRIi64 ")", + line, + UNIT(path)->id, + path_state_to_string(path->state), + path_result_to_string(path->result), + end - n); + log_info("line %u: %s: state = %s; result = %s", + line, + UNIT(service)->id, + service_state_to_string(service->state), + service_result_to_string(service->result)); - if (now(CLOCK_MONOTONIC) >= end) { + if (n >= end) { log_error("Test timeout when testing %s", UNIT(path)->id); exit(EXIT_FAILURE); } } } +#define check_states(...) _check_states(__LINE__, __VA_ARGS__) static void test_path_exists(Manager *m) { const char *test_path = "/tmp/test-path_exists"; From fcb7138ca7c5649aa3c0fe780db20ebae08b2cde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 31 Jul 2020 10:36:57 +0200 Subject: [PATCH 2/9] test-path: do not fail the test if we fail to start a service because of cgroup setup The test was failing because it couldn't start the service: path-modified.service: state = failed; result = exit-code path-modified.path: state = waiting; result = success path-modified.service: state = failed; result = exit-code path-modified.path: state = waiting; result = success path-modified.service: state = failed; result = exit-code path-modified.path: state = waiting; result = success path-modified.service: state = failed; result = exit-code path-modified.path: state = waiting; result = success path-modified.service: state = failed; result = exit-code path-modified.path: state = waiting; result = success path-modified.service: state = failed; result = exit-code Failed to connect to system bus: No such file or directory -.slice: Failed to enable/disable controllers on cgroup /system.slice/kojid.service, ignoring: Permission denied path-modified.service: Failed to create cgroup /system.slice/kojid.service/path-modified.service: Permission denied path-modified.service: Failed to attach to cgroup /system.slice/kojid.service/path-modified.service: No such file or directory path-modified.service: Failed at step CGROUP spawning /bin/true: No such file or directory path-modified.service: Main process exited, code=exited, status=219/CGROUP path-modified.service: Failed with result 'exit-code'. Test timeout when testing path-modified.path In fact any of the services that we try to start may fail, especially considering that we're doing some rogue cgroup operations. See https://github.com/systemd/systemd/pull/16603#issuecomment-679133641. --- src/core/execute.h | 2 +- src/test/test-path.c | 92 +++++++++++++++++++++++++++++++------------- 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/src/core/execute.h b/src/core/execute.h index c4345005c1f..12ea849e1cd 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -87,7 +87,7 @@ struct ExecStatus { dual_timestamp exit_timestamp; pid_t pid; int code; /* as in siginfo_t::si_code */ - int status; /* as in sigingo_t::si_status */ + int status; /* as in siginfo_t::si_status */ }; /* Stores information about commands we execute. Covers both configuration settings as well as runtime data. */ diff --git a/src/test/test-path.c b/src/test/test-path.c index f0ebf07e4c0..60f7672a39b 100644 --- a/src/test/test-path.c +++ b/src/test/test-path.c @@ -77,8 +77,8 @@ static Service *service_for_path(Manager *m, Path *path, const char *service_nam return SERVICE(service_unit); } -static void _check_states(unsigned line, - Manager *m, Path *path, Service *service, PathState path_state, ServiceState service_state) { +static int _check_states(unsigned line, + Manager *m, Path *path, Service *service, PathState path_state, ServiceState service_state) { assert_se(m); assert_se(service); @@ -102,11 +102,24 @@ static void _check_states(unsigned line, service_state_to_string(service->state), service_result_to_string(service->result)); + if (service->state == SERVICE_FAILED && + service->main_exec_status.status == EXIT_CGROUP) + /* On a general purpose system we may fail to start the service for reasons which are + * not under our control: permission limits, resource exhaustion, etc. Let's skip the + * test in those cases. */ + return log_notice_errno(SYNTHETIC_ERRNO(ECANCELED), + "Failed to start service %s, aborting test: %s/%s", + UNIT(service)->id, + service_state_to_string(service->state), + service_result_to_string(service->result)); + if (n >= end) { log_error("Test timeout when testing %s", UNIT(path)->id); exit(EXIT_FAILURE); } } + + return 0; } #define check_states(...) _check_states(__LINE__, __VA_ARGS__) @@ -124,18 +137,22 @@ static void test_path_exists(Manager *m) { service = service_for_path(m, path, NULL); assert_se(unit_start(unit) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(touch(test_path) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; /* Service restarts if file still exists */ assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; assert_se(rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(unit_stop(unit) >= 0); } @@ -154,18 +171,22 @@ static void test_path_existsglob(Manager *m) { service = service_for_path(m, path, NULL); assert_se(unit_start(unit) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(touch(test_path) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; /* Service restarts if file still exists */ assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; assert_se(rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(unit_stop(unit) >= 0); } @@ -185,23 +206,28 @@ static void test_path_changed(Manager *m) { service = service_for_path(m, path, NULL); assert_se(unit_start(unit) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(touch(test_path) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; /* Service does not restart if file still exists */ assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; f = fopen(test_path, "w"); assert_se(f); fclose(f); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; (void) rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL); assert_se(unit_stop(unit) >= 0); @@ -222,23 +248,28 @@ static void test_path_modified(Manager *m) { service = service_for_path(m, path, NULL); assert_se(unit_start(unit) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(touch(test_path) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; /* Service does not restart if file still exists */ assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; f = fopen(test_path, "w"); assert_se(f); fputs("test", f); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; (void) rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL); assert_se(unit_stop(unit) >= 0); @@ -258,14 +289,17 @@ static void test_path_unit(Manager *m) { service = service_for_path(m, path, "path-mycustomunit.service"); assert_se(unit_start(unit) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(touch(test_path) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; assert_se(rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(unit_stop(unit) >= 0); } @@ -286,22 +320,26 @@ static void test_path_directorynotempty(Manager *m) { assert_se(access(test_path, F_OK) < 0); assert_se(unit_start(unit) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; /* MakeDirectory default to no */ assert_se(access(test_path, F_OK) < 0); assert_se(mkdir_p(test_path, 0755) >= 0); assert_se(touch(strjoina(test_path, "test_file")) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; /* Service restarts if directory is still not empty */ assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING); + if (check_states(m, path, service, PATH_RUNNING, SERVICE_RUNNING) < 0) + return; assert_se(rm_rf(test_path, REMOVE_ROOT|REMOVE_PHYSICAL) == 0); assert_se(unit_stop(UNIT(service)) >= 0); - check_states(m, path, service, PATH_WAITING, SERVICE_DEAD); + if (check_states(m, path, service, PATH_WAITING, SERVICE_DEAD) < 0) + return; assert_se(unit_stop(unit) >= 0); } From 333d102c644e7539b071ff5409d5a5e2dab35d65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 Sep 2020 08:58:54 +0200 Subject: [PATCH 3/9] test-path: use Type=exec In general, Type=exec is superior to Type=simple. Let's not assume that the service is started before it was really started. --- test/test-path/path-changed.service | 2 +- test/test-path/path-directorynotempty.service | 2 +- test/test-path/path-exists.service | 2 +- test/test-path/path-existsglob.service | 2 +- test/test-path/path-makedirectory.service | 2 +- test/test-path/path-modified.service | 2 +- test/test-path/path-mycustomunit.service | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/test-path/path-changed.service b/test/test-path/path-changed.service index fb465d76bbc..b75552df4f8 100644 --- a/test/test-path/path-changed.service +++ b/test/test-path/path-changed.service @@ -3,5 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=simple +Type=exec RemainAfterExit=true diff --git a/test/test-path/path-directorynotempty.service b/test/test-path/path-directorynotempty.service index fb465d76bbc..b75552df4f8 100644 --- a/test/test-path/path-directorynotempty.service +++ b/test/test-path/path-directorynotempty.service @@ -3,5 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=simple +Type=exec RemainAfterExit=true diff --git a/test/test-path/path-exists.service b/test/test-path/path-exists.service index fb465d76bbc..b75552df4f8 100644 --- a/test/test-path/path-exists.service +++ b/test/test-path/path-exists.service @@ -3,5 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=simple +Type=exec RemainAfterExit=true diff --git a/test/test-path/path-existsglob.service b/test/test-path/path-existsglob.service index fb465d76bbc..b75552df4f8 100644 --- a/test/test-path/path-existsglob.service +++ b/test/test-path/path-existsglob.service @@ -3,5 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=simple +Type=exec RemainAfterExit=true diff --git a/test/test-path/path-makedirectory.service b/test/test-path/path-makedirectory.service index fb465d76bbc..b75552df4f8 100644 --- a/test/test-path/path-makedirectory.service +++ b/test/test-path/path-makedirectory.service @@ -3,5 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=simple +Type=exec RemainAfterExit=true diff --git a/test/test-path/path-modified.service b/test/test-path/path-modified.service index fb465d76bbc..b75552df4f8 100644 --- a/test/test-path/path-modified.service +++ b/test/test-path/path-modified.service @@ -3,5 +3,5 @@ Description=Service Test for Path units [Service] ExecStart=/bin/true -Type=simple +Type=exec RemainAfterExit=true diff --git a/test/test-path/path-mycustomunit.service b/test/test-path/path-mycustomunit.service index bcdafe4f307..8fbc40d13fa 100644 --- a/test/test-path/path-mycustomunit.service +++ b/test/test-path/path-mycustomunit.service @@ -3,5 +3,5 @@ Description=Service Test Path Unit [Service] ExecStart=/bin/true -Type=simple +Type=exec RemainAfterExit=true From 67b2edb21f11d7b3bd2b5f8c88ceed4c5194c78a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 Sep 2020 09:01:48 +0200 Subject: [PATCH 4/9] xdg-autostart-generator: use Type=exec We check that the binary exists before writing the service file, but let's also not consider the service started until the fork has happened. This is still relatively new stuff, so we're can change the implementation details like this. --- src/xdg-autostart-generator/xdg-autostart-service.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xdg-autostart-generator/xdg-autostart-service.c b/src/xdg-autostart-generator/xdg-autostart-service.c index 9317e9d0289..da5e79dcff5 100644 --- a/src/xdg-autostart-generator/xdg-autostart-service.c +++ b/src/xdg-autostart-generator/xdg-autostart-service.c @@ -604,7 +604,7 @@ int xdg_autostart_service_generate_unit( fprintf(f, "\n[Service]\n" - "Type=simple\n" + "Type=exec\n" "ExecStart=:%s\n" "Restart=no\n" "TimeoutSec=5s\n" From 28c48f4d78d5c7c8ba0a6a679b09f263387e1831 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 Sep 2020 09:02:36 +0200 Subject: [PATCH 5/9] tests: replace the few remaining Type=simple with Type=exec Except for the places where we explicitly want to test Type=simple, we should use Type=exec. --- test/testsuite-04.units/forever-print-hola.service | 2 +- test/testsuite-11.units/fail-on-restart.service | 2 +- test/units/testsuite-16.service | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/testsuite-04.units/forever-print-hola.service b/test/testsuite-04.units/forever-print-hola.service index a0dc0956886..278145285c1 100644 --- a/test/testsuite-04.units/forever-print-hola.service +++ b/test/testsuite-04.units/forever-print-hola.service @@ -2,5 +2,5 @@ Description=ForeverPrintHola service [Service] -Type=simple +Type=exec ExecStart=sh -x -c 'while :; do printf "Hola\n" || touch /i-lose-my-logs; sleep 1; done' diff --git a/test/testsuite-11.units/fail-on-restart.service b/test/testsuite-11.units/fail-on-restart.service index 9264f151f31..6832cb241c4 100644 --- a/test/testsuite-11.units/fail-on-restart.service +++ b/test/testsuite-11.units/fail-on-restart.service @@ -4,6 +4,6 @@ StartLimitIntervalSec=1m StartLimitBurst=3 [Service] -Type=simple +Type=exec ExecStart=false Restart=always diff --git a/test/units/testsuite-16.service b/test/units/testsuite-16.service index b44baad91a6..34e89ff7527 100644 --- a/test/units/testsuite-16.service +++ b/test/units/testsuite-16.service @@ -13,7 +13,7 @@ StopWhenUnneeded=yes [Service] ExecStartPre=rm -f /failed /testok -Type=simple +Type=exec TimeoutStartSec=infinity ExecStartPre=/usr/lib/systemd/tests/testdata/units/%N.sh ExecStart=true From 4cce6f25241e405cc79498e93a030398d97f177a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 27 Sep 2020 15:33:20 +0200 Subject: [PATCH 6/9] test-path: start infinite sleep instead of a short command The test sometimes fails, e.g. in bionic-s390x ci. I think it might be because the service binary exits before we get a chance to notice that it is running: 13:59:31 --- Listing only the last 100 lines from a long log. --- 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 4639845) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 4539608) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 4439376) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 4338946) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 4238702) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 4138424) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 4038116) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3937835) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3837553) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3737250) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3636934) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3536622) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3436318) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3336021) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3235730) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3135468) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 3035158) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2934855) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2834541) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2732511) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2632255) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2532014) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2431746) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2331438) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2231213) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2130952) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 2030663) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1930428) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1830172) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1729906) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1629652) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1529368) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1429110) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1328852) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1228593) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1128320) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 1028083) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 927824) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 827564) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 724935) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 624664) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 524411) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 424124) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 323853) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 223585) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 120356) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: 18053) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 line 293: path-unit.path: state = running; result = success (left: -82385) 13:59:31 line 293: path-mycustomunit.service: state = exited; result = success 13:59:31 Test timeout when testing path-unit.path It seems test/test-path/path-service.service wasn't actually used for anything. --- test/test-path/path-changed.service | 2 +- test/test-path/path-directorynotempty.service | 2 +- test/test-path/path-exists.service | 2 +- test/test-path/path-existsglob.service | 2 +- test/test-path/path-makedirectory.service | 2 +- test/test-path/path-modified.service | 2 +- test/test-path/path-mycustomunit.service | 2 +- test/test-path/path-service.service | 6 ------ 8 files changed, 7 insertions(+), 13 deletions(-) delete mode 100644 test/test-path/path-service.service diff --git a/test/test-path/path-changed.service b/test/test-path/path-changed.service index b75552df4f8..1246ec2ee73 100644 --- a/test/test-path/path-changed.service +++ b/test/test-path/path-changed.service @@ -2,6 +2,6 @@ Description=Service Test for Path units [Service] -ExecStart=/bin/true +ExecStart=sleep infinity Type=exec RemainAfterExit=true diff --git a/test/test-path/path-directorynotempty.service b/test/test-path/path-directorynotempty.service index b75552df4f8..1246ec2ee73 100644 --- a/test/test-path/path-directorynotempty.service +++ b/test/test-path/path-directorynotempty.service @@ -2,6 +2,6 @@ Description=Service Test for Path units [Service] -ExecStart=/bin/true +ExecStart=sleep infinity Type=exec RemainAfterExit=true diff --git a/test/test-path/path-exists.service b/test/test-path/path-exists.service index b75552df4f8..1246ec2ee73 100644 --- a/test/test-path/path-exists.service +++ b/test/test-path/path-exists.service @@ -2,6 +2,6 @@ Description=Service Test for Path units [Service] -ExecStart=/bin/true +ExecStart=sleep infinity Type=exec RemainAfterExit=true diff --git a/test/test-path/path-existsglob.service b/test/test-path/path-existsglob.service index b75552df4f8..1246ec2ee73 100644 --- a/test/test-path/path-existsglob.service +++ b/test/test-path/path-existsglob.service @@ -2,6 +2,6 @@ Description=Service Test for Path units [Service] -ExecStart=/bin/true +ExecStart=sleep infinity Type=exec RemainAfterExit=true diff --git a/test/test-path/path-makedirectory.service b/test/test-path/path-makedirectory.service index b75552df4f8..1246ec2ee73 100644 --- a/test/test-path/path-makedirectory.service +++ b/test/test-path/path-makedirectory.service @@ -2,6 +2,6 @@ Description=Service Test for Path units [Service] -ExecStart=/bin/true +ExecStart=sleep infinity Type=exec RemainAfterExit=true diff --git a/test/test-path/path-modified.service b/test/test-path/path-modified.service index b75552df4f8..1246ec2ee73 100644 --- a/test/test-path/path-modified.service +++ b/test/test-path/path-modified.service @@ -2,6 +2,6 @@ Description=Service Test for Path units [Service] -ExecStart=/bin/true +ExecStart=sleep infinity Type=exec RemainAfterExit=true diff --git a/test/test-path/path-mycustomunit.service b/test/test-path/path-mycustomunit.service index 8fbc40d13fa..6a9bac09cdc 100644 --- a/test/test-path/path-mycustomunit.service +++ b/test/test-path/path-mycustomunit.service @@ -2,6 +2,6 @@ Description=Service Test Path Unit [Service] -ExecStart=/bin/true +ExecStart=sleep infinity Type=exec RemainAfterExit=true diff --git a/test/test-path/path-service.service b/test/test-path/path-service.service deleted file mode 100644 index f8499ec6195..00000000000 --- a/test/test-path/path-service.service +++ /dev/null @@ -1,6 +0,0 @@ -[Unit] -Description=Service Test for Path units - -[Service] -ExecStart=/bin/true -Type=oneshot From b8ee3493a54794ca1bbd84d62b4d9f7041fcfddc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 21 Oct 2020 10:04:23 +0200 Subject: [PATCH 7/9] meson: convert developer_mode boolean to an enum I initially changed this to add a third state. But even with two values having an explicit name instead of just 0/1 is mode descriptive. --- man/meson.build | 2 +- meson.build | 4 ++-- meson_options.txt | 2 +- src/basic/build.h | 5 +++++ src/basic/missing_capability.h | 2 +- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/man/meson.build b/man/meson.build index 3c2c7023ed0..d056ad0c71f 100644 --- a/man/meson.build +++ b/man/meson.build @@ -207,7 +207,7 @@ if dbus_docs.length() > 0 '@INPUT@'], input : dbus_docs) - if conf.get('DEVELOPER_MODE') == 1 + if conf.get('BUILD_MODE') == 'BUILD_MODE_DEVELOPER' test('dbus-docs-fresh', update_dbus_docs_py, args : ['--build-dir=@0@'.format(project_build_root), diff --git a/meson.build b/meson.build index 7c2d4e8b108..307d1bd5f7c 100644 --- a/meson.build +++ b/meson.build @@ -38,8 +38,8 @@ relative_source_path = run_command('realpath', project_source_root).stdout().strip() conf.set_quoted('RELATIVE_SOURCE_PATH', relative_source_path) -conf.set10('DEVELOPER_MODE', get_option('mode') == 'developer', - description : 'enable additional checks only suitable in development') +conf.set('BUILD_MODE', 'BUILD_MODE_' + get_option('mode').to_upper(), + description : 'tailor build to development or release builds') want_ossfuzz = get_option('oss-fuzz') want_libfuzzer = get_option('llvm-fuzz') diff --git a/meson_options.txt b/meson_options.txt index 59248c70999..2c10054769f 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -5,7 +5,7 @@ option('version-tag', type : 'string', description : 'override the git version string') option('mode', type : 'combo', choices : ['developer', 'release'], - description : 'enable additional checks suitable for systemd development') + description : 'autoenable features suitable for systemd development/release builds') option('split-usr', type : 'combo', choices : ['auto', 'true', 'false'], description : '''/bin, /sbin aren't symlinks into /usr''') diff --git a/src/basic/build.h b/src/basic/build.h index d160af5bc7e..e02ad391a96 100644 --- a/src/basic/build.h +++ b/src/basic/build.h @@ -161,3 +161,8 @@ _IDN_FEATURE_ " " \ _PCRE2_FEATURE_ " " \ _CGROUP_HIERARCHY_ + +enum { + BUILD_MODE_DEVELOPER, + BUILD_MODE_RELEASE, +}; diff --git a/src/basic/missing_capability.h b/src/basic/missing_capability.h index c52cd449339..4d37618741f 100644 --- a/src/basic/missing_capability.h +++ b/src/basic/missing_capability.h @@ -27,7 +27,7 @@ #ifdef CAP_LAST_CAP # if CAP_LAST_CAP > SYSTEMD_CAP_LAST_CAP -# if DEVELOPER_MODE && defined(TEST_CAPABILITY_C) +# if BUILD_MODE == BUILD_MODE_DEVELOPER && defined(TEST_CAPABILITY_C) # warning "The capability list here is outdated" # endif # else From 4eb0c875f8825199a829ddc597874915fbee0a84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 21 Oct 2020 11:29:00 +0200 Subject: [PATCH 8/9] tests: add helper function to autodetect CI environments Sadly there is no standarized way to check if we're running in some CI environment. So let's try to gather the heuristics in one helper function. --- src/basic/string-util.h | 6 ++++++ src/shared/tests.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/shared/tests.h | 3 +++ src/test/test-execute.c | 7 +------ 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/src/basic/string-util.h b/src/basic/string-util.h index cefbda3577b..a68d88c30bc 100644 --- a/src/basic/string-util.h +++ b/src/basic/string-util.h @@ -33,6 +33,12 @@ static inline bool streq_ptr(const char *a, const char *b) { return strcmp_ptr(a, b) == 0; } +static inline char* strstr_ptr(const char *haystack, const char *needle) { + if (!haystack || !needle) + return NULL; + return strstr(haystack, needle); +} + static inline const char* strempty(const char *s) { return s ?: ""; } diff --git a/src/shared/tests.c b/src/shared/tests.c index fe6d9dfbd50..808e2e6040b 100644 --- a/src/shared/tests.c +++ b/src/shared/tests.c @@ -301,3 +301,43 @@ int enter_cgroup_subroot(char **ret_cgroup) { int enter_cgroup_root(char **ret_cgroup) { return enter_cgroup(ret_cgroup, false); } + +const char *ci_environment(void) { + /* We return a string because we might want to provide multiple bits of information later on: not + * just the general CI environment type, but also whether we're sanitizing or not, etc. The caller is + * expected to use strstr on the returned value. */ + static const char *ans = POINTER_MAX; + const char *p; + int r; + + if (ans != POINTER_MAX) + return ans; + + /* We allow specifying the environment with $CITYPE. Nobody uses this so far, but we are ready. */ + p = getenv("CITYPE"); + if (!isempty(p)) + return (ans = p); + + if (getenv_bool("TRAVIS") > 0) + return (ans = "travis"); + if (getenv_bool("SEMAPHORE") > 0) + return (ans = "semaphore"); + if (getenv_bool("GITHUB_ACTIONS") > 0) + return (ans = "github-actions"); + if (getenv("AUTOPKGTEST_ARTIFACTS") || getenv("AUTOPKGTEST_TMP")) + return (ans = "autopkgtest"); + + FOREACH_STRING(p, "CI", "CONTINOUS_INTEGRATION") { + /* Those vars are booleans according to Semaphore and Travis docs: + * https://docs.travis-ci.com/user/environment-variables/#default-environment-variables + * https://docs.semaphoreci.com/ci-cd-environment/environment-variables/#ci + */ + r = getenv_bool(p); + if (r > 0) + return (ans = "unknown"); /* Some other unknown thing */ + if (r == 0) + return (ans = NULL); + } + + return (ans = NULL); +} diff --git a/src/shared/tests.h b/src/shared/tests.h index 505ca39775c..552e0f2c22e 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -40,3 +40,6 @@ bool can_memlock(void); } else { \ printf("systemd not booted skipping '%s'\n", #x); \ } + +/* Provide a convenient way to check if we're running in CI. */ +const char *ci_environment(void); diff --git a/src/test/test-execute.c b/src/test/test-execute.c index c18d68683e1..0bc2a45c303 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -36,11 +36,6 @@ static int cld_dumped_to_killed(int code) { return code == CLD_DUMPED ? CLD_KILLED : code; } -_unused_ static bool is_run_on_travis_ci(void) { - /* https://docs.travis-ci.com/user/environment-variables#default-environment-variables */ - return streq_ptr(getenv("TRAVIS"), "true"); -} - static void wait_for_service_finish(Manager *m, Unit *unit) { Service *service = NULL; usec_t ts; @@ -897,7 +892,7 @@ int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); #if HAS_FEATURE_ADDRESS_SANITIZER - if (is_run_on_travis_ci()) { + if (strstr_ptr(ci_environment(), "travis")) { log_notice("Running on TravisCI under ASan, skipping, see https://github.com/systemd/systemd/issues/10696"); return EXIT_TEST_SKIP; } From 16e925c6bb529dd862110dd5b4d57104ad98400c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 21 Oct 2020 10:07:23 +0200 Subject: [PATCH 9/9] test-path: relax test in "ci" and "release" modes --- src/test/test-path.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/test-path.c b/src/test/test-path.c index 60f7672a39b..2e20674c2f3 100644 --- a/src/test/test-path.c +++ b/src/test/test-path.c @@ -103,10 +103,11 @@ static int _check_states(unsigned line, service_result_to_string(service->result)); if (service->state == SERVICE_FAILED && - service->main_exec_status.status == EXIT_CGROUP) + service->main_exec_status.status == EXIT_CGROUP && + !ci_environment()) /* On a general purpose system we may fail to start the service for reasons which are * not under our control: permission limits, resource exhaustion, etc. Let's skip the - * test in those cases. */ + * test in those cases. On developer machines we require proper setup. */ return log_notice_errno(SYNTHETIC_ERRNO(ECANCELED), "Failed to start service %s, aborting test: %s/%s", UNIT(service)->id,