| Message ID | 20200430085742.1663-2-yangx.jy@cn.fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] syscalls/pidfd_open01.c: Add check for close-on-exec flag | expand |
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.
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 >
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 :)
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 >
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.
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
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.
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,
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(-)