[LTP] [PATCH v2] Add test case to cover the setting resource limit64 for process
Chunfu Wen
chwen@redhat.com
Thu Feb 20 09:01:19 CET 2025
Hello Petr,
Thanks for reviewing.
See inline comments.
Chunfu Wen
On Thu, Feb 20, 2025 at 12:45 AM Petr Vorel <pvorel@suse.cz> wrote:
> Hi Chunfu Wen,
>
> Besides Andrea's comments, there are other things to improve.
>
[chwen] merge all things into setrlimit06.c, and delete setrlimit07.c
>
>
> > The test ensures that the process gets the correct signals in the
> correct order:
>
> > First, it should get SIGXCPU after reaching the soft CPU time limit64.
> > Then, if the CPU time exceeds the hard limit, it should receive SIGKILL
>
> > Signed-off-by: chunfuwen <chwen@redhat.com>
> > ---
> > Changes in v2:
> > - Romove test descriptions and trailing line as suggested by Ricardo B.
> Marlière
> > - Added 2025 copyright as suggested by Ricardo B. Marlière
> > - Trim down include files as suggested by Ricardo B. Marlière
> > - Create new lapi/resource.h residing struct rlimit64 as suggested by
> Andrea
> > - Move setrlimit_u64() syscalls definitions into lapi/resource.h as
> suggested by Andrea
> > - Skip SAFE_* variants as suggested by Andrea
> > - use tst_buffers when passing the pointeras suggested by Andrea
> > - Link to v1:
> https://lore.kernel.org/all/20250218023107.1208990-1-chwen@redhat.com/
> > ---
> > include/lapi/resource.h | 26 ++++
> > .../kernel/syscalls/setrlimit/setrlimit07.c | 127 ++++++++++++++++++
> > 2 files changed, 153 insertions(+)
> > create mode 100644 include/lapi/resource.h
> > create mode 100644 testcases/kernel/syscalls/setrlimit/setrlimit07.c
>
> > diff --git a/include/lapi/resource.h b/include/lapi/resource.h
> > new file mode 100644
> > index 000000000..3310bc934
> > --- /dev/null
> > +++ b/include/lapi/resource.h
> > @@ -0,0 +1,26 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2025 Red Hat Inc. All Rights Reserved.
> > + * Author: Chunfu Wen <chwen@redhat.com>
> > + */
> > +
> > +#ifndef LAPI_RESOURCE_H__
> > +#define LAPI_RESOURCE_H__
> > +
> There should be at the top
>
> #include "config.h"
> otherwise #ifndef HAVE_STRUCT_RLIMIT64 is always false.
>
[chwen] Done.
>
> > +#define _GNU_SOURCE
> > +
> > +#include "lapi/syscalls.h"
>
> This should include some header, I guess:
> #include <sys/resource.h>
>
[chwen] added
>
> because there is already a check.
> AC_CHECK_TYPES([struct rlimit64],,,[
> #define _LARGEFILE64_SOURCE
> #include <sys/resource.h>
> ])
>
> NOTE: we prefer to uses libc headers instead of kernel headers if possible
> (e.g. not using <linux/resource.h>) due libc and kernel headers conflicts.
> https://sourceware.org/glibc/wiki/Synchronizing_Headers
>
> > +
> > +#ifndef HAVE_STRUCT_RLIMIT64
> > +struct rlimit64 {
> > + uint64_t rlim_cur;
> > + uint64_t rlim_max;
> > +};
> > +#endif
> > +
> > +static int setrlimit_u64(int resource, const struct rlimit64 *rlim)
> > +{
> > + return tst_syscall(__NR_prlimit64, 0, resource, rlim, NULL);
> I suppose using raw syscall is really needed, right?
>
> C library/kernel ABI differences
> Since glibc 2.13, the glibc getrlimit() and setrlimit() wrapper
> functions no longer invoke the corresponding system calls, but instead
> employ prlimit(), for the reasons described in BUGS.
>
> The name of the glibc wrapper function is prlimit(); the underlying
> system call is prlimit64().
>
> man getrlimit(2) says:
> https://man7.org/linux/man-pages/man2/getrlimit.2.html
>
> C library/kernel ABI differences
> Since glibc 2.13, the glibc getrlimit() and setrlimit() wrapper
> functions no longer invoke the corresponding system calls, but
> instead employ prlimit(), for the reasons described in BUGS.
>
> The name of the glibc wrapper function is prlimit(); the
> underlying system call is prlimit64().
>
> ...
> Since glibc 2.13, glibc works around the limitations of the
> getrlimit() and setrlimit() system calls by implementing
> setrlimit() and getrlimit() as wrapper functions that call
> prlimit().
> [chwen] See Wang Li' replied email
> > +}
> > +
> > +#endif /* LAPI_RESOURCE_H__ */
>
> I would probably separate adding the header into it's own commit, but it's
> ok to
> have everything in a single commit.
>
> > diff --git a/testcases/kernel/syscalls/setrlimit/setrlimit07.c
> b/testcases/kernel/syscalls/setrlimit/setrlimit07.c
> > new file mode 100644
> > index 000000000..60a4580da
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/setrlimit/setrlimit07.c
> > @@ -0,0 +1,127 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2025 Red Hat Inc. All Rights Reserved.
> > + * Author: Chunfu Wen <chwen@redhat.com>
> > + */
> > +
> > +/*
> > + * Set CPU time limit64 for a process and check its behavior
> > + * after reaching CPU time limit64.
> > + * 1) Process got SIGXCPU after reaching soft limit of CPU time limit64.
> > + * 2) Process got SIGKILL after reaching hard limit of CPU time limit64.
> > + */
>
> Please use docs like this:
>
> /*\
> * Set CPU time limit64 for a process and check its behavior
> * after reaching CPU time limit64.
> *
> * 1) Process got SIGXCPU after reaching soft limit of CPU time limit64.
> * 2) Process got SIGKILL after reaching hard limit of CPU time limit64.
> */
>
> /*\ is for docs to be parsed in metadata/ltp.json and included in
> https://linux-test-project.readthedocs.io/en/latest/users/test_catalog.html
>
> Extra space between list is needed to 1) and 2) be filtered as a list (not
> inline).
>
> Ideally you build the docs yourself
>
> https://linux-test-project.readthedocs.io/en/latest/developers/documentation.html#building-documentation
>
> and check the output in: doc/html/users/test_catalog.html#setrlimit07
> [chwen] this is not a issue since merging into setrlimit06
> > +
> > +#define _GNU_SOURCE
> > +
> > +#include <sys/wait.h>
> > +
> > +#include "tst_test.h"
> > +
> > +#include "lapi/syscalls.h"
> > +
> > +#include "lapi/resource.h"
>
> very nit: please remove blank lines between inludes
> [chwen] Done
> > +
> > +#ifndef HAVE_STRUCT_RLIMIT64
> > +struct rlimit64 {
> > + uint64_t rlim_cur;
> > + uint64_t rlim_max;
> > +};
> > +#endif
>
> This is defined in lapi/resource.h which you include, no need to repeat it
> here.
> [chwen] Done
> > +
> > +static struct rlimit64*rlim;
> > +
> > +static int *end;
> > +
> > +static void sighandler(int sig)
> > +{
> > + *end = sig;
> > +}
> > +
> > +static void setup(void)
> > +{
> > + signal(SIGXCPU, sighandler);
> > +
> > + end = mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE,
> > + MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>
> Please use SAFE_MMAP(), otherwise there would have to be check:
> [chwen] it is not a issue now since it is merged into setrlimit06.c
> if (end == MAP_FAILED)
> tst_brk(TBROK | TERRNO, "mmap failed on nfildes");
>
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > + if (end)
> > + munmap(end, sizeof(int));
> We have SAFE_MUNMAP
>
> > +}
> > +/*
> > +static int setrlimit_u64(int resource, const struct rlimit64 *rlim)
> > +{
> > + return tst_syscall(__NR_prlimit64, 0, resource, rlim, NULL);
> > +}*/
> Hm, commented out code, please remove it.
> [chwen] Done
> > +
> > +static void verify_setrlimit64(void)
> > +{
> > + int status;
> > + pid_t pid;
> > +
> > + rlim->rlim_cur = 1;
> > + rlim->rlim_max = 2;
> > +
> > + *end = 0;
> > +
> > + pid = fork();
> > + if (!pid) {
> > + TEST(setrlimit_u64(RLIMIT_CPU, rlim));
> > + if (TST_RET == -1) {
> > + tst_res(TFAIL | TTERRNO,
> > + "setrlimit_u64(RLIMIT_CPU) failed");
> We have TST_EXP_PASS() in tst_test_macros.h, please use it (or other if
> this is
> not appropriate.
>
> > + exit(1);
>
> > + }
> > +
> > + alarm(20);
> > +
> > + while (1)
> > + ;
> Why this?
> [chwen] aims to Suppress make check warnging
> > + }
> > +
> > + waitpid(pid, &status, 0);
> Maybe SAFE_WAITPID() ?
> > +
> > + if (WIFEXITED(status) && WEXITSTATUS(status) == 1)
> > + return;
> > +
> > + if (WIFSIGNALED(status)) {
> > + if (WTERMSIG(status) == SIGKILL && *end == SIGXCPU) {
> > + tst_res(TPASS,
> > + "Got SIGXCPU then SIGKILL after reaching
> both limit");
> > + return;
> > + }
> > +
> > + if (WTERMSIG(status) == SIGKILL && !*end) {
> > + tst_res(TFAIL,
> > + "Got only SIGKILL after reaching both
> limit");
> > + return;
> > + }
> > +
> > + if (WTERMSIG(status) == SIGALRM && *end == SIGXCPU) {
> > + tst_res(TFAIL,
> > + "Got only SIGXCPU after reaching both
> limit");
> > + return;
> > + }
> > +
> > + if (WTERMSIG(status) == SIGALRM && !*end) {
> > + tst_res(TFAIL,
> > + "Got no signal after reaching both limit");
> > + return;
> > + }
> > + }
> > +
> > + tst_res(TFAIL, "Child %s", tst_strstatus(status));
> > +}
> > +
> > +static struct tst_test test = {
> > + .test_all = verify_setrlimit64,
> > + .bufs = (struct tst_buffers []) {
> > + {&rlim, .size = sizeof(*rlim)},
> > + {},
> +1 for using guarded buffers.
> Please fix the indent.
>
> Last, but not least, code style:
> $ make check-setrlimit07
> CHECK testcases/kernel/syscalls/setrlimit/setrlimit07.c
> setrlimit07.c:31: ERROR: "foo*bar" should be "foo *bar"
> setrlimit07.c:54: WARNING: Block comments use * on subsequent lines
> setrlimit07.c:57: WARNING: Block comments use a trailing */ on a separate
> line
> setrlimit07.c:120: ERROR: code indent should use tabs where possible
> setrlimit07.c:120: WARNING: please, no spaces at the start of a line
> setrlimit07.c:121: ERROR: code indent should use tabs where possible
> setrlimit07.c:121: WARNING: please, no spaces at the start of a line
> setrlimit07.c:122: ERROR: code indent should use tabs where possible
> setrlimit07.c:122: WARNING: please, no spaces at the start of a line
>
> Kind regards,
> Petr
>
> > + },
> > + .setup = setup,
> > + .cleanup = cleanup,
> > + .forks_child = 1,
> > +};
> [chwen] all warning and error issue reported by make check-setrlimitxx
> were addressed
More information about the ltp
mailing list