From 4ef0ac8f500ba97f865e7a9981593fba64da7f25 Mon Sep 17 00:00:00 2001 From: Wen Yang Date: Tue, 18 Feb 2020 20:56:21 +0800 Subject: [PATCH 1/6] test: fix some failures in test-cgroup Fix the following issues in test-cgroup: 1, commit 65be7e06528a ("pid1: do not reset subtree_control on already-existing units with delegation") changed the return value of cg_create () as follows: "Returns 0 if the group already existed, 1 on success, negative otherwise." So we need to modify the test cases related to cg_create (). 2, commit efdb02375beb ("core: unified cgroup hierarchy support") changed cg_delete () to cg_rmdir (), so the test cases also need to be adjusted a bit. 3. There is no cleanup of "test-a". If we execute test-cgroup multiple times, we will encounter an error. Signed-off-by: Wen Yang --- src/test/test-cgroup.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/test/test-cgroup.c b/src/test/test-cgroup.c index f16cb291f2..afcdcdc441 100644 --- a/src/test/test-cgroup.c +++ b/src/test/test-cgroup.c @@ -13,10 +13,10 @@ int main(int argc, char *argv[]) { char *path; char *c, *p; + assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 1); assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0); - assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0); - assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-b") == 0); - assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-b/test-c") == 0); + assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-b") == 1); + assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-b/test-c") == 1); assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0) == 0); assert_se(cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, getpid_cached(), &path) == 0); @@ -29,7 +29,7 @@ int main(int argc, char *argv[]) { assert_se(path_equal(path, "/test-a")); free(path); - assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, "/test-b/test-d", 0) == 0); + assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, "/test-b/test-d", 0) == 1); assert_se(cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, getpid_cached(), &path) == 0); assert_se(path_equal(path, "/test-b/test-d")); @@ -57,8 +57,10 @@ int main(int argc, char *argv[]) { cg_trim(SYSTEMD_CGROUP_CONTROLLER, "/", false); - assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-b") < 0); - assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-a") >= 0); + assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-b") == 0); + assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-a") < 0); + assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", SYSTEMD_CGROUP_CONTROLLER, "system.slice", 0) > 0); + assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0); assert_se(cg_split_spec("foobar:/", &c, &p) == 0); assert_se(streq(c, "foobar")); @@ -66,7 +68,7 @@ int main(int argc, char *argv[]) { free(c); free(p); - assert_se(cg_split_spec("foobar:", &c, &p) < 0); + assert_se(cg_split_spec("foobar:", &c, &p) == 0); assert_se(cg_split_spec("foobar:asdfd", &c, &p) < 0); assert_se(cg_split_spec(":///", &c, &p) < 0); assert_se(cg_split_spec(":", &c, &p) < 0); From b35e9974faaf62d2916ae9d45b835cf26f4efe47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Mar 2020 10:36:01 +0100 Subject: [PATCH 2/6] test-cgroup: split into functions as usual --- src/test/test-cgroup.c | 62 ++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/src/test/test-cgroup.c b/src/test/test-cgroup.c index afcdcdc441..bc4f5260ca 100644 --- a/src/test/test-cgroup.c +++ b/src/test/test-cgroup.c @@ -7,12 +7,42 @@ #include "path-util.h" #include "process-util.h" #include "string-util.h" -#include "util.h" +#include "tests.h" -int main(int argc, char *argv[]) { - char *path; +static void test_cg_split_spec(void) { char *c, *p; + log_info("/* %s */", __func__); + + assert_se(cg_split_spec("foobar:/", &c, &p) == 0); + assert_se(streq(c, "foobar")); + assert_se(streq(p, "/")); + c = mfree(c); + p = mfree(p); + + assert_se(cg_split_spec("foobar:", &c, &p) == 0); + assert_se(cg_split_spec("foobar:asdfd", &c, &p) < 0); + assert_se(cg_split_spec(":///", &c, &p) < 0); + assert_se(cg_split_spec(":", &c, &p) < 0); + assert_se(cg_split_spec("", &c, &p) < 0); + assert_se(cg_split_spec("fo/obar:/", &c, &p) < 0); + + assert_se(cg_split_spec("/", &c, &p) >= 0); + assert_se(c == NULL); + assert_se(streq(p, "/")); + p = mfree(p); + + assert_se(cg_split_spec("foo", &c, &p) >= 0); + assert_se(streq(c, "foo")); + assert_se(p == NULL); + c = mfree(c); +} + +static void test_cg_create(void) { + char *path; + + log_info("/* %s */", __func__); + assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 1); assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0); assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-b") == 1); @@ -61,29 +91,13 @@ int main(int argc, char *argv[]) { assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-a") < 0); assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", SYSTEMD_CGROUP_CONTROLLER, "system.slice", 0) > 0); assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0); +} - assert_se(cg_split_spec("foobar:/", &c, &p) == 0); - assert_se(streq(c, "foobar")); - assert_se(streq(p, "/")); - free(c); - free(p); +int main(int argc, char *argv[]) { + test_setup_logging(LOG_DEBUG); - assert_se(cg_split_spec("foobar:", &c, &p) == 0); - assert_se(cg_split_spec("foobar:asdfd", &c, &p) < 0); - assert_se(cg_split_spec(":///", &c, &p) < 0); - assert_se(cg_split_spec(":", &c, &p) < 0); - assert_se(cg_split_spec("", &c, &p) < 0); - assert_se(cg_split_spec("fo/obar:/", &c, &p) < 0); - - assert_se(cg_split_spec("/", &c, &p) >= 0); - assert_se(c == NULL); - assert_se(streq(p, "/")); - free(p); - - assert_se(cg_split_spec("foo", &c, &p) >= 0); - assert_se(streq(c, "foo")); - assert_se(p == NULL); - free(c); + test_cg_split_spec(); + test_cg_create(); return 0; } From 2a8020fe9dd44ddb59268fbb3df7489421df3b75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Mar 2020 10:50:27 +0100 Subject: [PATCH 3/6] basic/cgroup-util: modernize cg_split_spec() Those cryptic one letter variable names, yuck! --- src/basic/cgroup-util.c | 102 +++++++++++++++------------------------- src/basic/cgroup-util.h | 2 +- 2 files changed, 40 insertions(+), 64 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 54fc6ecf8b..489a155b48 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -878,9 +878,8 @@ int cg_is_empty_recursive(const char *controller, const char *path) { } } -int cg_split_spec(const char *spec, char **controller, char **path) { - char *t = NULL, *u = NULL; - const char *e; +int cg_split_spec(const char *spec, char **ret_controller, char **ret_path) { + _cleanup_free_ char *controller = NULL, *path = NULL; assert(spec); @@ -888,76 +887,53 @@ int cg_split_spec(const char *spec, char **controller, char **path) { if (!path_is_normalized(spec)) return -EINVAL; - if (path) { - t = strdup(spec); - if (!t) + if (ret_path) { + path = strdup(spec); + if (!path) return -ENOMEM; - *path = path_simplify(t, false); + path_simplify(path, false); } - if (controller) - *controller = NULL; + } else { + const char *e; - return 0; - } - - e = strchr(spec, ':'); - if (!e) { - if (!cg_controller_is_valid(spec)) - return -EINVAL; - - if (controller) { - t = strdup(spec); - if (!t) + e = strchr(spec, ':'); + if (e) { + controller = strndup(spec, e-spec); + if (!controller) return -ENOMEM; + if (!cg_controller_is_valid(controller)) + return -EINVAL; - *controller = t; + if (!isempty(e + 1)) { + path = strdup(e+1); + if (!path) + return -ENOMEM; + + if (!path_is_normalized(path) || + !path_is_absolute(path)) + return -EINVAL; + + path_simplify(path, false); + } + + } else { + if (!cg_controller_is_valid(spec)) + return -EINVAL; + + if (ret_controller) { + controller = strdup(spec); + if (!controller) + return -ENOMEM; + } } - - if (path) - *path = NULL; - - return 0; } - t = strndup(spec, e-spec); - if (!t) - return -ENOMEM; - if (!cg_controller_is_valid(t)) { - free(t); - return -EINVAL; - } - - if (isempty(e+1)) - u = NULL; - else { - u = strdup(e+1); - if (!u) { - free(t); - return -ENOMEM; - } - - if (!path_is_normalized(u) || - !path_is_absolute(u)) { - free(t); - free(u); - return -EINVAL; - } - - path_simplify(u, false); - } - - if (controller) - *controller = t; - else - free(t); - - if (path) - *path = u; - else - free(u); - + if (ret_controller) + *ret_controller = TAKE_PTR(controller); + if (ret_path) + *ret_path = TAKE_PTR(path); return 0; } diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index ad16619063..300555f1ac 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -170,7 +170,7 @@ typedef int (*cg_kill_log_func_t)(pid_t pid, int sig, void *userdata); int cg_kill(const char *controller, const char *path, int sig, CGroupFlags flags, Set *s, cg_kill_log_func_t kill_log, void *userdata); int cg_kill_recursive(const char *controller, const char *path, int sig, CGroupFlags flags, Set *s, cg_kill_log_func_t kill_log, void *userdata); -int cg_split_spec(const char *spec, char **controller, char **path); +int cg_split_spec(const char *spec, char **ret_controller, char **ret_path); int cg_mangle_path(const char *path, char **result); int cg_get_path(const char *controller, const char *path, const char *suffix, char **fs); From 1c132196b1164c1e181280d92f8ac52d0311a085 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Mar 2020 10:54:43 +0100 Subject: [PATCH 4/6] test-cgroup: fix memleak https://github.com/systemd/systemd/pull/14901#issuecomment-587924705. --- src/test/test-cgroup.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/test-cgroup.c b/src/test/test-cgroup.c index bc4f5260ca..0971f8461e 100644 --- a/src/test/test-cgroup.c +++ b/src/test/test-cgroup.c @@ -21,6 +21,9 @@ static void test_cg_split_spec(void) { p = mfree(p); assert_se(cg_split_spec("foobar:", &c, &p) == 0); + c = mfree(c); + p = mfree(p); + assert_se(cg_split_spec("foobar:asdfd", &c, &p) < 0); assert_se(cg_split_spec(":///", &c, &p) < 0); assert_se(cg_split_spec(":", &c, &p) < 0); From 67da33231a675969b2afea7eb471418e530e0a4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Mar 2020 11:21:33 +0100 Subject: [PATCH 5/6] test-cgroup: do not require root to pass Nowadays with delegation to the user instance, we can make this work as non-root easily. If we still get access denied, just skip the test. --- src/test/test-cgroup.c | 85 +++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 30 deletions(-) diff --git a/src/test/test-cgroup.c b/src/test/test-cgroup.c index 0971f8461e..853c6c4ab4 100644 --- a/src/test/test-cgroup.c +++ b/src/test/test-cgroup.c @@ -4,6 +4,7 @@ #include "cgroup-setup.h" #include "cgroup-util.h" +#include "errno-util.h" #include "path-util.h" #include "process-util.h" #include "string-util.h" @@ -42,58 +43,82 @@ static void test_cg_split_spec(void) { } static void test_cg_create(void) { - char *path; - log_info("/* %s */", __func__); - assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 1); - assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0); - assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-b") == 1); - assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, "/test-b/test-c") == 1); - assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0) == 0); + _cleanup_free_ char *here = NULL; + assert_se(cg_pid_get_path_shifted(0, NULL, &here) >= 0); + + const char *test_a = prefix_roota(here, "/test-a"), + *test_b = prefix_roota(here, "/test-b"), + *test_c = prefix_roota(here, "/test-b/test-c"), + *test_d = prefix_roota(here, "/test-b/test-d"); + char *path; + int r; + + log_info("Paths for test:\n%s\n%s", test_a, test_b); + + r = cg_create(SYSTEMD_CGROUP_CONTROLLER, test_a); + if (IN_SET(r, -EPERM, -EACCES, -EROFS)) { + log_info_errno(r, "Skipping %s: %m", __func__); + return; + } + + assert_se(r == 1); + assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, test_a) == 0); + assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, test_b) == 1); + assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, test_c) == 1); + assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, test_b, 0) == 0); assert_se(cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, getpid_cached(), &path) == 0); - assert_se(streq(path, "/test-b")); + assert_se(streq(path, test_b)); free(path); - assert_se(cg_attach(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0) == 0); + assert_se(cg_attach(SYSTEMD_CGROUP_CONTROLLER, test_a, 0) == 0); assert_se(cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, getpid_cached(), &path) == 0); - assert_se(path_equal(path, "/test-a")); + assert_se(path_equal(path, test_a)); free(path); - assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, "/test-b/test-d", 0) == 1); + assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, test_d, 0) == 1); assert_se(cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, getpid_cached(), &path) == 0); - assert_se(path_equal(path, "/test-b/test-d")); + assert_se(path_equal(path, test_d)); free(path); - assert_se(cg_get_path(SYSTEMD_CGROUP_CONTROLLER, "/test-b/test-d", NULL, &path) == 0); - assert_se(path_equal(path, "/sys/fs/cgroup/systemd/test-b/test-d")); + assert_se(cg_get_path(SYSTEMD_CGROUP_CONTROLLER, test_d, NULL, &path) == 0); + log_debug("test_d: %s", path); + const char *full_d; + if (cg_all_unified()) + full_d = strjoina("/sys/fs/cgroup", test_d); + else if (cg_hybrid_unified()) + full_d = strjoina("/sys/fs/cgroup/unified", test_d); + else + full_d = strjoina("/sys/fs/cgroup/systemd", test_d); + assert_se(path_equal(path, full_d)); free(path); - assert_se(cg_is_empty(SYSTEMD_CGROUP_CONTROLLER, "/test-a") > 0); - assert_se(cg_is_empty(SYSTEMD_CGROUP_CONTROLLER, "/test-b") > 0); - assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a") > 0); - assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b") == 0); + assert_se(cg_is_empty(SYSTEMD_CGROUP_CONTROLLER, test_a) > 0); + assert_se(cg_is_empty(SYSTEMD_CGROUP_CONTROLLER, test_b) > 0); + assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, test_a) > 0); + assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, test_b) == 0); - assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, 0, NULL, NULL, NULL) == 0); - assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, 0, NULL, NULL, NULL) > 0); + assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, test_a, 0, 0, NULL, NULL, NULL) == 0); + assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, test_b, 0, 0, NULL, NULL, NULL) > 0); - assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0) > 0); + assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, test_b, SYSTEMD_CGROUP_CONTROLLER, test_a, 0) > 0); - assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0); - assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b") > 0); + assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, test_a) == 0); + assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, test_b) > 0); - assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, 0, NULL, NULL, NULL) > 0); - assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, 0, NULL, NULL, NULL) == 0); + assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, test_a, 0, 0, NULL, NULL, NULL) > 0); + assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, test_b, 0, 0, NULL, NULL, NULL) == 0); - cg_trim(SYSTEMD_CGROUP_CONTROLLER, "/", false); + cg_trim(SYSTEMD_CGROUP_CONTROLLER, test_b, false); - assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-b") == 0); - assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-a") < 0); - assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", SYSTEMD_CGROUP_CONTROLLER, "system.slice", 0) > 0); - assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0); + assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, test_b) == 0); + assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, test_a) < 0); + assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, test_a, SYSTEMD_CGROUP_CONTROLLER, here, 0) > 0); + assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, test_a) == 0); } int main(int argc, char *argv[]) { From 4f2db153719e4a1227339e75d50945acd74f2948 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Mar 2020 11:22:11 +0100 Subject: [PATCH 6/6] meson: mark test-cgroup as standard --- src/test/meson.build | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/meson.build b/src/test/meson.build index 5df1366561..eb66161861 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -590,8 +590,7 @@ tests += [ [['src/test/test-cgroup.c'], [], - [], - '', 'manual'], + []], [['src/test/test-cgroup-cpu.c'], [libcore,