[Devel] [PATCH vz10 v4] tests: add ve_printk selftest

[Pavel Tikhomirov]

Note: If we want to merge it after cgroup-v2 ve support, we would need to wait after we merge:

[PATCH VZ10 0/9] ve/cgroup-v2: support cgroup-v2 in VE
+
https://bitbucket.org/openvz/vzctl.ovz/pull-requests/175
+
https://bitbucket.org/openvz/libvzctl.ovz/pull-requests/484

and rework it (removing cgroup-v1 ve cgroup related things). The diff below works for me (note the cgroup.controllers_hidden use):

diff --git a/tools/testing/selftests/ve_printk/ve_printk_test.c b/tools/testing/selftests/ve_printk/ve_printk_test.c
index 078198039cb4..0061aa14d7f4 100644
--- a/tools/testing/selftests/ve_printk/ve_printk_test.c
+++ b/tools/testing/selftests/ve_printk/ve_printk_test.c
@@ -101,7 +101,7 @@ FIXTURE_SETUP(ve_printk)
        char path[PATH_MAX * 2];
 
        ASSERT_EQ(get_mount_path("cgroup2", NULL, self->cgv2_path, sizeof(self->cgv2_path)), 0);
-       ASSERT_EQ(get_mount_path("cgroup", "ve", self->cgve_path, sizeof(self->cgve_path)), 0);
+       ASSERT_EQ(get_mount_path("cgroup2", NULL, self->cgve_path, sizeof(self->cgve_path)), 0);
 
        self->ctid = CTID_MIN;
        while (self->ctid < CTID_MAX) {
@@ -118,8 +118,11 @@ FIXTURE_SETUP(ve_printk)
 
        snprintf(path, sizeof(path), "%s/%d", self->cgv2_path, self->ctid);
        ASSERT_EQ(mkdir(path, 0755), 0);
-       snprintf(path, sizeof(path), "%s/%d", self->cgve_path, self->ctid);
-       ASSERT_EQ(mkdir(path, 0755), 0);
+
+       snprintf(path, sizeof(path), "echo -ve > %s/%d/cgroup.controllers_hidden",
+                       self->cgve_path, self->ctid);
+       ASSERT_EQ(system(path), 0);
+
        snprintf(path, sizeof(path), "echo %d > %s/%d/ve.veid",
                        self->ctid, self->cgve_path, self->ctid);

But you can clean it up further, e.g. remove ->cgve_path field and remove run_vzct(cgve) argument.

> +int child_func(void *arg)
> +{
> +	int ret;
> +	int fd;
> +	struct fargs *args = (struct fargs *)arg;
> +	char cg_state[PATH_MAX];
> +
> +	/*
> +	 * Enter the VE namespace via unshare, because the CLONE_NEWVE flag
> +	 * intersect with CSIGNAL and cannot be used with clone.
> +	 */
> +	if (unshare(CLONE_NEWVE)) {
> +		return -1;

It is a good practice to return positive return codes on the program exit (return from the "main" function). The -1 here will be converted to less clean 255, it's better to return 1.

Note: you already use 2 and 1 error codes in ve_printk_test_logct(), so it might be a good thing to switch them to 3 and 2, if you want to preserve their separation.

> +	}
> +
> +	ret = setup_timens();
> +	if (ret)
> +		return ret;

Let's also be more explictit here, "return 1" would improve readability (we won't need to dive into the setup_timens function to review all the possible error codes which can be returned), and also follow "main" function return code policy. Same for other return paths in this function please.

> +
> +	snprintf(cg_state, sizeof(cg_state), "%s/%d/ve.state", args->cgve, args->ctid);
> +
> +	(void)umount2("/proc", MNT_DETACH);
> +	ret = mount("proc", "/proc", "proc", 0, NULL);
> +	if (ret < 0)
> +		return ret;
> +

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.