| Message ID | 4dd4dabd2cd574dc2657c5926e8e3d1a0c8a8ae6.1579259595.git.viresh.kumar@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] Add Syscall numbers for pidfd_open | expand |
Hi! > diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c > new file mode 100644 > index 000000000000..b67393bcafa2 > --- /dev/null > +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c > @@ -0,0 +1,68 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> > + * > + * Description: > + * This program opens the PID file descriptor of the child process created with > + * fork(). It then uses poll to monitor the file descriptor for process exit, as > + * indicated by an EPOLLIN event. > + */ > +#include <poll.h> > +#include <sys/types.h> > +#include <sys/syscall.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <unistd.h> > + > +#include "tst_test.h" > +#include "lapi/pidfd_open.h" > +#include "lapi/syscalls.h" > + > +static void run(void) > +{ > + struct pollfd pollfd; > + int p_id, fd, ready; > + > + TEST(fork()); > + > + if (TST_RET == -1) { > + tst_res(TFAIL, "fork() Failed"); > + tst_res(TBROK, "unable to continue"); > + } We do have SAFE_FORK() make use of that one instead. > + if (TST_RET == 0) { > + /* child */ > + usleep(1000); This is inherently racy, the process should not exit until the parent has called pidfd_open(). We do have a checkpoints in the test library that can synchronize between processes, generally it should look like: child: checkpoint_wait() exit() parent: pidfd_open() checkpoint_wake() > + exit(EXIT_SUCCESS); > + } else { > + /* parent */ > + p_id = TST_RET; > + > + TEST(pidfd_open(p_id, 0)); > + > + fd = TST_RET; > + if (fd == -1) { > + tst_res(TFAIL, "Cannot retrieve file descriptor to the child process"); ^ tst_res(TFAIL | TTERRNO, "pidfd_open() failed"); > + return; > + } > + > + pollfd.fd = fd; > + pollfd.events = POLLIN; > + > + ready = poll(&pollfd, 1, -1); > + if (ready == -1) { > + tst_res(TFAIL, "poll() Failed"); > + tst_res(TBROK, "unable to continue"); There is absolutely no need to print two messages on a failure, so this should just be tst_brk(TBROK | TERRNO, "poll() failed"); Also note that tst_brk() _EXITS_ the test, which is what you want here. > + } > + > + printf("Events (0x%x): POLLIN is %sset\n", pollfd.revents, > + (pollfd.revents & POLLIN) ? "" : "not "); No printf in tests, use tst_res(TINFO, ...) instead. Also we should check here that we got the event and that the child is dead at this point, or something along these lines. There does not seem to be single TPASS in this tests, which itself will report that the test is broken when it's executed. > + SAFE_CLOSE(fd); > + } > +} > + > +static struct tst_test test = { > + .min_kver = "5.3", > + .test_all = run, > +}; And we are also missing third test here, that checks various error condigitions such as flags != 0, invalid pid, etc.
On 21-01-20, 16:39, Cyril Hrubis wrote: > And we are also missing third test here, that checks various error > condigitions such as flags != 0, invalid pid, etc. Hi Cyril, Thanks for the feedback and sorry about all the mistakes as it was my very first attempt at running/updating ltp code :) Please have a look at the updated patch pasted below and let me know if anything is still missing.
On 22-01-20, 10:42, Viresh Kumar wrote: > From: Viresh Kumar <viresh.kumar@linaro.org> > Date: Thu, 16 Jan 2020 16:47:01 +0530 > Subject: [PATCH] syscalls/pidfd_open > > Add tests to check working of pidfd_open() syscall. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > configure.ac | 1 + > include/lapi/pidfd_open.h | 21 ++++++ > runtest/syscalls | 3 + > .../kernel/syscalls/pidfd_open/.gitignore | 2 + > testcases/kernel/syscalls/pidfd_open/Makefile | 6 ++ > .../kernel/syscalls/pidfd_open/pidfd_open01.c | 38 +++++++++++ > .../kernel/syscalls/pidfd_open/pidfd_open02.c | 45 +++++++++++++ > .../kernel/syscalls/pidfd_open/pidfd_open03.c | 64 +++++++++++++++++++ > 8 files changed, 180 insertions(+) > create mode 100644 include/lapi/pidfd_open.h > create mode 100644 testcases/kernel/syscalls/pidfd_open/.gitignore > create mode 100644 testcases/kernel/syscalls/pidfd_open/Makefile > create mode 100644 testcases/kernel/syscalls/pidfd_open/pidfd_open01.c > create mode 100644 testcases/kernel/syscalls/pidfd_open/pidfd_open02.c > create mode 100644 testcases/kernel/syscalls/pidfd_open/pidfd_open03.c + this as well (I will resent the patches properly again after you have had a chance to look at them once). diff --git a/runtest/syscalls b/runtest/syscalls index 9d6d288780a3..a2d749d526a8 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -848,6 +848,7 @@ personality02 personality02 pidfd_open01 pidfd_open01 pidfd_open02 pidfd_open02 +pidfd_open03 pidfd_open03 pidfd_send_signal01 pidfd_send_signal01 pidfd_send_signal02 pidfd_send_signal02 diff --git a/testcases/kernel/syscalls/pidfd_open/.gitignore b/testcases/kernel/syscalls/pidfd_open/.gitignore index be218f88647d..e0b8900c1c33 100644 --- a/testcases/kernel/syscalls/pidfd_open/.gitignore +++ b/testcases/kernel/syscalls/pidfd_open/.gitignore @@ -1,2 +1,3 @@ pidfd_open01 pidfd_open02 +pidfd_open03
Hi! > > And we are also missing third test here, that checks various error > > condigitions such as flags != 0, invalid pid, etc. > > Hi Cyril, > > Thanks for the feedback and sorry about all the mistakes as it was my > very first attempt at running/updating ltp code :) It takes time to learn... Also welcome :-). > Please have a look at the updated patch pasted below and let me know > if anything is still missing. It's usuall to send updated patches as new version, i.e. this should be called [PATCH v2] and just send to the ML as the previous one. So the next one should be [PATCH v3] ... > -------------------------8<------------------------- > From: Viresh Kumar <viresh.kumar@linaro.org> > Date: Thu, 16 Jan 2020 16:47:01 +0530 > Subject: [PATCH] syscalls/pidfd_open > > Add tests to check working of pidfd_open() syscall. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > configure.ac | 1 + > include/lapi/pidfd_open.h | 21 ++++++ > runtest/syscalls | 3 + > .../kernel/syscalls/pidfd_open/.gitignore | 2 + > testcases/kernel/syscalls/pidfd_open/Makefile | 6 ++ > .../kernel/syscalls/pidfd_open/pidfd_open01.c | 38 +++++++++++ > .../kernel/syscalls/pidfd_open/pidfd_open02.c | 45 +++++++++++++ > .../kernel/syscalls/pidfd_open/pidfd_open03.c | 64 +++++++++++++++++++ > 8 files changed, 180 insertions(+) > create mode 100644 include/lapi/pidfd_open.h > create mode 100644 testcases/kernel/syscalls/pidfd_open/.gitignore > create mode 100644 testcases/kernel/syscalls/pidfd_open/Makefile > create mode 100644 testcases/kernel/syscalls/pidfd_open/pidfd_open01.c > create mode 100644 testcases/kernel/syscalls/pidfd_open/pidfd_open02.c > create mode 100644 testcases/kernel/syscalls/pidfd_open/pidfd_open03.c > > diff --git a/configure.ac b/configure.ac > index 50d14967d3c6..1bf0911d88ad 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -79,6 +79,7 @@ AC_CHECK_FUNCS([ \ > mknodat \ > name_to_handle_at \ > openat \ > + pidfd_open \ > pidfd_send_signal \ > pkey_mprotect \ > preadv \ > diff --git a/include/lapi/pidfd_open.h b/include/lapi/pidfd_open.h > new file mode 100644 > index 000000000000..ced163be83bf > --- /dev/null > +++ b/include/lapi/pidfd_open.h > @@ -0,0 +1,21 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2020 Linaro Limited. All rights reserved. > + * Author: Viresh Kumar <viresh.kumar@linaro.org> > + */ > + > +#ifndef PIDFD_OPEN_H > +#define PIDFD_OPEN_H > + > +#include "config.h" > +#include <sys/types.h> > +#include "lapi/syscalls.h" > + > +#ifndef HAVE_PIDFD_OPEN > +int pidfd_open(pid_t pid, unsigned int flags) > +{ > + return tst_syscall(__NR_pidfd_open, pid, flags); > +} > +#endif > + > +#endif /* PIDFD_OPEN_H */ > diff --git a/runtest/syscalls b/runtest/syscalls > index fa87ef63fbc1..9d6d288780a3 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -846,6 +846,9 @@ pause03 pause03 > personality01 personality01 > personality02 personality02 > > +pidfd_open01 pidfd_open01 > +pidfd_open02 pidfd_open02 > + > pidfd_send_signal01 pidfd_send_signal01 > pidfd_send_signal02 pidfd_send_signal02 > pidfd_send_signal03 pidfd_send_signal03 > diff --git a/testcases/kernel/syscalls/pidfd_open/.gitignore b/testcases/kernel/syscalls/pidfd_open/.gitignore > new file mode 100644 > index 000000000000..be218f88647d > --- /dev/null > +++ b/testcases/kernel/syscalls/pidfd_open/.gitignore > @@ -0,0 +1,2 @@ > +pidfd_open01 > +pidfd_open02 These two files now miss pidfd_open03 > diff --git a/testcases/kernel/syscalls/pidfd_open/Makefile b/testcases/kernel/syscalls/pidfd_open/Makefile > new file mode 100644 > index 000000000000..5ea7d67db123 > --- /dev/null > +++ b/testcases/kernel/syscalls/pidfd_open/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_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c > new file mode 100644 > index 000000000000..2ca22ae3fb92 > --- /dev/null > +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c > @@ -0,0 +1,38 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> > + * > + * Description: > + * Basic pidfd_open() test, fetches the PID of the current process and tries to > + * get its file descriptor. > + */ > +#include <sys/types.h> > +#include <sys/syscall.h> > + > +#include "tst_test.h" > +#include "lapi/pidfd_open.h" > +#include "lapi/syscalls.h" > + > +static void run(void) > +{ > + int pid, fd; > + > + pid = getpid(); > + > + TEST(pidfd_open(pid, 0)); > + > + fd = TST_RET; > + if (fd == -1) { > + tst_res(TFAIL, "Cannot retrieve file descriptor to the current process"); ^ This is still missing the | TTERRNO bitflag so that the message prints errno as well Also the whole message could be shorter and to the point: "pidfd_open(getpid(), 0) failed" > + return; > + } > + > + SAFE_CLOSE(fd); > + > + tst_res(TPASS, "Retrieved file descriptor to the current process"); Here as well. > +} > + > +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 > new file mode 100644 > index 000000000000..ab2f86a31392 > --- /dev/null > +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c > @@ -0,0 +1,45 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> > + * > + * Description: > + * Basic pidfd_open() test to test invalid arguments. > + */ > +#include <sys/types.h> > +#include <sys/syscall.h> > + > +#include "tst_test.h" > +#include "lapi/pidfd_open.h" > +#include "lapi/syscalls.h" > + > +static void run(void) > +{ > + int pid, fd; > + > + /* Invalid pid */ > + pid = -1; > + > + fd = pidfd_open(pid, 0); > + if (fd != -1) { We have to check if the errno is correct here as well, also we usually use the TEST() macro that saves return value and errno at the same time. > + SAFE_CLOSE(fd); > + tst_res(TFAIL, "pidfd_open() didn't recognize invalid pid"); > + return; > + } > + > + pid = getpid(); > + > + /* Invalid flags */ > + fd = pidfd_open(pid, 1); > + if (fd != -1) { > + SAFE_CLOSE(fd); > + tst_res(TFAIL, "pidfd_open() didn't recognize invalid flags"); > + return; > + } > + > + tst_res(TPASS, "pidfd_open() failed for invalid arguments"); > +} > + > +static struct tst_test test = { > + .min_kver = "5.3", > + .test_all = run, > +}; These kid of tests are usually written in a way that the parameters are stored in a structure and the test function is called with increasing index to loop over all tests. Have a look at open/open02.c for example. But looking at the pidfd_open() manual, the only error that is missing here and reasonable to test here is ESRCH. Hint: see tst_get_unused_pid() > diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c > new file mode 100644 > index 000000000000..56861dfc014a > --- /dev/null > +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open03.c > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> > + * > + * Description: > + * This program opens the PID file descriptor of the child process created with > + * fork(). It then uses poll to monitor the file descriptor for process exit, as > + * indicated by an EPOLLIN event. > + */ > +#include <poll.h> > +#include <sys/types.h> > +#include <sys/syscall.h> > +#include <stdio.h> > +#include <stdlib.h> > + > +#include "tst_test.h" > +#include "lapi/pidfd_open.h" > +#include "lapi/syscalls.h" > + > +static void run(void) > +{ > + struct pollfd pollfd; > + int pid, fd, ready; > + > + pid = SAFE_FORK(); > + > + if (!pid) { > + /* child */ > + TST_CHECKPOINT_WAIT(0); > + exit(EXIT_SUCCESS); > + } else { > + /* parent */ These /* parent */ and /* child */ comments fall into "commenting the obvious" categrogy please avoid doing so. > + TEST(pidfd_open(pid, 0)); > + > + fd = TST_RET; > + if (fd == -1) { > + tst_res(TFAIL | TTERRNO, "pidfd_open() failed"); > + return; > + } > + > + TST_CHECKPOINT_WAKE(0); > + > + pollfd.fd = fd; > + pollfd.events = POLLIN; > + > + ready = poll(&pollfd, 1, -1); > + > + SAFE_CLOSE(fd); > + SAFE_WAITPID(pid, NULL, 0); > + > + if (ready == -1) > + tst_brk(TBROK | TERRNO, "poll() failed"); I guess that we may as well check that the ready is exactly 1 here as well, but that's a minor thing. > + tst_res(TPASS, "Events (0x%x): POLLIN is %sset\n", > + pollfd.revents, (pollfd.revents & POLLIN) ? "" : "not"); > + } > +} > + > +static struct tst_test test = { > + .min_kver = "5.3", > + .test_all = run, > + .forks_child = 1, > + .needs_checkpoints = 1, > +};
diff --git a/configure.ac b/configure.ac index 50d14967d3c6..1bf0911d88ad 100644 --- a/configure.ac +++ b/configure.ac @@ -79,6 +79,7 @@ AC_CHECK_FUNCS([ \ mknodat \ name_to_handle_at \ openat \ + pidfd_open \ pidfd_send_signal \ pkey_mprotect \ preadv \ diff --git a/include/lapi/pidfd_open.h b/include/lapi/pidfd_open.h new file mode 100644 index 000000000000..ced163be83bf --- /dev/null +++ b/include/lapi/pidfd_open.h @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 Linaro Limited. All rights reserved. + * Author: Viresh Kumar <viresh.kumar@linaro.org> + */ + +#ifndef PIDFD_OPEN_H +#define PIDFD_OPEN_H + +#include "config.h" +#include <sys/types.h> +#include "lapi/syscalls.h" + +#ifndef HAVE_PIDFD_OPEN +int pidfd_open(pid_t pid, unsigned int flags) +{ + return tst_syscall(__NR_pidfd_open, pid, flags); +} +#endif + +#endif /* PIDFD_OPEN_H */ diff --git a/runtest/syscalls b/runtest/syscalls index fa87ef63fbc1..9d6d288780a3 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -846,6 +846,9 @@ pause03 pause03 personality01 personality01 personality02 personality02 +pidfd_open01 pidfd_open01 +pidfd_open02 pidfd_open02 + pidfd_send_signal01 pidfd_send_signal01 pidfd_send_signal02 pidfd_send_signal02 pidfd_send_signal03 pidfd_send_signal03 diff --git a/testcases/kernel/syscalls/pidfd_open/.gitignore b/testcases/kernel/syscalls/pidfd_open/.gitignore new file mode 100644 index 000000000000..be218f88647d --- /dev/null +++ b/testcases/kernel/syscalls/pidfd_open/.gitignore @@ -0,0 +1,2 @@ +pidfd_open01 +pidfd_open02 diff --git a/testcases/kernel/syscalls/pidfd_open/Makefile b/testcases/kernel/syscalls/pidfd_open/Makefile new file mode 100644 index 000000000000..5ea7d67db123 --- /dev/null +++ b/testcases/kernel/syscalls/pidfd_open/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_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c new file mode 100644 index 000000000000..4e3417c487a6 --- /dev/null +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> + * + * Description: + * Basic pidfd_open() test, fetches the PID of the current process and tries to + * get its file descriptor. + */ +#include <sys/types.h> +#include <sys/syscall.h> + +#include "tst_test.h" +#include "lapi/pidfd_open.h" +#include "lapi/syscalls.h" + +static void run(void) +{ + int p_id, fd; + + p_id = getpid(); + + TEST(pidfd_open(p_id, 0)); + + fd = TST_RET; + if (fd == -1) { + tst_res(TFAIL, "Cannot retrieve file descriptor to the current process"); + return; + } + + SAFE_CLOSE(fd); + + tst_res(TPASS, "Retrieved file descriptor to the current process"); +} + +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 new file mode 100644 index 000000000000..b67393bcafa2 --- /dev/null +++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open02.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org> + * + * Description: + * This program opens the PID file descriptor of the child process created with + * fork(). It then uses poll to monitor the file descriptor for process exit, as + * indicated by an EPOLLIN event. + */ +#include <poll.h> +#include <sys/types.h> +#include <sys/syscall.h> +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +#include "tst_test.h" +#include "lapi/pidfd_open.h" +#include "lapi/syscalls.h" + +static void run(void) +{ + struct pollfd pollfd; + int p_id, fd, ready; + + TEST(fork()); + + if (TST_RET == -1) { + tst_res(TFAIL, "fork() Failed"); + tst_res(TBROK, "unable to continue"); + } + + if (TST_RET == 0) { + /* child */ + usleep(1000); + exit(EXIT_SUCCESS); + } else { + /* parent */ + p_id = TST_RET; + + TEST(pidfd_open(p_id, 0)); + + fd = TST_RET; + if (fd == -1) { + tst_res(TFAIL, "Cannot retrieve file descriptor to the child process"); + return; + } + + pollfd.fd = fd; + pollfd.events = POLLIN; + + ready = poll(&pollfd, 1, -1); + if (ready == -1) { + tst_res(TFAIL, "poll() Failed"); + tst_res(TBROK, "unable to continue"); + } + + printf("Events (0x%x): POLLIN is %sset\n", pollfd.revents, + (pollfd.revents & POLLIN) ? "" : "not "); + + SAFE_CLOSE(fd); + } +} + +static struct tst_test test = { + .min_kver = "5.3", + .test_all = run, +};
Add tests to check working of pidfd_open() syscall. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- configure.ac | 1 + include/lapi/pidfd_open.h | 21 ++++++ runtest/syscalls | 3 + .../kernel/syscalls/pidfd_open/.gitignore | 2 + testcases/kernel/syscalls/pidfd_open/Makefile | 6 ++ .../kernel/syscalls/pidfd_open/pidfd_open01.c | 38 +++++++++++ .../kernel/syscalls/pidfd_open/pidfd_open02.c | 68 +++++++++++++++++++ 7 files changed, 139 insertions(+) create mode 100644 include/lapi/pidfd_open.h create mode 100644 testcases/kernel/syscalls/pidfd_open/.gitignore create mode 100644 testcases/kernel/syscalls/pidfd_open/Makefile create mode 100644 testcases/kernel/syscalls/pidfd_open/pidfd_open01.c create mode 100644 testcases/kernel/syscalls/pidfd_open/pidfd_open02.c