| Message ID | 1645005868-2373-4-git-send-email-xuyang2018.jy@fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] kcmp.h: move it to ltp include/lapi directory | expand |
Hi Xu, ... > +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c ... > + remotefd = TST_RET; > + flag = fcntl(remotefd, F_GETFD); > + if (flag == -1) > + tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed"); Just: flag = SAFE_FCNTL(remotefd, F_GETFD); > + if (!(flag & FD_CLOEXEC)) > + tst_res(TFAIL, "pidfd_getfd() didn't set close-on-exec flag"); > + > + TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); > + if (TST_RET != 0) > + tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description"); Maybe just: TST_EXP_PASS_SILENT(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); if (!TST_PASS) return; Although your version is more descriptive. > + > + TST_CHECKPOINT_WAKE(0); > + SAFE_CLOSE(remotefd); > + > + tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd); > + SAFE_CLOSE(pidfd); Shouldn't be pidfd closed in cleanup? In case fcntl() fails it's kept open. > + SAFE_CLOSE(fds[0]); The same is for fds, which is already static. These are very minor and you can change it before merge. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
Hi Xu, > +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore > @@ -0,0 +1 @@ > +pidfd_getfd01 /pidfd_getfd01 Kind regards, Petr
Hi Petr > Hi Xu, > > ... >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c > ... >> + remotefd = TST_RET; >> + flag = fcntl(remotefd, F_GETFD); >> + if (flag == -1) >> + tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed"); > Just: > flag = SAFE_FCNTL(remotefd, F_GETFD); Yes, I almost forgot we have this macro. > >> + if (!(flag& FD_CLOEXEC)) >> + tst_res(TFAIL, "pidfd_getfd() didn't set close-on-exec flag"); >> + >> + TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); >> + if (TST_RET != 0) >> + tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description"); > Maybe just: > TST_EXP_PASS_SILENT(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); I think we can't use this macro here see below(kcmp manpage about return value): 0 v1 is equal to v2; in other words, the two processes share the resource. 1 v1 is less than v2. 2 v1 is greater than v2. 3 v1 is not equal to v2, but ordering information is unavailable. On error, -1 is returned, and errno is set appropriately. So 1,2,3 are not invalid return value. TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); if (TST_RET==-1) { tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly"); goto free; } else { if (TST_RET < 0 || TST_RET > 3) tst_res(TFAIL, "kcmp invalid return value %ld", TST_RET); goto free; } free: .... > > if (!TST_PASS) > return; > Although your version is more descriptive. > >> + >> + TST_CHECKPOINT_WAKE(0); >> + SAFE_CLOSE(remotefd); >> + >> + tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd); >> + SAFE_CLOSE(pidfd); > Shouldn't be pidfd closed in cleanup? In case fcntl() fails it's kept open. Will do. >> + SAFE_CLOSE(fds[0]); > The same is for fds, which is already static. ok. > > These are very minor and you can change it before merge. > > Reviewed-by: Petr Vorel<pvorel@suse.cz> Thanks for your review. ps: I merged the first 3 patches, and will send a v2 for the remaining 2 patches. Best Regards Yang Xu > > Kind regards, > Petr
Hi Petr > Hi Xu, > >> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore >> @@ -0,0 +1 @@ >> +pidfd_getfd01 > /pidfd_getfd01 Thanks, will add it in v2. The two ways also work well. ps:pidfd_open also miss "/". Shoud we add this rule in maintainer review list and .gitignore info in doc? Best Regards Yang Xu > > Kind regards, > Petr
Hi Xu, > >> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore > >> @@ -0,0 +1 @@ > >> +pidfd_getfd01 > > /pidfd_getfd01 > Thanks, will add it in v2. > The two ways also work well. / causes pattern to be relative. Because we have mostly unique files, we wouldn't worry whether the pattern is relative or global. And we have 47 .gitignore files in testcases/ not following this rule :). > ps:pidfd_open also miss "/". Shoud we add this rule in maintainer review > list and .gitignore info in doc? I'm not sure myself if we want to follow this rule. If yes, it should be there. Kind regards, Petr > Best Regards > Yang Xu
Hi Richie, Xu, > Hi Petr > > Hi Xu, > > ... > >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c > > ... > >> + remotefd = TST_RET; > >> + flag = fcntl(remotefd, F_GETFD); > >> + if (flag == -1) > >> + tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed"); > > Just: > > flag = SAFE_FCNTL(remotefd, F_GETFD); > Yes, I almost forgot we have this macro. @Richie: It'd be useful if sparse checks would suggest to use SAFE_*() functions, but not sure if easily detectable. Something like setup() and cleanup() function and syscall followed by if (foo == -1) followed by tst_brk(). Kind regards, Petr
Hi Xu, ... > >> + TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); > >> + if (TST_RET != 0) > >> + tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description"); > > Maybe just: > > TST_EXP_PASS_SILENT(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); > I think we can't use this macro here see below(kcmp manpage about return > value): > 0 v1 is equal to v2; in other words, the two processes share the resource. > 1 v1 is less than v2. > 2 v1 is greater than v2. > 3 v1 is not equal to v2, but ordering information is unavailable. > On error, -1 is returned, and errno is set appropriately. > So 1,2,3 are not invalid return value. I'm sorry, you're right. > TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); > if (TST_RET==-1) { > tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly"); > goto free; > } else { > if (TST_RET < 0 || TST_RET > 3) +1 (very nit: instead of if/else I'd use 2x if and (TST_RET < -1 || TST_RET > 3) - readability). Kind regards, Petr > tst_res(TFAIL, "kcmp invalid return value %ld", TST_RET); > goto free; > } > free: > ....
Hi Petr > Hi Xu, > > ... >>>> + TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); >>>> + if (TST_RET != 0) >>>> + tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description"); >>> Maybe just: >>> TST_EXP_PASS_SILENT(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); >> I think we can't use this macro here see below(kcmp manpage about return >> value): >> 0 v1 is equal to v2; in other words, the two processes share the resource. > >> 1 v1 is less than v2. > >> 2 v1 is greater than v2. > >> 3 v1 is not equal to v2, but ordering information is unavailable. > >> On error, -1 is returned, and errno is set appropriately. > >> So 1,2,3 are not invalid return value. > I'm sorry, you're right. > >> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); >> if (TST_RET==-1) { >> tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly"); >> goto free; >> } else { >> if (TST_RET< 0 || TST_RET> 3) > +1 (very nit: instead of if/else I'd use 2x if and (TST_RET< -1 || TST_RET> 3) > - readability). Good catch. I add the last "if" otherwise 1,2,3 are ignored. TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); if (TST_RET == -1) { tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly"); goto free; } if (TST_RET < 0 || TST_RET > 3) { tst_res(TFAIL, "kcmp invalid returns value(%d)", (int)TST_RET); goto free; } if (TST_RET != 0) { tst_res(TFAIL, "kcmp returns unexpected value(%d) instead of 0", (int)TST_RET); goto free; } tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd); free: Best Regards Yang Xu > > Kind regards, > Petr > >> tst_res(TFAIL, "kcmp invalid return value %ld", TST_RET); >> goto free; >> } > >> free: >> ....
Hi Xu, ... > >> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); > >> if (TST_RET==-1) { > >> tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly"); > >> goto free; > >> } else { > >> if (TST_RET< 0 || TST_RET> 3) > > +1 (very nit: instead of if/else I'd use 2x if and (TST_RET< -1 || TST_RET> 3) > > - readability). > Good catch. I add the last "if" otherwise 1,2,3 are ignored. > TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); > if (TST_RET == -1) { > tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly"); > goto free; > } > if (TST_RET < 0 || TST_RET > 3) { > tst_res(TFAIL, "kcmp invalid returns value(%d)", > (int)TST_RET); > goto free; > } > if (TST_RET != 0) { > tst_res(TFAIL, "kcmp returns unexpected value(%d) > instead of 0", > (int)TST_RET); > goto free; > } > tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd); FYI Unless we want to have explanations why it failed, we could use TST_EXP_VAL() or TST_EXP_VAL_SILENT(). Kind regards, Petr
Hello Petr, Petr Vorel <pvorel@suse.cz> writes: > Hi Richie, Xu, > >> Hi Petr >> > Hi Xu, > >> > ... >> >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c >> > ... >> >> + remotefd = TST_RET; >> >> + flag = fcntl(remotefd, F_GETFD); >> >> + if (flag == -1) >> >> + tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed"); >> > Just: >> > flag = SAFE_FCNTL(remotefd, F_GETFD); >> Yes, I almost forgot we have this macro. > @Richie: It'd be useful if sparse checks would suggest to use SAFE_*() > functions, but not sure if easily detectable. Something like setup() and > cleanup() function and syscall followed by if (foo == -1) followed by tst_brk(). > > Kind regards, > Petr Yeah, this would be relatively simple in Coccinelle. In Sparse I'm not sure, but it is one of the main use cases IMO. Hopefully the IR produced by these code patterns is fairly stable. In that case we can do some simple pattern matching.
Hi Richie, > Hello Petr, > Petr Vorel <pvorel@suse.cz> writes: > > Hi Richie, Xu, > >> Hi Petr > >> > Hi Xu, > >> > ... > >> >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c > >> > ... > >> >> + remotefd = TST_RET; > >> >> + flag = fcntl(remotefd, F_GETFD); > >> >> + if (flag == -1) > >> >> + tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed"); > >> > Just: > >> > flag = SAFE_FCNTL(remotefd, F_GETFD); > >> Yes, I almost forgot we have this macro. > > @Richie: It'd be useful if sparse checks would suggest to use SAFE_*() > > functions, but not sure if easily detectable. Something like setup() and > > cleanup() function and syscall followed by if (foo == -1) followed by tst_brk(). > > Kind regards, > > Petr > Yeah, this would be relatively simple in Coccinelle. In Sparse I'm not > sure, but it is one of the main use cases IMO. I remember few years ago (2017) metan did big cleanup with Coccinelle. If much simpler, we might want to introduce some basic scripts (without vendoring Coccinelle, it'd have to be installed). > Hopefully the IR produced by these code patterns is fairly stable. In > that case we can do some simple pattern matching. Frankly speaking, if ever implemented it's probably you who is going to implement it, thus your choice what you use :). Kind regards, Petr
Hello Petr, Petr Vorel <pvorel@suse.cz> writes: > Hi Richie, > >> Hello Petr, > >> Petr Vorel <pvorel@suse.cz> writes: > >> > Hi Richie, Xu, > >> >> Hi Petr >> >> > Hi Xu, > >> >> > ... >> >> >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c >> >> > ... >> >> >> + remotefd = TST_RET; >> >> >> + flag = fcntl(remotefd, F_GETFD); >> >> >> + if (flag == -1) >> >> >> + tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed"); >> >> > Just: >> >> > flag = SAFE_FCNTL(remotefd, F_GETFD); >> >> Yes, I almost forgot we have this macro. >> > @Richie: It'd be useful if sparse checks would suggest to use SAFE_*() >> > functions, but not sure if easily detectable. Something like setup() and >> > cleanup() function and syscall followed by if (foo == -1) followed by tst_brk(). > >> > Kind regards, >> > Petr > >> Yeah, this would be relatively simple in Coccinelle. In Sparse I'm not >> sure, but it is one of the main use cases IMO. > I remember few years ago (2017) metan did big cleanup with Coccinelle. > If much simpler, we might want to introduce some basic scripts (without > vendoring Coccinelle, it'd have to be installed). Indeed, we already have some in scripts/coccinelle. These are mainly useful for refactoring. > >> Hopefully the IR produced by these code patterns is fairly stable. In >> that case we can do some simple pattern matching. > Frankly speaking, if ever implemented it's probably you who is going to > implement it, thus your choice what you use :). > > Kind regards, > Petr Yeah, although it still needs reviewing :-p
Hi Petr > Hi Xu, > > ... >>>> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); >>>> if (TST_RET==-1) { >>>> tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly"); >>>> goto free; >>>> } else { >>>> if (TST_RET< 0 || TST_RET> 3) >>> +1 (very nit: instead of if/else I'd use 2x if and (TST_RET< -1 || TST_RET> 3) >>> - readability). >> Good catch. I add the last "if" otherwise 1,2,3 are ignored. > >> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); >> if (TST_RET == -1) { >> tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly"); >> goto free; >> } >> if (TST_RET< 0 || TST_RET> 3) { >> tst_res(TFAIL, "kcmp invalid returns value(%d)", >> (int)TST_RET); >> goto free; >> } >> if (TST_RET != 0) { >> tst_res(TFAIL, "kcmp returns unexpected value(%d) >> instead of 0", >> (int)TST_RET); >> goto free; >> } > >> tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd); > > FYI Unless we want to have explanations why it failed, we could use TST_EXP_VAL() or > TST_EXP_VAL_SILENT(). Ok, I will use TST_EXP_VAL_SILENT. Best Regards Yang Xu > > Kind regards, > Petr
diff --git a/runtest/syscalls b/runtest/syscalls index ce6f89f88..6155712cc 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -951,6 +951,8 @@ pause03 pause03 personality01 personality01 personality02 personality02 +pidfd_getfd01 pidfd_getfd01 + pidfd_open01 pidfd_open01 pidfd_open02 pidfd_open02 pidfd_open03 pidfd_open03 diff --git a/testcases/kernel/syscalls/pidfd_getfd/.gitignore b/testcases/kernel/syscalls/pidfd_getfd/.gitignore new file mode 100644 index 000000000..c193ffee7 --- /dev/null +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore @@ -0,0 +1 @@ +pidfd_getfd01 diff --git a/testcases/kernel/syscalls/pidfd_getfd/Makefile b/testcases/kernel/syscalls/pidfd_getfd/Makefile new file mode 100644 index 000000000..5ea7d67db --- /dev/null +++ b/testcases/kernel/syscalls/pidfd_getfd/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +top_srcdir ?= ../../../.. + +include $(top_srcdir)/include/mk/testcases.mk +include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c new file mode 100644 index 000000000..f3f5132fe --- /dev/null +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved. + * Author: Yang Xu <xuyang2018.jy@fujitsu.com> + */ + +/*\ + * [Description] + * + * Basic pidfd_getfd() test: + * + * - the close-on-exec flag is set on the file descriptor returned by + * pidfd_getfd + * - use kcmp to check whether a file descriptor idx1 in the process pid1 + * refers to the same open file description as file descriptor idx2 in + * the process pid2 + */ + +#include <unistd.h> +#include <fcntl.h> +#include <stdlib.h> +#include <stdio.h> +#include "tst_test.h" +#include "lapi/pidfd_open.h" +#include "lapi/pidfd_getfd.h" +#include "lapi/kcmp.h" +#include "tst_safe_macros.h" + +#define TESTFILE "testfile" + +static int fds[2]; + +static void do_child(void) +{ + int fd; + + SAFE_CLOSE(fds[0]); + fd = SAFE_CREAT(TESTFILE, 0644); + SAFE_WRITE(1, fds[1], &fd, sizeof(fd)); + TST_CHECKPOINT_WAIT(0); + SAFE_CLOSE(fd); + SAFE_CLOSE(fds[1]); + exit(0); +} + +static void run(void) +{ + int flag, pid, pidfd, targetfd, remotefd; + + SAFE_PIPE(fds); + pid = SAFE_FORK(); + if (!pid) + do_child(); + + SAFE_CLOSE(fds[1]); + TST_PROCESS_STATE_WAIT(pid, 'S', 0); + + TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open(pid, 0)"); + pidfd = TST_RET; + SAFE_READ(1, fds[0], &targetfd, sizeof(targetfd)); + TST_EXP_FD_SILENT(pidfd_getfd(pidfd, targetfd, 0), + "pidfd_getfd(%d, %d , 0)", pidfd, targetfd); + + remotefd = TST_RET; + flag = fcntl(remotefd, F_GETFD); + if (flag == -1) + tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed"); + if (!(flag & FD_CLOEXEC)) + tst_res(TFAIL, "pidfd_getfd() didn't set close-on-exec flag"); + + TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd)); + if (TST_RET != 0) + tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description"); + + TST_CHECKPOINT_WAKE(0); + SAFE_CLOSE(remotefd); + + tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd); + SAFE_CLOSE(pidfd); + SAFE_CLOSE(fds[0]); + tst_reap_children(); +} + +static void setup(void) +{ + pidfd_open_supported(); + pidfd_getfd_supported(); +} + +static struct tst_test test = { + .needs_root = 1, + .needs_checkpoints = 1, + .forks_child = 1, + .setup = setup, + .test_all = run, +};
The pidfd_getfd() system call allocates a new file descriptor in the calling process. This new file descriptor is a duplicate of an existing file descriptor, targetfd, in the process referred to by the PID file descriptor pidfd. pidfd_getfd was introduced since kernel 5.6 and pidfd_open was introduced since kernel 5.3. pidfd_getfd is not similar to pidfd_send_signal because it didn't support fd from /proc/pid directory. So we must check kernel whether support pidfd_open because some linux distribution backports pidfd_getfd but not backport pidfd_open. Here we check whether the newfd set close-on-exce flag and use kcmp to check the two fds whether point to the same open file instead of using fstat to check. Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- runtest/syscalls | 2 + .../kernel/syscalls/pidfd_getfd/.gitignore | 1 + .../kernel/syscalls/pidfd_getfd/Makefile | 6 ++ .../syscalls/pidfd_getfd/pidfd_getfd01.c | 96 +++++++++++++++++++ 4 files changed, 105 insertions(+) create mode 100644 testcases/kernel/syscalls/pidfd_getfd/.gitignore create mode 100644 testcases/kernel/syscalls/pidfd_getfd/Makefile create mode 100644 testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c