Re: [LTP] [PATCH v4] Rewrite userns06.c using new LTP API

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

BTW it'd help reviewers a bit if you include a changelog.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
TL;DR: TST_TEST_TCONF() in userns06_capcheck.c

> diff --git a/testcases/kernel/containers/userns/userns06_capcheck.c b/testcases/kernel/containers/userns/userns06_capcheck.c
...
> -/*
> - * Verify that:
> +/*\
> + * [Description]
> + *
>   * When a process with non-zero user IDs performs an execve(), the
>   * process's capability sets are cleared. When a process with zero
>   * user IDs performs an execve(), the process's capability sets
>   * are set.
>   */
nit: I wonder if we want to have docparse documentation in both userns06.c and
userns06_capcheck.c, they now look as 2 separate tests. Maybe describe
everything in userns06.c.

> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +#include "config.h"
> +
> +#ifdef HAVE_LIBCAP
...
> +	if (argc < 2)
> +		tst_brk(TBROK, "userns06_capcheck <privileged|unprivileged>");
> +
> +	tst_reinit();
I'm not sure if tst_reinit() shouldn't be called even before tst_brk(TBROK, ...).
> +
> +	SAFE_FILE_SCANF("/proc/sys/kernel/cap_last_cap", "%d", &last_cap);
> +
>  	if (strcmp("privileged", argv[1]))
>  		expected_flag = 0;
nit: It might help debugging to print argv[1] in TINF0.

>  	caps = cap_get_proc();
> -	SAFE_FILE_SCANF(NULL, "/proc/sys/kernel/cap_last_cap", "%d", &last_cap);
> +
>  	for (i = 0; i <= last_cap; i++) {
>  		cap_get_flag(caps, i, CAP_EFFECTIVE, &flag_val);
>  		if (flag_val != expected_flag)
>  			break;
> +
>  		cap_get_flag(caps, i, CAP_PERMITTED, &flag_val);
>  		if (flag_val != expected_flag)
>  			break;
>  	}

> -	if (flag_val != expected_flag) {
> -		printf("unexpected effective/permitted caps at %d\n", i);
> -		exit(1);
> -	}
> +	if (flag_val != expected_flag)
> +		tst_res(TFAIL, "unexpected effective/permitted caps at %d", i);
The flags are CAP_EFFECTIVE and CAP_PERMITTED only here, right?
(i.e. no CAP_INHERITABLE). Not sure how helpful would be to print here which
flag was the failing one.

> +	else
> +		tst_res(TPASS, "expected caps at %d", i);
> +}

>  #else
> -	printf("System is missing libcap.\n");
> -#endif
> -	tst_exit();
> +int main(void)
> +{
> +	tst_brk(TBROK, "System is missing libcap");
>  }
Why don't you also use TST_TEST_TCONF() here?
> +#endif

Kind regards,
Petr

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