[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Xiao Yang]

pidfd_open(2) will set close-on-exec flag on the file descriptor as it
manpage states, so check close-on-exec flag by fcntl(2).

BTW:
I tried to pass (long) TST_RET to fcntl() but triggered the following
compiler warning, so pass (int) pidfd instead.
------------------------------------------------------
In file included from pidfd_open01.c:9:
pidfd_open01.c: In function ?run?:
../../../../include/tst_test.h:76:41: warning: format ?%i? expects argument of type ?int?, but argument 5 has type ?long int? [-Wformat=]
   76 |   tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
      |                                         ^~~~~~~~~
../../../../include/tst_safe_macros.h:224:5: note: in expansion of macro ?tst_brk?
  224 |     tst_brk(TBROK | TERRNO,                          \
      |     ^~~~~~~
pidfd_open01.c:20:9: note: in expansion of macro ?SAFE_FCNTL?
   20 |  flag = SAFE_FCNTL(TST_RET, F_GETFD);
------------------------------------------------------

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 testcases/kernel/syscalls/pidfd_open/pidfd_open01.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
index 93bb86687..ba1580bc7 100644
--- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
@@ -11,13 +11,20 @@
 
 static void run(void)
 {
-	TEST(pidfd_open(getpid(), 0));
+	int pidfd, flag;
+
+	TEST(pidfd = pidfd_open(getpid(), 0));
 
 	if (TST_RET == -1)
 		tst_brk(TFAIL | TTERRNO, "pidfd_open(getpid(), 0) failed");
 
+	flag = SAFE_FCNTL(pidfd, F_GETFD);
+
 	SAFE_CLOSE(TST_RET);
 
+	if (!(flag & FD_CLOEXEC))
+		tst_brk(TFAIL, "pidfd_open(getpid(), 0) didn't set close-on-exec flag");
+
 	tst_res(TPASS, "pidfd_open(getpid(), 0) passed");
 }
 
-- 
2.21.0

[LTP] [PATCH v2 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag

[Xiao Yang]

1) Drop .min_kver flag directly because of two following reasons:
   a) pidfd_open(2) may be backported to old kernel which is less
      than v5.3 so kernel version check is meaningless.
   b) tst_syscall() can report TCONF if pidfd_open(2) is not supported.
2) For pidfd_open03.c, check if pidfd_open(2) is not supported before
   calling fork().

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 testcases/kernel/syscalls/pidfd_open/pidfd_open01.c |  1 -
 testcases/kernel/syscalls/pidfd_open/pidfd_open02.c |  1 -
 testcases/kernel/syscalls/pidfd_open/pidfd_open03.c | 12 +++++++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
index ba1580bc7..6f5114f68 100644
--- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
@@ -29,6 +29,5 @@ static void run(void)
 }
 
 static struct tst_test test = {
-	.min_kver = "5.3",
 	.test_all = run,
 };
diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
index dc86cae7a..a7328ddfe 100644
--- a/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
@@ -51,7 +51,6 @@ static void run(unsigned int n)
 }
 
 static struct tst_test test = {
-	.min_kver = "5.3",
 	.tcnt = ARRAY_SIZE(tcases),
 	.test = run,
 	.setup = setup,
diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
index 48470e5e1..9e27ee75e 100644
--- a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
@@ -49,8 +49,18 @@ static void run(void)
 		tst_res(TPASS, "pidfd_open() passed");
 }
 
+static void setup(void)
+{
+	int pidfd = -1;
+
+	// Check if pidfd_open(2) is not supported
+	pidfd = tst_syscall(__NR_pidfd_open, getpid(), 0);
+	if (pidfd != -1)
+		SAFE_CLOSE(pidfd);
+}
+
 static struct tst_test test = {
-	.min_kver = "5.3",
+	.setup = setup,
 	.test_all = run,
 	.forks_child = 1,
 	.needs_checkpoints = 1,
-- 
2.21.0

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Xiao Yang]

Hi Cyril, Petr

For the patch set, I and Viresh have the following doubts so do you have 
any suggestion about them?
1) I keep TEST() in pidfd_open01/pidfd_open03 for now but I think it is
    surplus because pidfd/fd and TERRNO are enough to check return value
    and errno.
    I wonder if it is necessary to keep TEST()?
2) tst_syscall() is enough to check the support of pidfd_open() and I
    don't want to define check function as fsopen_supported_by_kernel()
    does.
    Do you think so?

BTW:
I don't like the implementation of fsopen_supported_by_kernel():
a) syscall()/tst_syscall() is enough to check the support of
pidfd_open(2) and 'tst_kvercmp(5, 2, 0)) < 0' will skip the check if
a kernel on distribution is newer than v5.2 but drop the support of
pidfd_open(2) on purpose.
b) tst_syscall() has checked ENOSYS error so we can simple
fsopen_supported_by_kernel() by replacing syscall() with tst_syscalls().

Best Regards,
Xiao Yang
On 2020/5/13 9:26, Xiao Yang wrote:
> pidfd_open(2) will set close-on-exec flag on the file descriptor as it
> manpage states, so check close-on-exec flag by fcntl(2).
>
> BTW:
> I tried to pass (long) TST_RET to fcntl() but triggered the following
> compiler warning, so pass (int) pidfd instead.
> ------------------------------------------------------
> In file included from pidfd_open01.c:9:
> pidfd_open01.c: In function ?run?:
> ../../../../include/tst_test.h:76:41: warning: format ?%i? expects argument of type ?int?, but argument 5 has type ?long int? [-Wformat=]
>     76 |   tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
>        |                                         ^~~~~~~~~
> ../../../../include/tst_safe_macros.h:224:5: note: in expansion of macro ?tst_brk?
>    224 |     tst_brk(TBROK | TERRNO,                          \
>        |     ^~~~~~~
> pidfd_open01.c:20:9: note: in expansion of macro ?SAFE_FCNTL?
>     20 |  flag = SAFE_FCNTL(TST_RET, F_GETFD);
> ------------------------------------------------------
>
> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> Reviewed-by: Viresh Kumar<viresh.kumar@linaro.org>
> ---
>   testcases/kernel/syscalls/pidfd_open/pidfd_open01.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> index 93bb86687..ba1580bc7 100644
> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> @@ -11,13 +11,20 @@
>
>   static void run(void)
>   {
> -	TEST(pidfd_open(getpid(), 0));
> +	int pidfd, flag;
> +
> +	TEST(pidfd = pidfd_open(getpid(), 0));
>
>   	if (TST_RET == -1)
>   		tst_brk(TFAIL | TTERRNO, "pidfd_open(getpid(), 0) failed");
>
> +	flag = SAFE_FCNTL(pidfd, F_GETFD);
> +
>   	SAFE_CLOSE(TST_RET);
>
> +	if (!(flag&  FD_CLOEXEC))
> +		tst_brk(TFAIL, "pidfd_open(getpid(), 0) didn't set close-on-exec flag");
> +
>   	tst_res(TPASS, "pidfd_open(getpid(), 0) passed");
>   }
>

[LTP] [PATCH v2 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag

[Viresh Kumar]

On 13-05-20, 09:26, Xiao Yang wrote:
> 1) Drop .min_kver flag directly because of two following reasons:
>    a) pidfd_open(2) may be backported to old kernel which is less
>       than v5.3 so kernel version check is meaningless.
>    b) tst_syscall() can report TCONF if pidfd_open(2) is not supported.
> 2) For pidfd_open03.c, check if pidfd_open(2) is not supported before
>    calling fork().
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

You can not apply someone's tag without them explicitly asking you to.

> ---
>  testcases/kernel/syscalls/pidfd_open/pidfd_open01.c |  1 -
>  testcases/kernel/syscalls/pidfd_open/pidfd_open02.c |  1 -
>  testcases/kernel/syscalls/pidfd_open/pidfd_open03.c | 12 +++++++++++-
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> index ba1580bc7..6f5114f68 100644
> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> @@ -29,6 +29,5 @@ static void run(void)
>  }
>  
>  static struct tst_test test = {
> -	.min_kver = "5.3",
>  	.test_all = run,
>  };
> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
> index dc86cae7a..a7328ddfe 100644
> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
> @@ -51,7 +51,6 @@ static void run(unsigned int n)
>  }
>  
>  static struct tst_test test = {
> -	.min_kver = "5.3",
>  	.tcnt = ARRAY_SIZE(tcases),
>  	.test = run,
>  	.setup = setup,
> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
> index 48470e5e1..9e27ee75e 100644
> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
> @@ -49,8 +49,18 @@ static void run(void)
>  		tst_res(TPASS, "pidfd_open() passed");
>  }
>  
> +static void setup(void)
> +{
> +	int pidfd = -1;
> +
> +	// Check if pidfd_open(2) is not supported
> +	pidfd = tst_syscall(__NR_pidfd_open, getpid(), 0);
> +	if (pidfd != -1)
> +		SAFE_CLOSE(pidfd);
> +}
> +

This only fixes the 3rd test, the other two will have an issue now.

>  static struct tst_test test = {
> -	.min_kver = "5.3",
> +	.setup = setup,
>  	.test_all = run,
>  	.forks_child = 1,
>  	.needs_checkpoints = 1,
> -- 
> 2.21.0
> 
> 

-- 
viresh

[LTP] [PATCH v2 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag

[Xiao Yang]

On 2020/5/13 13:55, Viresh Kumar wrote:
> On 13-05-20, 09:26, Xiao Yang wrote:
>> 1) Drop .min_kver flag directly because of two following reasons:
>>     a) pidfd_open(2) may be backported to old kernel which is less
>>        than v5.3 so kernel version check is meaningless.
>>     b) tst_syscall() can report TCONF if pidfd_open(2) is not supported.
>> 2) For pidfd_open03.c, check if pidfd_open(2) is not supported before
>>     calling fork().
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> Reviewed-by: Viresh Kumar<viresh.kumar@linaro.org>
>
> You can not apply someone's tag without them explicitly asking you to.
Hi Viresh,

OK, I can remove it.

>
>> ---
>>   testcases/kernel/syscalls/pidfd_open/pidfd_open01.c |  1 -
>>   testcases/kernel/syscalls/pidfd_open/pidfd_open02.c |  1 -
>>   testcases/kernel/syscalls/pidfd_open/pidfd_open03.c | 12 +++++++++++-
>>   3 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> index ba1580bc7..6f5114f68 100644
>> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> @@ -29,6 +29,5 @@ static void run(void)
>>   }
>>
>>   static struct tst_test test = {
>> -	.min_kver = "5.3",
>>   	.test_all = run,
>>   };
>> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
>> index dc86cae7a..a7328ddfe 100644
>> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
>> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c
>> @@ -51,7 +51,6 @@ static void run(unsigned int n)
>>   }
>>
>>   static struct tst_test test = {
>> -	.min_kver = "5.3",
>>   	.tcnt = ARRAY_SIZE(tcases),
>>   	.test = run,
>>   	.setup = setup,
>> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
>> index 48470e5e1..9e27ee75e 100644
>> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
>> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
>> @@ -49,8 +49,18 @@ static void run(void)
>>   		tst_res(TPASS, "pidfd_open() passed");
>>   }
>>
>> +static void setup(void)
>> +{
>> +	int pidfd = -1;
>> +
>> +	// Check if pidfd_open(2) is not supported
>> +	pidfd = tst_syscall(__NR_pidfd_open, getpid(), 0);
>> +	if (pidfd != -1)
>> +		SAFE_CLOSE(pidfd);
>> +}
>> +
>
> This only fixes the 3rd test, the other two will have an issue now.

Could you tell which issue happen? Thanks a lot.
The other two don't need the extra check because the implementation of 
pidfd_open() can do it well.  For 3rd test, I want to check the support 
of pidfs_open() before doing fork().

Thanks,
Xiao Yang
>
>>   static struct tst_test test = {
>> -	.min_kver = "5.3",
>> +	.setup = setup,
>>   	.test_all = run,
>>   	.forks_child = 1,
>>   	.needs_checkpoints = 1,
>> --
>> 2.21.0
>>
>>
>

[LTP] [PATCH v2 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag

[Viresh Kumar]

On 13-05-20, 14:03, Xiao Yang wrote:
> Could you tell which issue happen? Thanks a lot.
> The other two don't need the extra check because the implementation of
> pidfd_open() can do it well.  For 3rd test, I want to check the support of
> pidfs_open() before doing fork().

What I meant was that the solution needs to be consistent across
tests. For example, with the current change the run() function will
run for all tests in pidfd_open02.c and print the message that syscall
isn't supported, while it would be better to run it only once in setup
and get done with it. i.e. 1 message instead of 3 similar ones.

-- 
viresh

[LTP] [PATCH v2 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag

[Xiao Yang]

? 2020/5/13 14:13, Viresh Kumar ??:
> On 13-05-20, 14:03, Xiao Yang wrote:
>> Could you tell which issue happen? Thanks a lot.
>> The other two don't need the extra check because the implementation of
>> pidfd_open() can do it well.  For 3rd test, I want to check the support of
>> pidfs_open() before doing fork().
>
> What I meant was that the solution needs to be consistent across
Hi Viresh,

Current change can do correct check for pidfd_open[1-3] so don't need to 
add redundant check.

> tests. For example, with the current change the run() function will
> run for all tests in pidfd_open02.c and print the message that syscall
> isn't supported, while it would be better to run it only once in setup
> and get done with it. i.e. 1 message instead of 3 similar ones.
>

Are you sure?

Triggering first tst_brk(TCONF, ...) will break the whole test instead 
of a subtest.

Thanks,
Xiao Yang

[LTP] [PATCH v2 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag

[Viresh Kumar]

On 13-05-20, 14:31, Xiao Yang wrote:
> ? 2020/5/13 14:13, Viresh Kumar ??:
> > On 13-05-20, 14:03, Xiao Yang wrote:
> > > Could you tell which issue happen? Thanks a lot.
> > > The other two don't need the extra check because the implementation of
> > > pidfd_open() can do it well.  For 3rd test, I want to check the support of
> > > pidfs_open() before doing fork().
> > 
> > What I meant was that the solution needs to be consistent across
> Hi Viresh,
> 
> Current change can do correct check for pidfd_open[1-3] so don't need to add
> redundant check.

How will a check from file 03.c check it for 01.c or 02.c ?

> > tests. For example, with the current change the run() function will
> > run for all tests in pidfd_open02.c and print the message that syscall
> > isn't supported, while it would be better to run it only once in setup
> > and get done with it. i.e. 1 message instead of 3 similar ones.
> > 
> 
> Are you sure?
> 
> Triggering first tst_brk(TCONF, ...) will break the whole test instead of a
> subtest.

Ahh, yes. I missed the tst_brk part :(

But I will still like it to be consistent across files. But anyway,
whatever you and Cyril decide is fine :)

Thanks.

-- 
viresh

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Petr Vorel]

Hi Yang,

> For the patch set, I and Viresh have the following doubts so do you have any
> suggestion about them?
> 1) I keep TEST() in pidfd_open01/pidfd_open03 for now but I think it is
>    surplus because pidfd/fd and TERRNO are enough to check return value
>    and errno.
>    I wonder if it is necessary to keep TEST()?

yes, I've noticed your discussion at v1, sorry I wasn't able to follow.
https://patchwork.ozlabs.org/project/ltp/patch/20200430085742.1663-1-yangx.jy@cn.fujitsu.com/
Just to get context, We're talking about part of the changes between v1 and v2:

+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
@@ -27,9 +27,11 @@ static void run(void)
                exit(EXIT_SUCCESS);
        }

-       fd = pidfd_open(pid, 0);
+       TEST(pidfd_open(pid, 0));
+
+       fd = TST_RET;
        if (fd == -1)
-               tst_brk(TFAIL | TERRNO, "pidfd_open() failed");
+               tst_brk(TFAIL | TTERRNO, "pidfd_open() failed");

        TST_CHECKPOINT_WAKE(0);

I haven't found Cyril's request to use TEST(). To be honest, not sure if it was
meant to make sure that errno needs to be reset before (which TEST()) does.
If not, using pidfd_open() directly would be ok for me.


> 2) tst_syscall() is enough to check the support of pidfd_open() and I
>    don't want to define check function as fsopen_supported_by_kernel()
>    does.
>    Do you think so?

> BTW:
> I don't like the implementation of fsopen_supported_by_kernel():
> a) syscall()/tst_syscall() is enough to check the support of
> pidfd_open(2) and 'tst_kvercmp(5, 2, 0)) < 0' will skip the check if
+1 for tst_syscall()

> a kernel on distribution is newer than v5.2 but drop the support of
> pidfd_open(2) on purpose.
"drop support of pidfd_open(2) on purpose": would anybody has a reason to do
that?

> b) tst_syscall() has checked ENOSYS error so we can simple
> fsopen_supported_by_kernel() by replacing syscall() with tst_syscalls().

Well, one of the benefits of fsopen_supported_by_kernel() was to reduce a bit of
duplicity. Even if the implementation is like TEST and SAFE_CLOSE(), it's
a fewer lines (+ function name as a self docs).

void fsopen_supported_by_kernel(void)
{
	TEST(tst_syscall(__NR_fsopen, NULL, 0));
	if (TST_RET != -1)
		SAFE_CLOSE(TST_RET);
}

For your change for pidfd_open03.c:

static void setup(void)
{
	int pidfd = -1;

	// Check if pidfd_open(2) is not supported
	pidfd = tst_syscall(__NR_pidfd_open, getpid(), 0);
	if (pidfd != -1)
		SAFE_CLOSE(pidfd);
}

 static struct tst_test test = {
-       .min_kver = "5.3",
+       .setup = setup,

How about to call the function pidfd_open_supported_by_kernel()?
Than you can remove the comment (which BTW should use C style /* */).
And IMHO you don't have to assign pidfd to -1.

Kind regards,
Petr

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Xiao Yang]

On 2020/5/13 17:20, Petr Vorel wrote:
> Hi Yang,
>
>> For the patch set, I and Viresh have the following doubts so do you have any
>> suggestion about them?
>> 1) I keep TEST() in pidfd_open01/pidfd_open03 for now but I think it is
>>     surplus because pidfd/fd and TERRNO are enough to check return value
>>     and errno.
>>     I wonder if it is necessary to keep TEST()?
>
> yes, I've noticed your discussion at v1, sorry I wasn't able to follow.
> https://patchwork.ozlabs.org/project/ltp/patch/20200430085742.1663-1-yangx.jy@cn.fujitsu.com/
> Just to get context, We're talking about part of the changes between v1 and v2:
>
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
> @@ -27,9 +27,11 @@ static void run(void)
>                  exit(EXIT_SUCCESS);
>          }
>
> -       fd = pidfd_open(pid, 0);
> +       TEST(pidfd_open(pid, 0));
> +
> +       fd = TST_RET;
>          if (fd == -1)
> -               tst_brk(TFAIL | TERRNO, "pidfd_open() failed");
> +               tst_brk(TFAIL | TTERRNO, "pidfd_open() failed");
>
>          TST_CHECKPOINT_WAKE(0);
>
> I haven't found Cyril's request to use TEST(). To be honest, not sure if it was
> meant to make sure that errno needs to be reset before (which TEST()) does.
> If not, using pidfd_open() directly would be ok for me.
Hi Petr,

Thanks a lot for your quick reply.

Resetting errno may not necessary because errno will be set again when
fd == -1.

>
>
>> 2) tst_syscall() is enough to check the support of pidfd_open() and I
>>     don't want to define check function as fsopen_supported_by_kernel()
>>     does.
>>     Do you think so?
>
>> BTW:
>> I don't like the implementation of fsopen_supported_by_kernel():
>> a) syscall()/tst_syscall() is enough to check the support of
>> pidfd_open(2) and 'tst_kvercmp(5, 2, 0))<  0' will skip the check if
> +1 for tst_syscall()
>
>> a kernel on distribution is newer than v5.2 but drop the support of
>> pidfd_open(2) on purpose.
> "drop support of pidfd_open(2) on purpose": would anybody has a reason to do
> that?

As my pervious mail said, It is just a possible situation? for example:
Upstream kernel introduces btrfs filesystem long long ago but the
kernel of RHEL8 drops btrfs filesystem because of some reasons.

It is just a reason used to explain why I want to drop the kernel 
version check.

>
>> b) tst_syscall() has checked ENOSYS error so we can simple
>> fsopen_supported_by_kernel() by replacing syscall() with tst_syscalls().
>
> Well, one of the benefits of fsopen_supported_by_kernel() was to reduce a bit of
> duplicity. Even if the implementation is like TEST and SAFE_CLOSE(), it's
> a fewer lines (+ function name as a self docs).
>
> void fsopen_supported_by_kernel(void)
> {
> 	TEST(tst_syscall(__NR_fsopen, NULL, 0));
> 	if (TST_RET != -1)
> 		SAFE_CLOSE(TST_RET);
> }
>
> For your change for pidfd_open03.c:
>
> static void setup(void)
> {
> 	int pidfd = -1;
>
> 	// Check if pidfd_open(2) is not supported
> 	pidfd = tst_syscall(__NR_pidfd_open, getpid(), 0);
> 	if (pidfd != -1)
> 		SAFE_CLOSE(pidfd);
> }
>
>   static struct tst_test test = {
> -       .min_kver = "5.3",
> +       .setup = setup,
>
> How about to call the function pidfd_open_supported_by_kernel()?

OK

> Than you can remove the comment (which BTW should use C style /* */).

OK

> And IMHO you don't have to assign pidfd to -1.

In pidfd_open_supported_by_kernel(), do you want to drop 'pidfd = -1' 
directly or drop 'pidfd = -1' by using TEST()?

Best Regards,
Xiao Yang
>
> Kind regards,
> Petr
>
>
> .
>

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Petr Vorel]

Hi Yang,

> Thanks a lot for your quick reply.
Thanks for a patience (we're not working just on LTP unfortunately).

> Resetting errno may not necessary because errno will be set again when
> fd == -1.
Agree, I'm just careful, thus asking :).

> > > 2) tst_syscall() is enough to check the support of pidfd_open() and I
> > >     don't want to define check function as fsopen_supported_by_kernel()
> > >     does.
> > >     Do you think so?

> > > BTW:
> > > I don't like the implementation of fsopen_supported_by_kernel():
> > > a) syscall()/tst_syscall() is enough to check the support of
> > > pidfd_open(2) and 'tst_kvercmp(5, 2, 0))<  0' will skip the check if
> > +1 for tst_syscall()

> > > a kernel on distribution is newer than v5.2 but drop the support of
> > > pidfd_open(2) on purpose.
> > "drop support of pidfd_open(2) on purpose": would anybody has a reason to do
> > that?

> As my pervious mail said, It is just a possible situation? for example:
> Upstream kernel introduces btrfs filesystem long long ago but the
> kernel of RHEL8 drops btrfs filesystem because of some reasons.
I guess filesystem changes are the most frequent. But as I said, I wouldn't mind 
this implementation:
void fsopen_supported_by_kernel(void)
{
	TEST(tst_syscall(__NR_fsopen, NULL, 0));
	if (TST_RET != -1)
		SAFE_CLOSE(TST_RET);
}

> It is just a reason used to explain why I want to drop the kernel version
> check.

...

> > How about to call the function pidfd_open_supported_by_kernel()?

> OK

> > Than you can remove the comment (which BTW should use C style /* */).

> OK

> > And IMHO you don't have to assign pidfd to -1.

> In pidfd_open_supported_by_kernel(), do you want to drop 'pidfd = -1'
> directly or drop 'pidfd = -1' by using TEST()?
I meant (as it's always assigned by the call, it's just a nit.):
-int pidfd = -1;
+int pidfd;

Kind regards,
Petr

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Cyril Hrubis]

Hi!
> pidfd_open(2) will set close-on-exec flag on the file descriptor as it
> manpage states, so check close-on-exec flag by fcntl(2).
> 
> BTW:
> I tried to pass (long) TST_RET to fcntl() but triggered the following
> compiler warning, so pass (int) pidfd instead.
> ------------------------------------------------------
> In file included from pidfd_open01.c:9:
> pidfd_open01.c: In function ???run???:
> ../../../../include/tst_test.h:76:41: warning: format ???%i??? expects argument of type ???int???, but argument 5 has type ???long int??? [-Wformat=]
>    76 |   tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
>       |                                         ^~~~~~~~~
> ../../../../include/tst_safe_macros.h:224:5: note: in expansion of macro ???tst_brk???
>   224 |     tst_brk(TBROK | TERRNO,                          \
>       |     ^~~~~~~
> pidfd_open01.c:20:9: note: in expansion of macro ???SAFE_FCNTL???
>    20 |  flag = SAFE_FCNTL(TST_RET, F_GETFD);

You can either cast the TST_RET to int as SAFE_FCNTL((int)TST_RET, ...)
or if you decide to store the pidfd into an int variable there is no
added value in using the TEST() macro here so the code should just do
pidfd = pidfd_open(...) and use the pidfd instead of TST_RET.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Xiao Yang]

On 2020/5/13 20:34, Cyril Hrubis wrote:
> Hi!
>> pidfd_open(2) will set close-on-exec flag on the file descriptor as it
>> manpage states, so check close-on-exec flag by fcntl(2).
>>
>> BTW:
>> I tried to pass (long) TST_RET to fcntl() but triggered the following
>> compiler warning, so pass (int) pidfd instead.
>> ------------------------------------------------------
>> In file included from pidfd_open01.c:9:
>> pidfd_open01.c: In function ???run???:
>> ../../../../include/tst_test.h:76:41: warning: format ???%i??? expects argument of type ???int???, but argument 5 has type ???long int??? [-Wformat=]
>>     76 |   tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
>>        |                                         ^~~~~~~~~
>> ../../../../include/tst_safe_macros.h:224:5: note: in expansion of macro ???tst_brk???
>>    224 |     tst_brk(TBROK | TERRNO,                          \
>>        |     ^~~~~~~
>> pidfd_open01.c:20:9: note: in expansion of macro ???SAFE_FCNTL???
>>     20 |  flag = SAFE_FCNTL(TST_RET, F_GETFD);
>
> You can either cast the TST_RET to int as SAFE_FCNTL((int)TST_RET, ...)
> or if you decide to store the pidfd into an int variable there is no
> added value in using the TEST() macro here so the code should just do
> pidfd = pidfd_open(...) and use the pidfd instead of TST_RET.
Hi Cyril,

Thanks for your reply.
I also think it is OK to replace TEST() and TST_RET directly.

Thanks,
Xiao Yang
>

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Petr Vorel]

Hi Yang,

one more note:

> > As my pervious mail said, It is just a possible situation? for example:
> > Upstream kernel introduces btrfs filesystem long long ago but the
> > kernel of RHEL8 drops btrfs filesystem because of some reasons.
> I guess filesystem changes are the most frequent. But as I said, I wouldn't mind
> this implementation:
> void fsopen_supported_by_kernel(void)
> {
> 	TEST(tst_syscall(__NR_fsopen, NULL, 0));
> 	if (TST_RET != -1)
> 		SAFE_CLOSE(TST_RET);
> }
BTW the same approach is used in include/lapi/openat2.h

void openat2_supported_by_kernel(void)
{
	if ((tst_kvercmp(5, 6, 0)) < 0) {
		/* Check if the syscall is backported on an older kernel */
		TEST(syscall(__NR_openat2, -1, NULL, NULL, 0));
		if (TST_RET == -1 && TST_ERR == ENOSYS)
			tst_brk(TCONF, "Test not supported on kernel version < v5.6");
	}
}

and clone3_supported_by_kernel(). Both merged by Cyril.

To be honest I like this approach, because 1) it defines when new syscall was
backported 2) if there is really problem that some functionality was removed, we
can always handle it. But IMHO that's going to be rare (btrfs removed in RHEL 8
is IMHO because RHEL does not want to support it, but that would not happen for
syscalls).

I'd also like to be consistent how we handle these new syscalls.

Kind regards,
Petr

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Xiao Yang]

On 2020/5/14 15:37, Petr Vorel wrote:
> Hi Yang,
>
> one more note:
>
>>> As my pervious mail said, It is just a possible situation? for example:
>>> Upstream kernel introduces btrfs filesystem long long ago but the
>>> kernel of RHEL8 drops btrfs filesystem because of some reasons.
>> I guess filesystem changes are the most frequent. But as I said, I wouldn't mind
>> this implementation:
>> void fsopen_supported_by_kernel(void)
>> {
>> 	TEST(tst_syscall(__NR_fsopen, NULL, 0));
>> 	if (TST_RET != -1)
>> 		SAFE_CLOSE(TST_RET);
>> }
> BTW the same approach is used in include/lapi/openat2.h
>
> void openat2_supported_by_kernel(void)
> {
> 	if ((tst_kvercmp(5, 6, 0))<  0) {
> 		/* Check if the syscall is backported on an older kernel */
> 		TEST(syscall(__NR_openat2, -1, NULL, NULL, 0));
> 		if (TST_RET == -1&&  TST_ERR == ENOSYS)
> 			tst_brk(TCONF, "Test not supported on kernel version<  v5.6");
> 	}
> }
>
> and clone3_supported_by_kernel(). Both merged by Cyril.
>
> To be honest I like this approach, because 1) it defines when new syscall was
> backported
Hi Petr?

Hmm, the reason seems a little weak, it can be done by adding a 
comment(e.g. "the syscall is introduced since v5.6.0").

2) if there is really problem that some functionality was removed, we
> can always handle it. But IMHO that's going to be rare (btrfs removed in RHEL 8
> is IMHO because RHEL does not want to support it, but that would not happen for
> syscalls).

Without the rare situation, I also think tst_syscall() is enough to 
check the support of syscall.

>
> I'd also like to be consistent how we handle these new syscalls.
Agreed.

I also think if we can implement common func(e.g. 
syscall_supported_by_kernel()).

Best Regards,
Xiao Yang
>
> Kind regards,
> Petr
>
>
> .
>

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Petr Vorel]

Hi Yang,

> > To be honest I like this approach, because 1) it defines when new syscall was
> > backported

> Hmm, the reason seems a little weak, it can be done by adding a comment(e.g.
> "the syscall is introduced since v5.6.0").
Sure, that would work as well.

> 2) if there is really problem that some functionality was removed, we
> > can always handle it. But IMHO that's going to be rare (btrfs removed in RHEL 8
> > is IMHO because RHEL does not want to support it, but that would not happen for
> > syscalls).

> Without the rare situation, I also think tst_syscall() is enough to check
> the support of syscall.
Well, nothing that much important, but I'd like to hear the opinion of
other maintainers. BTW We now concentrate on pre-release fixes.

> > I'd also like to be consistent how we handle these new syscalls.
> Agreed.

> I also think if we can implement common func(e.g.
> syscall_supported_by_kernel()).
Sure, feel free to send a patch (could be a macro).


Kind regards,
Petr

[LTP] [PATCH v2 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag

[Xiao Yang]

On 2020/5/14 22:14, Petr Vorel wrote:
> Hi Yang,
>
>>> To be honest I like this approach, because 1) it defines when new syscall was
>>> backported
>
>> Hmm, the reason seems a little weak, it can be done by adding a comment(e.g.
>> "the syscall is introduced since v5.6.0").
> Sure, that would work as well.
>
>> 2) if there is really problem that some functionality was removed, we
>>> can always handle it. But IMHO that's going to be rare (btrfs removed in RHEL 8
>>> is IMHO because RHEL does not want to support it, but that would not happen for
>>> syscalls).
>
>> Without the rare situation, I also think tst_syscall() is enough to check
>> the support of syscall.
> Well, nothing that much important, but I'd like to hear the opinion of
> other maintainers. BTW We now concentrate on pre-release fixes.
Hi Petr,

Sure.

>
>>> I'd also like to be consistent how we handle these new syscalls.
>> Agreed.
>
>> I also think if we can implement common func(e.g.
>> syscall_supported_by_kernel()).
> Sure, feel free to send a patch (could be a macro).

OK, I will send a patch after the release.

Best Regards,
Xiao Yang
>
>
> Kind regards,
> Petr
>
>
> .
>