Merge pull request #14901 from w-simon/fix-tests

test: fix some failures in test-cgroup

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;
 }
 

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);

src/core/service.c

@@ -3501,6 +3501,12 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) {
                                 break;
 
                         case SERVICE_STOP_POST:
+
+                                if (control_pid_good(s) <= 0)
+                                        service_enter_signal(s, SERVICE_FINAL_SIGTERM, f);
+
+                                break;
+
                         case SERVICE_FINAL_SIGTERM:
                         case SERVICE_FINAL_SIGKILL:
 
@@ -3650,6 +3656,10 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) {
                                 break;
 
                         case SERVICE_STOP_POST:
+                                if (main_pid_good(s) <= 0)
+                                        service_enter_signal(s, SERVICE_FINAL_SIGTERM, f);
+                                break;
+
                         case SERVICE_FINAL_SIGTERM:
                         case SERVICE_FINAL_SIGKILL:
                                 if (main_pid_good(s) <= 0)

src/journal/sd-journal.c

@@ -435,11 +435,12 @@ _public_ void sd_journal_flush_matches(sd_journal *j) {
         detach_location(j);
 }
 
-_pure_ static int compare_with_location(JournalFile *f, Location *l) {
+_pure_ static int compare_with_location(const JournalFile *f, const Location *l, const JournalFile *current_file) {
         int r;
 
         assert(f);
         assert(l);
+        assert(current_file);
         assert(f->location_type == LOCATION_SEEK);
         assert(IN_SET(l->type, LOCATION_DISCRETE, LOCATION_SEEK));
 
@@ -448,7 +449,8 @@ _pure_ static int compare_with_location(JournalFile *f, Location *l) {
             l->realtime_set &&
             f->current_realtime == l->realtime &&
             l->xor_hash_set &&
-            f->current_xor_hash == l->xor_hash)
+            f->current_xor_hash == l->xor_hash &&
+            f != current_file)
                 return 0;
 
         if (l->seqnum_set &&
@@ -787,7 +789,7 @@ static int next_beyond_location(sd_journal *j, JournalFile *f, direction_t direc
                 if (j->current_location.type == LOCATION_DISCRETE) {
                         int k;
 
-                        k = compare_with_location(f, &j->current_location);
+                        k = compare_with_location(f, &j->current_location, j->current_file);
 
                         found = direction == DIRECTION_DOWN ? k > 0 : k < 0;
                 } else

src/test/meson.build

@@ -594,8 +594,7 @@ tests += [
 
         [['src/test/test-cgroup.c'],
          [],
-         [],
-         '', 'manual'],
+         []],
 
         [['src/test/test-cgroup-cpu.c'],
          [libcore,

src/test/test-cgroup.c

@@ -4,69 +4,27 @@
 
 #include "cgroup-setup.h"
 #include "cgroup-util.h"
+#include "errno-util.h"
 #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;
 
-        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_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"));
-        free(path);
-
-        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"));
-        free(path);
-
-        assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, "/test-b/test-d", 0) == 0);
-
-        assert_se(cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, getpid_cached(), &path) == 0);
-        assert_se(path_equal(path, "/test-b/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"));
-        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_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_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);
-
-        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);
+        log_info("/* %s */", __func__);
 
         assert_se(cg_split_spec("foobar:/", &c, &p) == 0);
         assert_se(streq(c, "foobar"));
         assert_se(streq(p, "/"));
-        free(c);
-        free(p);
+        c = mfree(c);
+        p = mfree(p);
+
+        assert_se(cg_split_spec("foobar:", &c, &p) == 0);
+        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);
@@ -76,12 +34,98 @@ int main(int argc, char *argv[]) {
         assert_se(cg_split_spec("/", &c, &p) >= 0);
         assert_se(c == NULL);
         assert_se(streq(p, "/"));
-        free(p);
+        p = mfree(p);
 
         assert_se(cg_split_spec("foo", &c, &p) >= 0);
         assert_se(streq(c, "foo"));
         assert_se(p == NULL);
-        free(c);
+        c = mfree(c);
+}
+
+static void test_cg_create(void) {
+        log_info("/* %s */", __func__);
+
+        _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));
+        free(path);
+
+        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));
+        free(path);
+
+        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_d));
+        free(path);
+
+        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_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_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);
+
+        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, here, 0) > 0);
+        assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, test_a) == 0);
+}
+
+int main(int argc, char *argv[]) {
+        test_setup_logging(LOG_DEBUG);
+
+        test_cg_split_spec();
+        test_cg_create();
 
         return 0;
 }

test/TEST-47-ISSUE-14566/Makefile

@@ -0,0 +1 @@
+../TEST-01-BASIC/Makefile

test/TEST-47-ISSUE-14566/repro.sh

@@ -0,0 +1,5 @@
+#!/bin/bash
+
+sleep infinity &
+echo $! > /leakedtestpid
+wait $!

test/TEST-47-ISSUE-14566/test.sh

@@ -0,0 +1,43 @@
+#!/bin/bash
+set -e
+TEST_DESCRIPTION="Test that KillMode=mixed does not leave left over proccesses with ExecStopPost="
+. $TEST_BASE_DIR/test-functions
+
+test_setup() {
+    create_empty_image_rootdir
+
+    (
+        LOG_LEVEL=5
+        eval $(udevadm info --export --query=env --name=${LOOPDEV}p2)
+
+        setup_basic_environment
+        mask_supporting_services
+
+        # setup the testsuite service
+        cat >$initdir/etc/systemd/system/testsuite.service <<EOF
+[Unit]
+Description=Testsuite service
+
+[Service]
+ExecStart=/testsuite.sh
+Type=oneshot
+EOF
+        cat > $initdir/etc/systemd/system/issue_14566_test.service << EOF
+[Unit]
+Description=Issue 14566 Repro
+
+[Service]
+ExecStart=/repro.sh
+ExecStopPost=/bin/true
+KillMode=mixed
+EOF
+
+        cp testsuite.sh $initdir/
+        cp repro.sh $initdir/
+
+        setup_testsuite
+    )
+    setup_nspawn_root
+}
+
+do_test "$@"

test/TEST-47-ISSUE-14566/testsuite.sh

@@ -0,0 +1,23 @@
+#!/bin/bash
+set -ex
+set -o pipefail
+
+systemd-analyze log-level debug
+systemd-analyze log-target console
+
+systemctl start issue_14566_test
+systemctl status issue_14566_test
+
+leaked_pid=$(cat /leakedtestpid)
+
+systemctl stop issue_14566_test
+
+# Leaked PID will still be around if we're buggy.
+# I personally prefer to see 42.
+ps -p "$leaked_pid" && exit 42
+
+systemd-analyze log-level info
+
+echo OK > /testok
+
+exit 0