* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
@ 2020-04-30 8:57 Xiao Yang
2020-04-30 8:57 ` [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag Xiao Yang
2020-05-04 5:09 ` [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag Viresh Kumar
0 siblings, 2 replies; 17+ messages in thread
From: Xiao Yang @ 2020-04-30 8:57 UTC (permalink / raw)
To: ltp
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).
Also avoid compiler warning by replacing (long) TST_RET with (int) pidfd:
------------------------------------------------------
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>
---
.../kernel/syscalls/pidfd_open/pidfd_open01.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
index 93bb86687..293e93b63 100644
--- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
@@ -6,17 +6,27 @@
* Basic pidfd_open() test, fetches the PID of the current process and tries to
* get its file descriptor.
*/
+
+#include <sys/types.h>
+#include <unistd.h>
+#include <fcntl.h>
#include "tst_test.h"
#include "lapi/pidfd_open.h"
static void run(void)
{
- TEST(pidfd_open(getpid(), 0));
+ int pidfd = 0, flag = 0;
+
+ pidfd = pidfd_open(getpid(), 0);
+ if (pidfd == -1)
+ tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
+
+ flag = SAFE_FCNTL(pidfd, F_GETFD);
- if (TST_RET == -1)
- tst_brk(TFAIL | TTERRNO, "pidfd_open(getpid(), 0) failed");
+ SAFE_CLOSE(pidfd);
- 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag
2020-04-30 8:57 [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag Xiao Yang
@ 2020-04-30 8:57 ` Xiao Yang
2020-05-04 5:11 ` Viresh Kumar
2020-05-04 5:09 ` [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag Viresh Kumar
1 sibling, 1 reply; 17+ messages in thread
From: Xiao Yang @ 2020-04-30 8:57 UTC (permalink / raw)
To: ltp
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() and remove unnecessary TEST().
Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
.../kernel/syscalls/pidfd_open/pidfd_open01.c | 1 -
.../kernel/syscalls/pidfd_open/pidfd_open02.c | 1 -
.../kernel/syscalls/pidfd_open/pidfd_open03.c | 14 +++++++++-----
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
index 293e93b63..983dcdccb 100644
--- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
@@ -32,6 +32,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..2fc3b3a5f 100644
--- a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
@@ -27,11 +27,9 @@ static void run(void)
exit(EXIT_SUCCESS);
}
- TEST(pidfd_open(pid, 0));
-
- fd = TST_RET;
+ fd = pidfd_open(pid, 0);
if (fd == -1)
- tst_brk(TFAIL | TTERRNO, "pidfd_open() failed");
+ tst_brk(TFAIL | TERRNO, "pidfd_open() failed");
TST_CHECKPOINT_WAKE(0);
@@ -49,8 +47,14 @@ static void run(void)
tst_res(TPASS, "pidfd_open() passed");
}
+static void setup(void)
+{
+ // Check if pidfd_open(2) is not supported
+ tst_syscall(__NR_pidfd_open, -1, 0);
+}
+
static struct tst_test test = {
- .min_kver = "5.3",
+ .setup = setup,
.test_all = run,
.forks_child = 1,
.needs_checkpoints = 1,
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
2020-04-30 8:57 [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag Xiao Yang
2020-04-30 8:57 ` [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag Xiao Yang
@ 2020-05-04 5:09 ` Viresh Kumar
2020-05-04 8:30 ` =?unknown-8bit?b?5p2o5pmT?=
2020-05-04 11:31 ` Xiao Yang
1 sibling, 2 replies; 17+ messages in thread
From: Viresh Kumar @ 2020-05-04 5:09 UTC (permalink / raw)
To: ltp
On 30-04-20, 16:57, 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).
>
> Also avoid compiler warning by replacing (long) TST_RET with (int) pidfd:
> ------------------------------------------------------
> 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);
This log isn't useful as the warning started coming after your change
only and not before.
> ------------------------------------------------------
>
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
> .../kernel/syscalls/pidfd_open/pidfd_open01.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> index 93bb86687..293e93b63 100644
> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> @@ -6,17 +6,27 @@
> * Basic pidfd_open() test, fetches the PID of the current process and tries to
> * get its file descriptor.
> */
> +
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> #include "tst_test.h"
> #include "lapi/pidfd_open.h"
>
> static void run(void)
> {
> - TEST(pidfd_open(getpid(), 0));
> + int pidfd = 0, flag = 0;
None of these need to be initialized.
> +
> + pidfd = pidfd_open(getpid(), 0);
> + if (pidfd == -1)
> + tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
This could have been written as:
TEST(pidfd = pidfd_open(getpid(), 0));
> +
> + flag = SAFE_FCNTL(pidfd, F_GETFD);
>
> - if (TST_RET == -1)
> - tst_brk(TFAIL | TTERRNO, "pidfd_open(getpid(), 0) failed");
> + SAFE_CLOSE(pidfd);
>
> - 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
>
>
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag
2020-04-30 8:57 ` [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag Xiao Yang
@ 2020-05-04 5:11 ` Viresh Kumar
2020-05-04 12:49 ` Xiao Yang
0 siblings, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2020-05-04 5:11 UTC (permalink / raw)
To: ltp
On 30-04-20, 16:57, 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() and remove unnecessary TEST().
>
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
> .../kernel/syscalls/pidfd_open/pidfd_open01.c | 1 -
> .../kernel/syscalls/pidfd_open/pidfd_open02.c | 1 -
> .../kernel/syscalls/pidfd_open/pidfd_open03.c | 14 +++++++++-----
> 3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> index 293e93b63..983dcdccb 100644
> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> @@ -32,6 +32,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..2fc3b3a5f 100644
> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
> @@ -27,11 +27,9 @@ static void run(void)
> exit(EXIT_SUCCESS);
> }
>
> - TEST(pidfd_open(pid, 0));
> -
> - fd = TST_RET;
> + fd = pidfd_open(pid, 0);
> if (fd == -1)
> - tst_brk(TFAIL | TTERRNO, "pidfd_open() failed");
> + tst_brk(TFAIL | TERRNO, "pidfd_open() failed");
Unrelated change, please drop it.
>
> TST_CHECKPOINT_WAKE(0);
>
> @@ -49,8 +47,14 @@ static void run(void)
> tst_res(TPASS, "pidfd_open() passed");
> }
>
> +static void setup(void)
> +{
> + // Check if pidfd_open(2) is not supported
> + tst_syscall(__NR_pidfd_open, -1, 0);
> +}
> +
> static struct tst_test test = {
> - .min_kver = "5.3",
> + .setup = setup,
> .test_all = run,
> .forks_child = 1,
> .needs_checkpoints = 1,
Please have a look at fsopen_supported_by_kernel() in lapi/fsmount.h
and make such a helper.
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
2020-05-04 5:09 ` [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag Viresh Kumar
@ 2020-05-04 8:30 ` =?unknown-8bit?b?5p2o5pmT?=
2020-05-04 11:32 ` Xiao Yang
2020-05-04 11:31 ` Xiao Yang
1 sibling, 1 reply; 17+ messages in thread
From: =?unknown-8bit?b?5p2o5pmT?= @ 2020-05-04 8:30 UTC (permalink / raw)
To: ltp
----- Original Message -----
From: "Viresh Kumar" <viresh.kumar@linaro.org>
To: "Xiao Yang" <yangx.jy@cn.fujitsu.com>
Cc: ltp@lists.linux.it
Sent: Mon, 4 May 2020 10:39:37 +0530
Subject: Re: [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
On 30-04-20, 16:57, 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).
>
> Also avoid compiler warning by replacing (long) TST_RET with (int) pidfd:
> ------------------------------------------------------
> 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);
This log isn't useful as the warning started coming after your change
only and not before.
Hi?
Right?just add a hint why I use pidfd instead so I want to keep it. Of course?I will say that avoid compiler warning from my change in v2 patch.
Thanks?
Xiao Yang
> ------------------------------------------------------
>
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
> .../kernel/syscalls/pidfd_open/pidfd_open01.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> index 93bb86687..293e93b63 100644
> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> @@ -6,17 +6,27 @@
> * Basic pidfd_open() test, fetches the PID of the current process and tries to
> * get its file descriptor.
> */
> +
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> #include "tst_test.h"
> #include "lapi/pidfd_open.h"
>
> static void run(void)
> {
> - TEST(pidfd_open(getpid(), 0));
> + int pidfd = 0, flag = 0;
None of these need to be initialized.
> +
> + pidfd = pidfd_open(getpid(), 0);
> + if (pidfd == -1)
> + tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
This could have been written as:
TEST(pidfd = pidfd_open(getpid(), 0));
> +
> + flag = SAFE_FCNTL(pidfd, F_GETFD);
>
> - if (TST_RET == -1)
> - tst_brk(TFAIL | TTERRNO, "pidfd_open(getpid(), 0) failed");
> + SAFE_CLOSE(pidfd);
>
> - 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
>
>
--
viresh
--
Mailing list info: https://lists.linux.it/listinfo/ltp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200504/ae01d41b/attachment-0001.htm>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
2020-05-04 5:09 ` [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag Viresh Kumar
2020-05-04 8:30 ` =?unknown-8bit?b?5p2o5pmT?=
@ 2020-05-04 11:31 ` Xiao Yang
2020-05-05 3:28 ` Viresh Kumar
1 sibling, 1 reply; 17+ messages in thread
From: Xiao Yang @ 2020-05-04 11:31 UTC (permalink / raw)
To: ltp
On 5/4/20 1:09 PM, Viresh Kumar wrote:
> On 30-04-20, 16:57, 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).
>>
>> Also avoid compiler warning by replacing (long) TST_RET with (int) pidfd:
>> ------------------------------------------------------
>> 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);
> This log isn't useful as the warning started coming after your change
> only and not before.
Hi Viresh,
Thanks for your review.
Right?just add a hint why I use pidfd instead so I want to keep it.
Of course?I will mention that my change introduces the compiler warning
in v2 patch.
>
>> ------------------------------------------------------
>>
>> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
>> ---
>> .../kernel/syscalls/pidfd_open/pidfd_open01.c | 18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> index 93bb86687..293e93b63 100644
>> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> @@ -6,17 +6,27 @@
>> * Basic pidfd_open() test, fetches the PID of the current process and tries to
>> * get its file descriptor.
>> */
>> +
>> +#include <sys/types.h>
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> #include "tst_test.h"
>> #include "lapi/pidfd_open.h"
>>
>> static void run(void)
>> {
>> - TEST(pidfd_open(getpid(), 0));
>> + int pidfd = 0, flag = 0;
> None of these need to be initialized.
Initialization or not initialization are both fine for me.
Do you have any strong reason to drop Initialization?
>
>> +
>> + pidfd = pidfd_open(getpid(), 0);
>> + if (pidfd == -1)
>> + tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
> This could have been written as:
> TEST(pidfd = pidfd_open(getpid(), 0));
Why do you want to keep TEST()? I don't think it is necessary:
1) pidfd and TERRNO are enough to check return value and errno.
2) It is OK for testcase to not use TEST().
Thanks,
Xiao Yang
>
>> +
>> + flag = SAFE_FCNTL(pidfd, F_GETFD);
>>
>> - if (TST_RET == -1)
>> - tst_brk(TFAIL | TTERRNO, "pidfd_open(getpid(), 0) failed");
>> + SAFE_CLOSE(pidfd);
>>
>> - 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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200504/7f2e00c4/attachment.htm>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
2020-05-04 8:30 ` =?unknown-8bit?b?5p2o5pmT?=
@ 2020-05-04 11:32 ` Xiao Yang
0 siblings, 0 replies; 17+ messages in thread
From: Xiao Yang @ 2020-05-04 11:32 UTC (permalink / raw)
To: ltp
Hi Viresh,
Please ignore this reply.
Thanks,
Xiao Yang
On 5/4/20 4:30 PM, ?? wrote:
>
> -----?Original?Message?-----
> From:?"Viresh?Kumar"?<viresh.kumar@linaro.org>
> To:?"Xiao?Yang"?<yangx.jy@cn.fujitsu.com>
> Cc:?ltp@lists.linux.it
> Sent:?Mon,?4?May?2020?10:39:37?+0530
> Subject:?Re:?[LTP]?[PATCH?1/2]?syscalls/pidfd_open01.c:?Add?check?for?close-on-exec?flag
>
>
> On?30-04-20,?16:57,?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).
> >
> >?Also?avoid?compiler?warning?by?replacing?(long)?TST_RET?with?(int)?pidfd:
> >?------------------------------------------------------
> >?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);
>
> This?log?isn't?useful?as?the?warning?started?coming?after?your?change
> only?and?not?before.
>
> Hi?
>
> Right?just add a hint why I use pidfd instead so I want to keep it.
> ?Of course?I will say that avoid compiler warning from my change in v2
> patch.
>
> Thanks?
> Xiao Yang
>
> >?------------------------------------------------------
> >
> >?Signed-off-by:?Xiao?Yang?<yangx.jy@cn.fujitsu.com>
> >?---
> >??.../kernel/syscalls/pidfd_open/pidfd_open01.c??|?18?++++++++++++++----
> >??1?file?changed,?14?insertions(+),?4?deletions(-)
> >
> >?diff?--git?a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c?b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> >?index?93bb86687..293e93b63?100644
> >?---?a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> >?+++?b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> >?@@?-6,17?+6,27?@@
> >???*?Basic?pidfd_open()?test,?fetches?the?PID?of?the?current?process?and?tries?to
> >???*?get?its?file?descriptor.
> >???*/
> >?+
> >?+#include?<sys/types.h>
> >?+#include?<unistd.h>
> >?+#include?<fcntl.h>
> >??#include?"tst_test.h"
> >??#include?"lapi/pidfd_open.h"
> >
> >??static?void?run(void)
> >??{
> >?- TEST(pidfd_open(getpid(),?0));
> >?+ int?pidfd?=?0,?flag?=?0;
>
> None?of?these?need?to?be?initialized.
>
> >?+
> >?+ pidfd?=?pidfd_open(getpid(),?0);
> >?+ if?(pidfd?==?-1)
> >?+ tst_brk(TFAIL?|?TERRNO,?"pidfd_open(getpid(),?0)?failed");
>
> This?could?have?been?written?as:
> ????????TEST(pidfd?=?pidfd_open(getpid(),?0));
>
> >?+
> >?+ flag?=?SAFE_FCNTL(pidfd,?F_GETFD);
> >
> >?- if?(TST_RET?==?-1)
> >?- tst_brk(TFAIL?|?TTERRNO,?"pidfd_open(getpid(),?0)?failed");
> >?+ SAFE_CLOSE(pidfd);
> >
> >?- 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
> >
> >
>
> --
> viresh
>
> --
> Mailing?list?info:?https://lists.linux.it/listinfo/ltp
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200504/a6c19794/attachment-0001.htm>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag
2020-05-04 5:11 ` Viresh Kumar
@ 2020-05-04 12:49 ` Xiao Yang
2020-05-05 3:35 ` Viresh Kumar
0 siblings, 1 reply; 17+ messages in thread
From: Xiao Yang @ 2020-05-04 12:49 UTC (permalink / raw)
To: ltp
On 5/4/20 1:11 PM, Viresh Kumar wrote:
> On 30-04-20, 16:57, 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() and remove unnecessary TEST().
>>
>> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
>> ---
>> .../kernel/syscalls/pidfd_open/pidfd_open01.c | 1 -
>> .../kernel/syscalls/pidfd_open/pidfd_open02.c | 1 -
>> .../kernel/syscalls/pidfd_open/pidfd_open03.c | 14 +++++++++-----
>> 3 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> index 293e93b63..983dcdccb 100644
>> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>> @@ -32,6 +32,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..2fc3b3a5f 100644
>> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
>> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c
>> @@ -27,11 +27,9 @@ static void run(void)
>> exit(EXIT_SUCCESS);
>> }
>>
>> - TEST(pidfd_open(pid, 0));
>> -
>> - fd = TST_RET;
>> + fd = pidfd_open(pid, 0);
>> if (fd == -1)
>> - tst_brk(TFAIL | TTERRNO, "pidfd_open() failed");
>> + tst_brk(TFAIL | TERRNO, "pidfd_open() failed");
> Unrelated change, please drop it.
Hi Viresh,
Yes, It is unrelated change and just a small cleanup.
My commit message has mentioned it and I don't want to do the cleanup in
seperate patch.
>
>>
>> TST_CHECKPOINT_WAKE(0);
>>
>> @@ -49,8 +47,14 @@ static void run(void)
>> tst_res(TPASS, "pidfd_open() passed");
>> }
>>
>> +static void setup(void)
>> +{
>> + // Check if pidfd_open(2) is not supported
>> + tst_syscall(__NR_pidfd_open, -1, 0);
>> +}
>> +
>> static struct tst_test test = {
>> - .min_kver = "5.3",
>> + .setup = setup,
>> .test_all = run,
>> .forks_child = 1,
>> .needs_checkpoints = 1,
> Please have a look at fsopen_supported_by_kernel() in lapi/fsmount.h
> and make such a helper.
First, I want to explain my check point:
Passing invalid argument can check the support of pidfd_open(2) by
ENOSYS errno and we don't need to close the pidfd.
Second, I don't like the implementation of fsopen_supported_by_kernel()
and give some suggestions:
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().
Like the following implementation:
-------------------------------------------------------
void fsopen_supported_by_kernel(void)
{
??? /* Check if the syscall is supported on a kernel */
??? TEST(tst_syscall(__NR_fsopen, NULL, 0));
??? if (TST_RET != -1)
??? ??? SAFE_CLOSE(TST_RET);
}
-------------------------------------------------------
Please correct me if I give some wrong info.
Thanks,
Xiao Yang
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
2020-05-04 11:31 ` Xiao Yang
@ 2020-05-05 3:28 ` Viresh Kumar
2020-05-05 8:44 ` Xiao Yang
2020-06-12 14:24 ` Cyril Hrubis
0 siblings, 2 replies; 17+ messages in thread
From: Viresh Kumar @ 2020-05-05 3:28 UTC (permalink / raw)
To: ltp
On 04-05-20, 19:31, Xiao Yang wrote:
> On 5/4/20 1:09 PM, Viresh Kumar wrote:
> > On 30-04-20, 16:57, 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).
> > >
> > > Also avoid compiler warning by replacing (long) TST_RET with (int) pidfd:
> > > ------------------------------------------------------
> > > 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);
> > This log isn't useful as the warning started coming after your change
> > only and not before.
>
> Hi Viresh,
>
> Thanks for your review.
>
> Right?just add a hint why I use pidfd instead so I want to keep it.
>
> Of course?I will mention that my change introduces the compiler warning in
> v2 patch.
Even that isn't required really. You can add a variable if you like.
> >
> > > ------------------------------------------------------
> > >
> > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> > > ---
> > > .../kernel/syscalls/pidfd_open/pidfd_open01.c | 18 ++++++++++++++----
> > > 1 file changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> > > index 93bb86687..293e93b63 100644
> > > --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> > > +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
> > > @@ -6,17 +6,27 @@
> > > * Basic pidfd_open() test, fetches the PID of the current process and tries to
> > > * get its file descriptor.
> > > */
> > > +
> > > +#include <sys/types.h>
> > > +#include <unistd.h>
> > > +#include <fcntl.h>
> > > #include "tst_test.h"
> > > #include "lapi/pidfd_open.h"
> > > static void run(void)
> > > {
> > > - TEST(pidfd_open(getpid(), 0));
> > > + int pidfd = 0, flag = 0;
> > None of these need to be initialized.
>
> Initialization or not initialization are both fine for me.
>
> Do you have any strong reason to drop Initialization?
Initializations are only required if there is a chance of the variable
getting used without being initialized, which isn't the case here.
Whatever value you set to these variables, they will get overwritten
anyway.
> >
> > > +
> > > + pidfd = pidfd_open(getpid(), 0);
> > > + if (pidfd == -1)
> > > + tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
> > This could have been written as:
> > TEST(pidfd = pidfd_open(getpid(), 0));
>
> Why do you want to keep TEST()? I don't think it is necessary:
>
> 1) pidfd and TERRNO are enough to check return value and errno.
>
> 2) It is OK for testcase to not use TEST().
As far as I have understood, that is the preferred way of doing it
from LTP maintainers.
Over that it was already there, why remove it now ? Just fix the
problems you are trying to fix and that should be good.
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag
2020-05-04 12:49 ` Xiao Yang
@ 2020-05-05 3:35 ` Viresh Kumar
2020-05-05 9:30 ` Xiao Yang
2020-06-12 14:30 ` Cyril Hrubis
0 siblings, 2 replies; 17+ messages in thread
From: Viresh Kumar @ 2020-05-05 3:35 UTC (permalink / raw)
To: ltp
On 04-05-20, 20:49, Xiao Yang wrote:
> Yes, It is unrelated change and just a small cleanup.
>
> My commit message has mentioned it and I don't want to do the cleanup in
> seperate patch.
Removing usage of TEST() is not cleanup but just a choice of the
developer of how to write code, it wasn't my choice and I have been
asked to do it by maintainers, so removing it like that isn't correct.
If you want to do it, please send a separate patch and don't mix with
unrelated changes. There should be a separate patch normally for
different changes.
> >
> > > TST_CHECKPOINT_WAKE(0);
> > > @@ -49,8 +47,14 @@ static void run(void)
> > > tst_res(TPASS, "pidfd_open() passed");
> > > }
> > > +static void setup(void)
> > > +{
> > > + // Check if pidfd_open(2) is not supported
> > > + tst_syscall(__NR_pidfd_open, -1, 0);
> > > +}
> > > +
> > > static struct tst_test test = {
> > > - .min_kver = "5.3",
> > > + .setup = setup,
> > > .test_all = run,
> > > .forks_child = 1,
> > > .needs_checkpoints = 1,
> > Please have a look at fsopen_supported_by_kernel() in lapi/fsmount.h
> > and make such a helper.
>
> First, I want to explain my check point:
>
> Passing invalid argument can check the support of pidfd_open(2) by ENOSYS
> errno and we don't need to close the pidfd.
>
> Second, I don't like the implementation of fsopen_supported_by_kernel() and
> give some suggestions:
>
> 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.
I don't think kernel can drop support of syscalls just like that, we
can't break user space. And if that is done, we need to fix userspace
again separately for that.
We came to the implementation of fsopen_supported_by_kernel() after a
lot of reviews and decided on that and so I asked you for the sake of
keeping similar code throughout LTP (of course there will be old
usages which are different) to have a similar implementation.
Anyway, I will leave it to Cyril to decide on that :)
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
2020-05-05 3:28 ` Viresh Kumar
@ 2020-05-05 8:44 ` Xiao Yang
2020-05-12 14:25 ` Xiao Yang
2020-06-12 14:24 ` Cyril Hrubis
1 sibling, 1 reply; 17+ messages in thread
From: Xiao Yang @ 2020-05-05 8:44 UTC (permalink / raw)
To: ltp
On 5/5/20 11:28 AM, Viresh Kumar wrote:
> On 04-05-20, 19:31, Xiao Yang wrote:
>> On 5/4/20 1:09 PM, Viresh Kumar wrote:
>>> On 30-04-20, 16:57, 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).
>>>>
>>>> Also avoid compiler warning by replacing (long) TST_RET with (int) pidfd:
>>>> ------------------------------------------------------
>>>> 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);
>>> This log isn't useful as the warning started coming after your change
>>> only and not before.
>> Hi Viresh,
>>
>> Thanks for your review.
>>
>> Right?just add a hint why I use pidfd instead so I want to keep it.
>>
>> Of course?I will mention that my change introduces the compiler warning in
>> v2 patch.
> Even that isn't required really. You can add a variable if you like.
Hi Viresh,
Thanks a lot for your review.
I prefer to keep it :-).
>
>>>> ------------------------------------------------------
>>>>
>>>> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
>>>> ---
>>>> .../kernel/syscalls/pidfd_open/pidfd_open01.c | 18 ++++++++++++++----
>>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>>>> index 93bb86687..293e93b63 100644
>>>> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>>>> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>>>> @@ -6,17 +6,27 @@
>>>> * Basic pidfd_open() test, fetches the PID of the current process and tries to
>>>> * get its file descriptor.
>>>> */
>>>> +
>>>> +#include <sys/types.h>
>>>> +#include <unistd.h>
>>>> +#include <fcntl.h>
>>>> #include "tst_test.h"
>>>> #include "lapi/pidfd_open.h"
>>>> static void run(void)
>>>> {
>>>> - TEST(pidfd_open(getpid(), 0));
>>>> + int pidfd = 0, flag = 0;
>>> None of these need to be initialized.
>> Initialization or not initialization are both fine for me.
>>
>> Do you have any strong reason to drop Initialization?
> Initializations are only required if there is a chance of the variable
> getting used without being initialized, which isn't the case here.
> Whatever value you set to these variables, they will get overwritten
> anyway.
Right, they will get overwritten anyway.
As my previous reply said, either of them is OK for me so I can drop
initializations as you suggested.
>
>>>> +
>>>> + pidfd = pidfd_open(getpid(), 0);
>>>> + if (pidfd == -1)
>>>> + tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
>>> This could have been written as:
>>> TEST(pidfd = pidfd_open(getpid(), 0));
>> Why do you want to keep TEST()? I don't think it is necessary:
>>
>> 1) pidfd and TERRNO are enough to check return value and errno.
>>
>> 2) It is OK for testcase to not use TEST().
> As far as I have understood, that is the preferred way of doing it
> from LTP maintainers.
>
> Over that it was already there, why remove it now ? Just fix the
> problems you are trying to fix and that should be good.
Hi Cyril,
TEST() seems surplus after my change so I want to remove it directly.
I wonder if it is necessary to keep TEST()?
Thanks,
Xiao Yang
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag
2020-05-05 3:35 ` Viresh Kumar
@ 2020-05-05 9:30 ` Xiao Yang
2020-05-05 9:51 ` Viresh Kumar
2020-05-12 14:32 ` Xiao Yang
2020-06-12 14:30 ` Cyril Hrubis
1 sibling, 2 replies; 17+ messages in thread
From: Xiao Yang @ 2020-05-05 9:30 UTC (permalink / raw)
To: ltp
Cc: Cyril
On 5/5/20 11:35 AM, Viresh Kumar wrote:
> On 04-05-20, 20:49, Xiao Yang wrote:
>> Yes, It is unrelated change and just a small cleanup.
>>
>> My commit message has mentioned it and I don't want to do the cleanup in
>> seperate patch.
> Removing usage of TEST() is not cleanup but just a choice of the
> developer of how to write code, it wasn't my choice and I have been
> asked to do it by maintainers, so removing it like that isn't correct.
> If you want to do it, please send a separate patch and don't mix with
> unrelated changes. There should be a separate patch normally for
> different changes.
Hi Viresh?
I think TEST() is surplus because fd and TERRNO is enough to finish
check point.
It is not an important change and I will keep it if you and Cyril prefer
to use TEST().
>
>>>> TST_CHECKPOINT_WAKE(0);
>>>> @@ -49,8 +47,14 @@ static void run(void)
>>>> tst_res(TPASS, "pidfd_open() passed");
>>>> }
>>>> +static void setup(void)
>>>> +{
>>>> + // Check if pidfd_open(2) is not supported
>>>> + tst_syscall(__NR_pidfd_open, -1, 0);
>>>> +}
>>>> +
>>>> static struct tst_test test = {
>>>> - .min_kver = "5.3",
>>>> + .setup = setup,
>>>> .test_all = run,
>>>> .forks_child = 1,
>>>> .needs_checkpoints = 1,
>>> Please have a look at fsopen_supported_by_kernel() in lapi/fsmount.h
>>> and make such a helper.
>> First, I want to explain my check point:
>>
>> Passing invalid argument can check the support of pidfd_open(2) by ENOSYS
>> errno and we don't need to close the pidfd.
>>
>> Second, I don't like the implementation of fsopen_supported_by_kernel() and
>> give some suggestions:
>>
>> 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.
> I don't think kernel can drop support of syscalls just like that, we
> can't break user space. And if that is done, we need to fix userspace
> again separately for that.
Hi Viresh,
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.
>
> We came to the implementation of fsopen_supported_by_kernel() after a
> lot of reviews and decided on that and so I asked you for the sake of
> keeping similar code throughout LTP (of course there will be old
> usages which are different) to have a similar implementation.
>
> Anyway, I will leave it to Cyril to decide on that :)
Hi Cyril,
Do you have any comment on the implementation of
fsopen_supported_by_kernel() and
my check point(i.e. check the support of pidfd_open(2) by passing
invalid argument ).
Thanks,
Xiao Yang
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag
2020-05-05 9:30 ` Xiao Yang
@ 2020-05-05 9:51 ` Viresh Kumar
2020-05-12 14:32 ` Xiao Yang
1 sibling, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2020-05-05 9:51 UTC (permalink / raw)
To: ltp
On 05-05-20, 17:30, Xiao Yang wrote:
> I think TEST() is surplus because fd and TERRNO is enough to finish check
> point.
>
> It is not an important change and I will keep it if you and Cyril prefer to
> use TEST().
> 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 will let Cyril decide on these :)
Thanks Xiao.
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
2020-05-05 8:44 ` Xiao Yang
@ 2020-05-12 14:25 ` Xiao Yang
0 siblings, 0 replies; 17+ messages in thread
From: Xiao Yang @ 2020-05-12 14:25 UTC (permalink / raw)
To: ltp
? 2020/5/5 16:44, Xiao Yang ??:
> On 5/5/20 11:28 AM, Viresh Kumar wrote:
>> On 04-05-20, 19:31, Xiao Yang wrote:
>>> On 5/4/20 1:09 PM, Viresh Kumar wrote:
>>>> On 30-04-20, 16:57, 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).
>>>>>
>>>>> Also avoid compiler warning by replacing (long) TST_RET with (int)
>>>>> pidfd:
>>>>> ------------------------------------------------------
>>>>> 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);
>>>> This log isn't useful as the warning started coming after your change
>>>> only and not before.
>>> Hi Viresh,
>>>
>>> Thanks for your review.
>>>
>>> Right?just add a hint why I use pidfd instead so I want to keep it.
>>>
>>> Of course?I will mention that my change introduces the compiler
>>> warning in
>>> v2 patch.
>> Even that isn't required really. You can add a variable if you like.
>
> Hi Viresh,
>
> Thanks a lot for your review.
>
> I prefer to keep it :-).
>
>>
>>>>> ------------------------------------------------------
>>>>>
>>>>> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
>>>>> ---
>>>>> .../kernel/syscalls/pidfd_open/pidfd_open01.c | 18 ++++++++++++++----
>>>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>>>>> b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>>>>> index 93bb86687..293e93b63 100644
>>>>> --- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>>>>> +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
>>>>> @@ -6,17 +6,27 @@
>>>>> * Basic pidfd_open() test, fetches the PID of the current process
>>>>> and tries to
>>>>> * get its file descriptor.
>>>>> */
>>>>> +
>>>>> +#include <sys/types.h>
>>>>> +#include <unistd.h>
>>>>> +#include <fcntl.h>
>>>>> #include "tst_test.h"
>>>>> #include "lapi/pidfd_open.h"
>>>>> static void run(void)
>>>>> {
>>>>> - TEST(pidfd_open(getpid(), 0));
>>>>> + int pidfd = 0, flag = 0;
>>>> None of these need to be initialized.
>>> Initialization or not initialization are both fine for me.
>>>
>>> Do you have any strong reason to drop Initialization?
>> Initializations are only required if there is a chance of the variable
>> getting used without being initialized, which isn't the case here.
>> Whatever value you set to these variables, they will get overwritten
>> anyway.
>
> Right, they will get overwritten anyway.
>
> As my previous reply said, either of them is OK for me so I can drop
> initializations as you suggested.
>
>>
>>>>> +
>>>>> + pidfd = pidfd_open(getpid(), 0);
>>>>> + if (pidfd == -1)
>>>>> + tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
>>>> This could have been written as:
>>>> TEST(pidfd = pidfd_open(getpid(), 0));
>>> Why do you want to keep TEST()? I don't think it is necessary:
>>>
>>> 1) pidfd and TERRNO are enough to check return value and errno.
>>>
>>> 2) It is OK for testcase to not use TEST().
>> As far as I have understood, that is the preferred way of doing it
>> from LTP maintainers.
>>
>> Over that it was already there, why remove it now ? Just fix the
>> problems you are trying to fix and that should be good.
>
> Hi Cyril,
>
> TEST() seems surplus after my change so I want to remove it directly.
>
> I wonder if it is necessary to keep TEST()?
Hi Cyril,
Do you have any comment on the doubt?
Best Regards,
Xiao Yang
>
> Thanks,
>
> Xiao Yang
>
>>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag
2020-05-05 9:30 ` Xiao Yang
2020-05-05 9:51 ` Viresh Kumar
@ 2020-05-12 14:32 ` Xiao Yang
1 sibling, 0 replies; 17+ messages in thread
From: Xiao Yang @ 2020-05-12 14:32 UTC (permalink / raw)
To: ltp
On 2020/5/5 17:30, Xiao Yang wrote:
>>>>> TST_CHECKPOINT_WAKE(0);
>>>>> @@ -49,8 +47,14 @@ static void run(void)
>>>>> tst_res(TPASS, "pidfd_open() passed");
>>>>> }
>>>>> +static void setup(void)
>>>>> +{
>>>>> + // Check if pidfd_open(2) is not supported
>>>>> + tst_syscall(__NR_pidfd_open, -1, 0);
>>>>> +}
>>>>> +
>>>>> static struct tst_test test = {
>>>>> - .min_kver = "5.3",
>>>>> + .setup = setup,
>>>>> .test_all = run,
>>>>> .forks_child = 1,
>>>>> .needs_checkpoints = 1,
>>>> Please have a look at fsopen_supported_by_kernel() in lapi/fsmount.h
>>>> and make such a helper.
>>> First, I want to explain my check point:
>>>
>>> Passing invalid argument can check the support of pidfd_open(2) by
>>> ENOSYS
>>> errno and we don't need to close the pidfd.
>>>
>>> Second, I don't like the implementation of
>>> fsopen_supported_by_kernel() and
>>> give some suggestions:
>>>
>>> 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.
>> I don't think kernel can drop support of syscalls just like that, we
>> can't break user space. And if that is done, we need to fix userspace
>> again separately for that.
>
> Hi Viresh,
>
> 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.
>
>>
>> We came to the implementation of fsopen_supported_by_kernel() after a
>> lot of reviews and decided on that and so I asked you for the sake of
>> keeping similar code throughout LTP (of course there will be old
>> usages which are different) to have a similar implementation.
>>
>> Anyway, I will leave it to Cyril to decide on that :)
>
> Hi Cyril,
>
> Do you have any comment on the implementation of
> fsopen_supported_by_kernel() and
>
> my check point(i.e. check the support of pidfd_open(2) by passing
> invalid argument ).
Hi Cyril,
Do you have comment on the support check of
pidfd_open()/fsopen_supported_by_kernel()?
Thank you in advance.
Best Regards,
Xiao Yang
^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag
2020-05-05 3:28 ` Viresh Kumar
2020-05-05 8:44 ` Xiao Yang
@ 2020-06-12 14:24 ` Cyril Hrubis
1 sibling, 0 replies; 17+ messages in thread
From: Cyril Hrubis @ 2020-06-12 14:24 UTC (permalink / raw)
To: ltp
Hi!
> > > > +
> > > > + pidfd = pidfd_open(getpid(), 0);
> > > > + if (pidfd == -1)
> > > > + tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
> > > This could have been written as:
> > > TEST(pidfd = pidfd_open(getpid(), 0));
> >
> > Why do you want to keep TEST()? I don't think it is necessary:
> >
> > 1) pidfd and TERRNO are enough to check return value and errno.
> >
> > 2) It is OK for testcase to not use TEST().
>
> As far as I have understood, that is the preferred way of doing it
> from LTP maintainers.
>
> Over that it was already there, why remove it now ? Just fix the
> problems you are trying to fix and that should be good.
I do not care that much if the test uses the macro or not, but you should
really keep changes separate from the removal of the kernel minimal version.
Also I guess that we can supress the warning by a cast in the SAFE_FCNTL()
macro so that we can pass long there.
This should do:
diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index c39d8768b..c153f163c 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -215,14 +215,14 @@ pid_t safe_getpgid(const char *file, const int lineno, pid_t pid);
({int tst_ret_ = ioctl(fd, request, ##__VA_ARGS__); \
tst_ret_ < 0 ? \
tst_brk(TBROK | TERRNO, \
- "ioctl(%i,%s,...) failed", fd, #request), 0 \
+ "ioctl(%i,%s,...) failed", (int)fd, #request), 0 \
: tst_ret_;})
#define SAFE_FCNTL(fd, cmd, ...) \
({int tst_ret_ = fcntl(fd, cmd, ##__VA_ARGS__); \
tst_ret_ == -1 ? \
tst_brk(TBROK | TERRNO, \
- "fcntl(%i,%s,...) failed", fd, #cmd), 0 \
+ "fcntl(%i,%s,...) failed", (int)fd, #cmd), 0 \
: tst_ret_;})
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag
2020-05-05 3:35 ` Viresh Kumar
2020-05-05 9:30 ` Xiao Yang
@ 2020-06-12 14:30 ` Cyril Hrubis
1 sibling, 0 replies; 17+ messages in thread
From: Cyril Hrubis @ 2020-06-12 14:30 UTC (permalink / raw)
To: ltp
Hi!
> > First, I want to explain my check point:
> >
> > Passing invalid argument can check the support of pidfd_open(2) by ENOSYS
> > errno and we don't need to close the pidfd.
> >
> > Second, I don't like the implementation of fsopen_supported_by_kernel() and
> > give some suggestions:
> >
> > 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
Hmm, man pidf_open says that it's implemented starting 5.3 or do I miss
something?
> > ??? if a kernel on distribution is newer than v5.2 but drop the support of
> > pidfd_open(2) on purpose.
>
> I don't think kernel can drop support of syscalls just like that, we
> can't break user space. And if that is done, we need to fix userspace
> again separately for that.
For most cases we cannot, there are a few that are guarded by CONFIG_
macros though e.g. SysV IPC.
> We came to the implementation of fsopen_supported_by_kernel() after a
> lot of reviews and decided on that and so I asked you for the sake of
> keeping similar code throughout LTP (of course there will be old
> usages which are different) to have a similar implementation.
>
> Anyway, I will leave it to Cyril to decide on that :)
Agree here, doing the check only if kernel version is not sufficient
seems to be the most reasoanble strategy here.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-06-12 14:30 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-30 8:57 [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag Xiao Yang
2020-04-30 8:57 ` [LTP] [PATCH 2/2] syscalls/pidfd_open*.c: Drop .min_kver flag Xiao Yang
2020-05-04 5:11 ` Viresh Kumar
2020-05-04 12:49 ` Xiao Yang
2020-05-05 3:35 ` Viresh Kumar
2020-05-05 9:30 ` Xiao Yang
2020-05-05 9:51 ` Viresh Kumar
2020-05-12 14:32 ` Xiao Yang
2020-06-12 14:30 ` Cyril Hrubis
2020-05-04 5:09 ` [LTP] [PATCH 1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag Viresh Kumar
2020-05-04 8:30 ` =?unknown-8bit?b?5p2o5pmT?=
2020-05-04 11:32 ` Xiao Yang
2020-05-04 11:31 ` Xiao Yang
2020-05-05 3:28 ` Viresh Kumar
2020-05-05 8:44 ` Xiao Yang
2020-05-12 14:25 ` Xiao Yang
2020-06-12 14:24 ` Cyril Hrubis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.