Re: [LTP] [PATCH v2] Introduce ioctl_pidfd_get_info_supported() function

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> - use new routine in ioctl_pidfd05 test
> - refactor ioctl_pidfd_info_exit_supported() routine
> 
> Suggested-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> ---
>  testcases/kernel/syscalls/ioctl/ioctl_pidfd.h | 49 ++++++++++++-------
>  .../kernel/syscalls/ioctl/ioctl_pidfd05.c     |  9 +++-
>  2 files changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_pidfd.h b/testcases/kernel/syscalls/ioctl/ioctl_pidfd.h
> index 811f969cd..067ef18ba 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_pidfd.h
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_pidfd.h
> @@ -9,19 +9,16 @@
>  #include "tst_test.h"
>  #include "lapi/pidfd.h"
>  
> -static inline int ioctl_pidfd_info_exit_supported(void)
> +static inline long ioctl_pidfd_get_info_supported(void)
>  {
> -	int ret;
>  	pid_t pid;
> -	int pidfd;
> -	int supported = 0;
> +	int pidfd, ret;
>  	struct pidfd_info info;
>  
> -	if (tst_kvercmp(6, 15, 0) >= 0)
> +	if (tst_kvercmp(6, 12, 0) >= 0)
>  		return 1;
>  
>  	memset(&info, 0, sizeof(struct pidfd_info));
> -	info.mask = PIDFD_INFO_EXIT;

We have to keep the PIDFD_INFO_EXIT in the mask, otherwise it will never
be set back by the kernel.

>  	pid = SAFE_FORK();
>  	if (!pid)
> @@ -31,23 +28,39 @@ static inline int ioctl_pidfd_info_exit_supported(void)
>  	SAFE_WAITPID(pid, NULL, 0);
>  
>  	ret = ioctl(pidfd, PIDFD_GET_INFO, &info);
> +	SAFE_CLOSE(pidfd);
> +
>  	if (ret == -1) {
> -		/* - ENOTTY: old kernels not implementing fs/pidfs.c:pidfd_ioctl
> -		 * - EINVAL: until v6.13 kernel
> -		 * - ESRCH: all kernels between v6.13 and v6.15
> +		/* - ENOTTY: kernel too old, ioctl(PIDFD_GET_INFO) not implemented; return -1 */
> +		if (errno == ENOTTY)
> +			return -1;
> +
> +		/* - EINVAL: ioctl(PIDFD_GET_INFO) exists but invalid args
> +		 * - ESRCH: ioctl(PIDFD_GET_INFO) exists but task already exited
> +		 * both mean supported, but info.mask is not set; return 0
>  		 */
> -		if (errno != ENOTTY &&
> -			errno != EINVAL &&
> -			errno != ESRCH)
> -			tst_brk(TBROK | TERRNO, "ioctl error");
> -	} else {
> -		if (info.mask & PIDFD_INFO_EXIT)
> -			supported = 1;
> +		if (errno == EINVAL || errno == ESRCH)
> +			return 0;

If we do not pass the PIDFD_INFO_EXIT above we will end up with ESRCH
here all the time.

Generally I do not like this code that much, since we depend on the fact
that we get ESCHR from the syscall if PIDFD_INFO_EXIT is not
implemented. Without the PIDFD_INFO_EXIT flag the pidfd_info() syscall
is supposed to work on a process before it's waited for and
PIDFD_INFO_EXIT does not work before the process is waited for, mixing
these two checks is a bit ugly.

> +		tst_brk(TBROK | TERRNO, "unexpected ioctl(PIDFD_GET_INFO) error");
>  	}
>  
> -	SAFE_CLOSE(pidfd);
> +	/* ioctl(PIDFD_GET_INFO) successful; return mask */
> +	return info.mask;
> +}
> +
> +static inline bool ioctl_pidfd_info_exit_supported(void)
> +{
> +	long mask;
> +
> +	if (tst_kvercmp(6, 15, 0) >= 0)
> +		return 1;
> +
> +	mask = ioctl_pidfd_get_info_supported();
> +	if (mask == -1)
> +		return 0;
>  
> -	return supported;
> +	return !!(mask & PIDFD_INFO_EXIT);
>  }
>  
>  #endif
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_pidfd05.c b/testcases/kernel/syscalls/ioctl/ioctl_pidfd05.c
> index c379717b3..871f2fe5e 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_pidfd05.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_pidfd05.c
> @@ -14,7 +14,7 @@
>  #include "tst_test.h"
>  #include "lapi/pidfd.h"
>  #include "lapi/sched.h"
> -#include "lapi/ioctl.h"
> +#include "ioctl_pidfd.h"
>  
>  struct pidfd_info_invalid {
>  	uint32_t dummy;
> @@ -48,8 +48,15 @@ static void run(void)
>  	SAFE_CLOSE(pidfd);
>  }
>  
> +static void setup(void)
> +{
> +	if (ioctl_pidfd_get_info_supported() == -1)
> +		tst_brk(TCONF, "ioctl(PIDFD_GET_INFO) is not implemented");
> +}
> +
>  static struct tst_test test = {
>  	.test_all = run,
> +	.setup = setup,
>  	.forks_child = 1,
>  	.bufs = (struct tst_buffers []) {
>  		{&args, .size = sizeof(*args)},
> -- 
> 2.51.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

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