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

[Petr Vorel <pvorel@suse.cz>]

Hi Wei, Cyril,

> Fixs: #530
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  include/tst_test.h | 14 ++++++++++++++
>  lib/tst_test.c     | 28 ++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)

> diff --git a/include/tst_test.h b/include/tst_test.h
> index 75c2109b9..dcdbc71d7 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -15,6 +15,7 @@
>  #include <limits.h>
>  #include <string.h>
>  #include <errno.h>
> +#include <sys/resource.h>

>  #include "tst_common.h"
>  #include "tst_res_flags.h"
> @@ -148,6 +149,11 @@ extern unsigned int tst_variant;

>  #define TST_UNLIMITED_RUNTIME (-1)

> +struct tst_ulimit_val {
> +	int resource;
> +	rlim_t rlim_cur;
> +};
> +
>  struct tst_test {
>  	/* number of tests available in test() function */
>  	unsigned int tcnt;
> @@ -306,6 +312,11 @@ struct tst_test {
>  	 */
>  	const struct tst_path_val *save_restore;

> +	/*
> +	 * {NULL, NULL} terminated array of (ulimit resource type and value)
> +	 */
> +	const struct tst_ulimit_val *ulimit;
> +
>  	/*
>  	 * NULL terminated array of kernel config options required for the
>  	 * test.
> @@ -392,6 +403,9 @@ int tst_validate_children_(const char *file, const int lineno,
>  #define tst_validate_children(child_count) \
>  	tst_validate_children_(__FILE__, __LINE__, (child_count))

> +#define tst_set_ulimit(conf) \
> +	tst_set_ulimit_(__FILE__, __LINE__, (conf))
> +
>  #ifndef TST_NO_DEFAULT_MAIN

>  static struct tst_test test;
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 2e58cad33..59eeda7e7 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1129,6 +1129,25 @@ static void do_cgroup_requires(void)
>  	tst_cg_init();
>  }

This patch requires manual adjustments (it does not apply to the master any more).

Also, this should be documented in doc/C-Test-API.asciidoc.

> +void tst_set_ulimit_(const char *file, const int lineno, const struct tst_ulimit_val *conf)
$ cd lib && make check-tst_test
...
tst_test.c:1150:13: warning: LTP-003: Symbol 'tst_set_ulimit_' has the LTP public library prefix, but is static (private).

If we want to use also in the tests (like tst_validate_children), the function
signature should be also in the header, e.g.:

include/tst_test.h
...
void tst_set_ulimit_(const char *file, const int lineno, const struct tst_ulimit_val *conf);
#define tst_set_ulimit(conf) \
	tst_set_ulimit_(__FILE__, __LINE__, (conf))

And, because offered to use in th tests, the function would deserve 1) comment
in include/tst_test.h.

But because tst_set_ulimit_() is just a wrapper over safe_setrlimit(), I don't
think it's needed to be called in the tests directly. Therefore the
#define tst_set_ulimit(conf) definition in the header is IMHO not needed
and tst_set_ulimit_() in lib/tst_test.c could be tst_set_ulimit() (without
underscore) and with static keyword.

> +{
> +	struct rlimit rlim;
> +
> +	SAFE_GETRLIMIT(conf->resource, &rlim);
When function has file and line, you want to use it for all library functions,
thus:
	safe_getrlimit(file, lineno, conf->resource, &rlim);

NOTE: some functions in tst_test.c calls SAFE_() macros, which is wrong. I'll
send patch after this is fixed.

> +
> +	if (conf->rlim_cur > rlim.rlim_max) {
> +		rlim.rlim_max = conf->rlim_cur;
> +		rlim.rlim_cur = conf->rlim_cur;
> +	} else {
> +		rlim.rlim_cur = conf->rlim_cur;
> +	}

rlim.rlim_cur is set twice, why not just:

	rlim.rlim_cur = conf->rlim_cur;

	if (conf->rlim_cur > rlim.rlim_max)
		rlim.rlim_max = conf->rlim_cur;

> +
> +	tst_res_(file, lineno, TINFO, "Set ulimit resource:%d rlim_cur:%lu rlim_max:%lu",
nit: space after ':' helps readability:
> +		conf->resource, rlim.rlim_cur, rlim.rlim_max);
> +
> +	safe_setrlimit(file, lineno, conf->resource, &rlim);
> +}
> +
>  static void do_setup(int argc, char *argv[])
>  {
>  	if (!tst_test)
> @@ -1227,6 +1246,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_set_ulimit(pvl);
> +			pvl++;
> +		}
> +	}
> +
>  	if (tst_test->mntpoint)
>  		SAFE_MKDIR(tst_test->mntpoint, 0777);

I suggest to merge with following changes.
@Cyril: feel free to improve the description.

diff --git doc/C-Test-API.asciidoc doc/C-Test-API.asciidoc
index db16be36e..c42a9754c 100644
--- doc/C-Test-API.asciidoc
+++ doc/C-Test-API.asciidoc
@@ -2426,6 +2426,24 @@ Test can be skipped on various conditions: on enabled SecureBoot
 ('.skip_in_secureboot = 1'), lockdown ('.skip_in_lockdown = 1') or in 32-bit
 compat mode ('.skip_in_compat = 1').
 
+1.43 Set resource limits
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+'.ulimit' allows to set resource limits on particular resource. NOTE: It sets 'rlim_cur'
+only if it's higher than 'rlim_cur'.
+
+[source,c]
+-------------------------------------------------------------------------------
+#include "tst_test.h"
+
+static struct tst_test test = {
+	...
+	.ulimit = (const struct tst_ulimit_val[]) {
+		{RLIMIT_STACK, RLIM_INFINITY},
+		{}
+	},
+};
+
 2. Common problems
 ------------------
 
diff --git include/tst_test.h include/tst_test.h
index bbcfb308d..42c348906 100644
--- include/tst_test.h
+++ include/tst_test.h
@@ -314,7 +314,7 @@ struct tst_test {
 	const struct tst_path_val *save_restore;
 
 	/*
-	 * {NULL, NULL} terminated array of (ulimit resource type and value)
+	 * {} terminated array of ulimit resource type and value.
 	 */
 	const struct tst_ulimit_val *ulimit;
 
@@ -404,9 +404,6 @@ int tst_validate_children_(const char *file, const int lineno,
 #define tst_validate_children(child_count) \
 	tst_validate_children_(__FILE__, __LINE__, (child_count))
 
-#define tst_set_ulimit(conf) \
-	tst_set_ulimit_(__FILE__, __LINE__, (conf))
-
 #ifndef TST_NO_DEFAULT_MAIN
 
 static struct tst_test test;
diff --git lib/tst_test.c lib/tst_test.c
index d2f967ade..6c81b9b02 100644
--- lib/tst_test.c
+++ lib/tst_test.c
@@ -1147,20 +1147,24 @@ static void do_cgroup_requires(void)
 	tst_cg_init();
 }
 
-void tst_set_ulimit_(const char *file, const int lineno, const struct tst_ulimit_val *conf)
+#define tst_set_ulimit(conf) \
+	tst_set_ulimit_(__FILE__, __LINE__, (conf))
+
+/*
+ * Set resource limits.
+ */
+static void tst_set_ulimit_(const char *file, const int lineno, const struct tst_ulimit_val *conf)
 {
 	struct rlimit rlim;
 
-	SAFE_GETRLIMIT(conf->resource, &rlim);
+	safe_getrlimit(file, lineno, conf->resource, &rlim);
 
-	if (conf->rlim_cur > rlim.rlim_max) {
+	rlim.rlim_cur = conf->rlim_cur;
+
+	if (conf->rlim_cur > rlim.rlim_max)
 		rlim.rlim_max = conf->rlim_cur;
-		rlim.rlim_cur = conf->rlim_cur;
-	} else {
-		rlim.rlim_cur = conf->rlim_cur;
-	}
 
-	tst_res_(file, lineno, TINFO, "Set ulimit resource:%d rlim_cur:%lu rlim_max:%lu",
+	tst_res_(file, lineno, TINFO, "Set ulimit resource: %d rlim_cur: %lu rlim_max: %lu",
 		conf->resource, rlim.rlim_cur, rlim.rlim_max);
 
 	safe_setrlimit(file, lineno, conf->resource, &rlim);

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