[v1,1/2] lib: Add .ulimit

Fix: #530
Signed-off-by: Wei Gao <wegao@suse.com>
---
 include/tst_test.h   |  6 ++++++
 include/tst_ulimit.h | 18 ++++++++++++++++++
 lib/tst_test.c       |  9 +++++++++
 lib/tst_ulimit.c     | 24 ++++++++++++++++++++++++
 4 files changed, 57 insertions(+)
 create mode 100644 include/tst_ulimit.h
 create mode 100644 lib/tst_ulimit.c

> 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

Hi!
> +	/*
> +	 * {NULL, NULL} terminated array of (ulimit resource type and value)
              ^
	      That's technically {0, NULL} or maybe we can just start
	      using the newer syntax with just {}
> +	 */
> +	const struct tst_ulimit_val *ulimit;
> +
>  	/*
>  	 * 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
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + *
> + * 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);

Maybe tst_ulimit_set()?

> +#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
> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +#include "tst_ulimit.h"
> +
> +struct tst_ulimit_conf {
> +	int resource;
> +	struct rlimit rlim;
> +};

This structure is defined in header already.

> +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;

I wonder if we should adjust the max only if it's smallre than the
requested value.

> +	tst_res(TINFO, "Set ulimit resource:%d rlim_cur:%ld rlim_max:%ld", conf->resource, rlim.rlim_cur, rlim.rlim_max);
> +	SAFE_SETRLIMIT(conf->resource, &rlim);
> +}
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

Hi!
> > +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;
> 
> I wonder if we should adjust the max only if it's smallre than the
> requested value.

Looking again we store both values in the structure. I wonder if we need
to set both, maybe we just need the rlim_cur value in the tst_ulimit_val
structure and we would attempt to adjust rlim_max only if its smaller
than rlim_cur. That would allow us to use the API even for tests that
does not require root to raise limits within the permitted maxium.