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

[Petr Vorel <pvorel@suse.cz>]

> 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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp