Re: [LTP] [PATCH v4] clone3: Add clone3's clone_args cgroup

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

...
> +++ b/include/tst_cgroup.h
> @@ -157,6 +157,9 @@ const char *
>  tst_cg_group_name(const struct tst_cg_group *const cg)
>  		      __attribute__ ((nonnull, warn_unused_result));

Maybe to add a comment to describe the function? (other functions have a
description).

> +int tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg)
> +		      __attribute__ ((nonnull, warn_unused_result));
> +
>  /* Remove a descendant CGroup */
>  struct tst_cg_group *
>  tst_cg_group_rm(struct tst_cg_group *const cg)
> diff --git a/include/tst_clone.h b/include/tst_clone.h
> index 9ffdc68d1..7b278dfa7 100644
> --- a/include/tst_clone.h
> +++ b/include/tst_clone.h
> @@ -11,6 +11,7 @@
>  struct tst_clone_args {
>  	uint64_t flags;
>  	uint64_t exit_signal;
> +	uint64_t cgroup;
>  };

...
> +++ b/testcases/kernel/syscalls/clone3/clone303.c
...
> +
> +static void run(void)
> +{
> +	pid_t pid;
> +
> +	pid = clone_into_cgroup(fd);
> +
> +	if (!pid) {
> +		TST_CHECKPOINT_WAIT(0);
> +		return;
> +	}
> +
> +	char buf[BUF_LEN];
> +
> +	SAFE_CG_READ(cg_child_test_simple, "cgroup.procs", buf, BUF_LEN);
> +
> +	int x = atoi(buf);
> +
> +	if (x == pid)
x is using only here. Why not just:
	if (atoi(buf) == pid)

> +		tst_res(TPASS, "clone3 case pass!");
> +	else
> +		tst_brk(TFAIL | TTERRNO, "clone3() failed !");
> +
> +	TST_CHECKPOINT_WAKE(0);
> +
> +	SAFE_WAITPID(pid, NULL, 0);
> +
> +}
> +
> +static void setup(void)
> +{
> +	clone3_supported_by_kernel();
> +
> +	cg_child_test_simple = tst_cg_group_mk(tst_cg, "cg_test_simple");
> +
> +	fd = tst_cg_group_unified_dir_fd(cg_child_test_simple);
> +
> +	if (fd < 0)
> +		tst_res(TFAIL | TTERRNO, "get dir fd failed !");
Wouldn't be better to use here
		tst_brk(TBROK | TTERRNO, "get dir fd failed!");

Or even just
		tst_brk(TBROK, "get dir fd failed!");

Because tst_cg_group_unified_dir_fd() does not do any real operation which would
set errno.

I mean does make sense to continue testing if fd is invalid?
> +}

...

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp