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

[Petr Vorel <pvorel@suse.cz>]

Hi Avinesh, Cyril,

...
> >  	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.

+1, I'm sorry to overlook this change.

> >  	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.

+1 (again, I'm sorry to overlook it).

> 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.

Avinesh, I'm sorry, I see my suggestion to reuse code to detect PIDFD_INFO_EXIT
for checking PIDFD_GET_INFO was not a good idea. It'll be clearer to have them
separated.

BTW although ioctl_pidfd_info_exit_supported() should be kept as is, it does not
harm to use bool for it. But that's not related to this fix.

Kind regards,
Petr

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