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

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

Hi Petr
> 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)
OK.
>
>> + * - 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().
OK.
>
>> +
>> +	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.
I will look Cyril's patch and wait.
>
>> +	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?
>
Maybe can add SAFE_PIDFD_OPEN.
> 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.
>
Will try.

Best Regards
Yang Xu
>
> 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