Re: [LTP] [PATCH v1 1/2] lib: Add .ulimit

Petr Vorel
2023-10-23T11:03:49Z

> Fix: #530
very nit: Although this works, we usually use:
Fixes: #530

...
> /*
> * NULL terminated array of kernel config options required for the
> * test.
> diff --git a/include/tst_ulimit.h b/include/tst_ulimit.h
> new file mode 100644
> index 000000000..b4f97670a
> --- /dev/null
> +++ b/include/tst_ulimit.h

I wonder if it's worth to add new header and C source just for single struct and
function. We might do so, but I consider that as a bad habit. I'd move that to
tst_test.{c,h}. But maybe others see it differently.

> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
Why v2 only? We use GLP v2+ for new code.
SPDX-License-Identifier: GPL-2.0-or-later

> + *
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +#ifndef TST_ULIMIT_H__
> +#define TST_ULIMIT_H__
> +
> +#include <sys/resource.h>
> +
> +struct tst_ulimit_val {
> + int resource;
> + struct rlimit rlim;
> +};
> +
> +void tst_ulimit_conf(const struct tst_ulimit_val *conf);
> +
> +#endif
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 2e58cad33..a8c7c7ba6 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1227,6 +1227,15 @@ static void do_setup(int argc, char *argv[])
> }
> }

> + if (tst_test->ulimit) {
> + const struct tst_ulimit_val *pvl = tst_test->ulimit;
> +
> + while (pvl->resource) {
> + tst_ulimit_conf(pvl);
> + pvl++;
> + }
> + }
> +
> if (tst_test->mntpoint)
> SAFE_MKDIR(tst_test->mntpoint, 0777);

> diff --git a/lib/tst_ulimit.c b/lib/tst_ulimit.c
> new file mode 100644
> index 000000000..1249d65d8
> --- /dev/null
> +++ b/lib/tst_ulimit.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Again, v2+ please.

> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +#include "tst_ulimit.h"
Again, please IMHO this should be in tst_test.c.

> +
> +struct tst_ulimit_conf {
> + int resource;
> + struct rlimit rlim;
> +};
> +
> +void tst_ulimit_conf(const struct tst_ulimit_val *conf)
> +{
> + struct rlimit rlim;
> +
> + rlim.rlim_cur = conf->rlim.rlim_cur;
> + rlim.rlim_max = conf->rlim.rlim_max;
> +
> + tst_res(TINFO, "Set ulimit resource:%d rlim_cur:%ld rlim_max:%ld", conf->resource, rlim.rlim_cur, rlim.rlim_max);
nit: Could we improve formatting (spaces, commas):
tst_res(TINFO, "Set ulimit resource: %d, rlim_cur: %ld, rlim_max: %ld",
conf->resource, rlim.rlim_cur, rlim.rlim_max);

First I thought all library code should use tst_res_(), so that we get file and
line of the original call, not the place in the library. But in code is used
only in the library thus tst_res() is ok.

> + SAFE_SETRLIMIT(conf->resource, &rlim);
Also here is probably ok that SAFE_SETRLIMIT() is correct (not
safe_setrlimit()).

Kind regards,
Petr