Re: [LTP] [PATCH 5/5] syscalls/pidfd_getfd02: add basic error test

[Petr Vorel <pvorel@suse.cz>]

Hi Xu,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Thanks! Few comments below, can be fixed before merge.

> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
> @@ -1 +1,2 @@
>  pidfd_getfd01
> +pidfd_getfd02
Again /pidfd_getfd02

> diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c
...
> +/*\
> + * [Description]
> + *
> + * Tests basic error handling of the pidfd_open syscall.
> + *
> + * - EBADF pidfd is not a valid PID file descriptor
> + * - EBADF targetfd is not an open file descriptor in the process referred
> + *   to by pidfd
> + * - EINVAL flags is not 0
> + * - ESRCH the process referred to by pidfd does not exist(it has terminated and
> + *   been waited on))

nit: add space and remove redundant bracket
 * - ESRCH the process referred to by pidfd does not exist (it has terminated and
 *   been waited on)

> + * - EPERM the calling process doesn't have PTRACE_MODE_ATTACH_REALCREDS permissions
> + *   over the process referred to by pidfd

+1 (only ENFILE "The system-wide limit on the total number of open files has been
reached." which is probably not worth of implementing).
...

> +static void setup(void)
> +{
> +	struct passwd *pw;
> +
> +	pw = SAFE_GETPWNAM("nobody");
> +	uid = pw->pw_uid;
> +
> +	pidfd_open_supported();
> +	pidfd_getfd_supported();
nit: I'd put these before SAFE_GETPWNAM().

> +
> +	TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open");
If you wait for Cyril's patch adding auto stringification
https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/

you can use just:
TST_EXP_FD_SILENT(pidfd_open(getpid(), 0));

and get more info.

> +	if (!TST_PASS)
> +		tst_brk(TFAIL | TTERRNO, "pidfd_open failed");

@Cyril: would it be possible to to allow using also tst_brk() in macros in
tst_test_macros.h?

Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated

> +	valid_pidfd = TST_RET;
> +}
> +
> +static void run(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +	int pid;
> +
> +	switch (tc->exp_errno) {
> +	case EPERM:
> +		pid = SAFE_FORK();
SAFE_FORK() can be before switch.

> +		if (!pid) {
> +			SAFE_SETUID(uid);
> +			TST_EXP_FAIL2(pidfd_getfd(valid_pidfd, tc->targetfd, tc->flags),
> +				tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s",
> +				valid_pidfd, tc->targetfd, tc->flags, tc->name);
> +			TST_CHECKPOINT_WAKE(0);
> +			exit(0);
> +		}
> +		TST_CHECKPOINT_WAIT(0);
> +		SAFE_WAIT(NULL);
> +		return;
> +	break;
> +	case ESRCH:
> +		pid = SAFE_FORK();
> +		if (!pid) {
> +			TST_CHECKPOINT_WAIT(0);
> +			exit(0);
> +		}
> +		TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open");
> +		*tc->pidfd = TST_RET;
> +		TST_CHECKPOINT_WAKE(0);
> +		SAFE_WAIT(NULL);
> +	break;
> +	default:
> +	break;
> +	};

IMHO more readable would be instead of switch use if/else if or 2 functions.


Kind regards,
Petr

> +
> +	TST_EXP_FAIL2(pidfd_getfd(*tc->pidfd, tc->targetfd, tc->flags),
> +		tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s",
> +		*tc->pidfd, tc->targetfd, tc->flags, tc->name);
> +}

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