From a896a747c3468e448b08c21cb0c57cb4b7b94f65 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sep 14 2020 15:59:27 +0000 Subject: Apply patches to make test-path pass --- diff --git a/0001-Revert-test-path-increase-timeout.patch b/0001-Revert-test-path-increase-timeout.patch index a9c226f..74684f2 100644 --- a/0001-Revert-test-path-increase-timeout.patch +++ b/0001-Revert-test-path-increase-timeout.patch @@ -1,7 +1,7 @@ From a73d30081a13eaeffce87f997726a179ec44d817 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 31 Jul 2020 10:50:37 +0200 -Subject: [PATCH 1/2] Revert "test-path: increase timeout" +Subject: [PATCH 1/4] Revert "test-path: increase timeout" This partially reverts commit 500727c220354b81b68ed6667d9a6f0fafe3ba19. diff --git a/0002-test-path-do-not-fail-the-test-if-we-fail-to-start-s.patch b/0002-test-path-do-not-fail-the-test-if-we-fail-to-start-s.patch deleted file mode 100644 index c285891..0000000 --- a/0002-test-path-do-not-fail-the-test-if-we-fail-to-start-s.patch +++ /dev/null @@ -1,53 +0,0 @@ -From a2deeaeaa90d493ef8a2b20656745cd0531a1b30 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/2] test-path: do not fail the test if we fail to start some - service - -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 - -Let's just ignore the failure here. Services can occasionally fail to start, -there's not much we can do in that case. ---- - src/test/test-path.c | 8 ++++++++ - 1 file changed, 8 insertions(+) - -diff --git a/src/test/test-path.c b/src/test/test-path.c -index 63b709c8da..6c0db53f10 100644 ---- a/src/test/test-path.c -+++ b/src/test/test-path.c -@@ -98,6 +98,14 @@ static void check_states(Manager *m, Path *path, Service *service, PathState pat - service_state_to_string(service->state), - service_result_to_string(service->result)); - -+ if (service->state == SERVICE_FAILED) { -+ log_warning("Failed to start service %s, ignoring: %s/%s", -+ UNIT(service)->id, -+ service_state_to_string(service->state), -+ service_result_to_string(service->result)); -+ break; -+ } -+ - if (now(CLOCK_MONOTONIC) >= end) { - log_error("Test timeout when testing %s", UNIT(path)->id); - exit(EXIT_FAILURE); diff --git a/0002-test-path-more-debugging-information.patch b/0002-test-path-more-debugging-information.patch new file mode 100644 index 0000000..6aef2dd --- /dev/null +++ b/0002-test-path-more-debugging-information.patch @@ -0,0 +1,78 @@ +From 4c38dcdc8d8f22dddc521faedad6a4f45fa81d63 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 2/4] 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 63b709c8da..84dcf5e37d 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) + 2 * 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 %d: %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 %d: %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"; diff --git a/0003-test-path-do-not-fail-the-test-if-we-fail-to-start-s.patch b/0003-test-path-do-not-fail-the-test-if-we-fail-to-start-s.patch new file mode 100644 index 0000000..571d85c --- /dev/null +++ b/0003-test-path-do-not-fail-the-test-if-we-fail-to-start-s.patch @@ -0,0 +1,245 @@ +From 67c6ff720796bc97f262ba93c6ea87da93b04a1a 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 3/4] test-path: do not fail the test if we fail to start some + service + +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/test/test-path.c | 88 ++++++++++++++++++++++++++++++-------------- + 1 file changed, 61 insertions(+), 27 deletions(-) + +diff --git a/src/test/test-path.c b/src/test/test-path.c +index 84dcf5e37d..d6c37b77e6 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,20 @@ static void _check_states(unsigned line, + service_state_to_string(service->state), + service_result_to_string(service->result)); + ++ if (service->state == SERVICE_FAILED) ++ 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 +133,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 +167,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 +202,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 +244,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 +285,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 +316,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); + } diff --git a/0004-test-path-use-Type-exec.patch b/0004-test-path-use-Type-exec.patch new file mode 100644 index 0000000..3734dc6 --- /dev/null +++ b/0004-test-path-use-Type-exec.patch @@ -0,0 +1,94 @@ +From 1a83d7234e374e991235f4ef21c56998f93cb875 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 4/4] 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 fb465d76bb..b75552df4f 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 fb465d76bb..b75552df4f 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 fb465d76bb..b75552df4f 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 fb465d76bb..b75552df4f 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 fb465d76bb..b75552df4f 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 fb465d76bb..b75552df4f 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 bcdafe4f30..8fbc40d13f 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 diff --git a/systemd.spec b/systemd.spec index c3e29f1..9b156a4 100644 --- a/systemd.spec +++ b/systemd.spec @@ -71,10 +71,12 @@ GIT_DIR=../../src/systemd/.git git diffab -M v233..master@{2017-06-15} -- hwdb/[ Patch0001: use-bfq-scheduler.patch Patch0002: 0001-Revert-test-path-increase-timeout.patch -Patch0003: 0002-test-path-do-not-fail-the-test-if-we-fail-to-start-s.patch +Patch0003: 0002-test-path-more-debugging-information.patch +Patch0004: 0003-test-path-do-not-fail-the-test-if-we-fail-to-start-s.patch +Patch0005: 0004-test-path-use-Type-exec.patch -Patch0004: 0001-test-acl-util-output-more-debug-info.patch -Patch0005: 0001-Do-not-assert-in-test_add_acls_for_user.patch +Patch0006: 0001-test-acl-util-output-more-debug-info.patch +Patch0007: 0001-Do-not-assert-in-test_add_acls_for_user.patch %ifarch %{ix86} x86_64 aarch64 %global have_gnu_efi 1