[LTP] [PATCH v3] save_restore: Introduce new struct field for flags

[LTP] [PATCH v3] save_restore: Introduce new struct field for flags

[Martin Doucha]

Tests using the .save_restore functionality currently cannot run
without root privileges at all because the test will write
into the path at least at the end and trigger error, even when
the config paths are flagged as optional.

Introduce new tst_path_val field for flags and replace path prefix flags
with bit flags. Also introduce new flags to control handling of read/write
errors and read-only sysfiles and rewrite save_restore implementation
accordingly.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
Acked-by: Jan Stancek <jstancek@redhat.com>
---

Changes since v1:
- TST_SR_IF_ACCESS => TST_SR_COND_ACCESS
- TST_SR_IGNORE_RO => TST_SR_SKIP_RO
- TST_SR_IGNORE_MISSING => TST_SR_SKIP_MISSING (to match the IGNORE_RO change)

Changes since v2:
- Fixed save_restore example in C Test API docs

I'll send a follow-up patchset to replace setup() code which requires root
privileges without good reason after this patch gets merged. Here I've kept
test changes to the minimum needed to maintain current save_restore behavior
with the new flags system. The only change in behavior is the use of read-only
handling flags where it's clear that the change is desired.

Though a few tests should get closer attention during review:
- all KSM tests
- add_key05
- migrate_pages02


 doc/c-test-api.txt                            | 38 ++++----
 include/tst_sys_conf.h                        | 15 ++-
 lib/tst_sys_conf.c                            | 97 ++++++++++++-------
 lib/tst_test.c                                |  3 +-
 testcases/cve/icmp_rate_limit01.c             |  3 +-
 testcases/kernel/containers/userns/userns08.c |  3 +-
 testcases/kernel/kvm/kvm_pagefault01.c        |  3 +-
 testcases/kernel/mem/ksm/ksm01.c              | 10 +-
 testcases/kernel/mem/ksm/ksm02.c              | 10 +-
 testcases/kernel/mem/ksm/ksm03.c              | 10 +-
 testcases/kernel/mem/ksm/ksm04.c              | 10 +-
 testcases/kernel/mem/ksm/ksm05.c              |  2 +-
 testcases/kernel/mem/ksm/ksm06.c              |  9 +-
 testcases/kernel/syscalls/add_key/add_key05.c |  7 +-
 testcases/kernel/syscalls/bind/bind06.c       |  3 +-
 testcases/kernel/syscalls/fork/fork13.c       |  2 +-
 .../kernel/syscalls/ipc/msgget/msgget03.c     |  2 +-
 testcases/kernel/syscalls/madvise/madvise06.c |  2 +-
 testcases/kernel/syscalls/madvise/madvise08.c |  2 +-
 .../syscalls/migrate_pages/migrate_pages02.c  |  2 +-
 testcases/kernel/syscalls/sendto/sendto03.c   |  3 +-
 .../kernel/syscalls/setsockopt/setsockopt05.c |  3 +-
 .../kernel/syscalls/setsockopt/setsockopt06.c |  3 +-
 .../kernel/syscalls/setsockopt/setsockopt07.c |  3 +-
 .../kernel/syscalls/setsockopt/setsockopt08.c |  3 +-
 .../kernel/syscalls/setsockopt/setsockopt09.c |  3 +-
 testcases/kernel/syscalls/syslog/syslog11.c   |  2 +-
 27 files changed, 151 insertions(+), 102 deletions(-)

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 8844d9f2f..fa7e2d3b0 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -1605,35 +1605,33 @@ LTP library can be instructed to save and restore value of specified
 field 'save_restore'. It is a NULL-terminated array of struct
 'tst_path_val' where each tst_path_val.path represents a file, whose
 value is saved at the beginning and restored at the end of the test.
-If non-NULL value is passed it is written to the respective file at
-the beginning of the test. Only the first line of a specified file
-is saved and restored.
-
-Pathnames can be optionally prefixed to specify how strictly (during
-'store') are handled errors:
-
-* (no prefix) - test ends with 'TCONF', if file doesn't exist
-* '?'         - test prints info message and continues,
-                if file doesn't exist or open/read fails
-* '!'         - test ends with 'TBROK', if file doesn't exist
+If non-NULL string is passed in tst_path_val.val, it is written
+to the respective file at the beginning of the test. Only the first line
+of a specified file is saved and restored.
+
+By default, the test will end with TCONF if the file is read-only or
+does not exist. If the optional write of new value fails, the test will end
+with 'TBROK'. This behavior can be changed using tst_path_val.flags:
+
+* 'TST_SR_FAIL_MISSING' – End test with 'TBROK' if the file does not exist
+* 'TST_SR_SKIP_MISSING' – Continue without saving the file if it does not exist
+* 'TST_SR_FAIL_RO' – End test with 'TBROK' if the file is read-only
+* 'TST_SR_SKIP_RO' – Continue without saving the file if it is read-only
+* 'TST_SR_IGNORE_ERR' – Ignore errors when writing new value into the file
+* 'TST_SR_REQUIRED' – Equivalent to 'TST_SR_FAIL_MISSING | TST_SR_FAIL_RO'
+* 'TST_SR_COND_ACCESS' – Equivalent to 'TST_SR_SKIP_MISSING | TST_SR_SKIP_RO'
 
 'restore' is always strict and will TWARN if it encounters any error.
 
 [source,c]
 -------------------------------------------------------------------------------
-static void setup(void)
-{
-	FILE_PRINTF("/proc/sys/kernel/core_pattern", "/mypath");
-	SAFE_TRY_FILE_PRINTF("/proc/sys/user/max_user_namespaces", "%d", 10);
-}
-
 static struct tst_test test = {
 	...
 	.setup = setup,
 	.save_restore = (const struct tst_path_val[]) {
-		{"/proc/sys/kernel/core_pattern", NULL},
-		{"?/proc/sys/user/max_user_namespaces", NULL},
-		{"!/sys/kernel/mm/ksm/run", "1"},
+		{"/proc/sys/kernel/core_pattern", NULL, 0},
+		{"/proc/sys/user/max_user_namespaces", NULL, TST_SR_COND_ACCESS},
+		{"/sys/kernel/mm/ksm/run", "1", TST_SR_REQUIRED},
 		{}
 	},
 };
diff --git a/include/tst_sys_conf.h b/include/tst_sys_conf.h
index b7bbe36fc..b52b8913e 100644
--- a/include/tst_sys_conf.h
+++ b/include/tst_sys_conf.h
@@ -5,14 +5,23 @@
 #ifndef TST_SYS_CONF_H__
 #define TST_SYS_CONF_H__
 
+#define TST_SR_FAIL_MISSING 0x1
+#define TST_SR_SKIP_MISSING 0x2
+#define TST_SR_FAIL_RO 0x4
+#define TST_SR_SKIP_RO 0x8
+#define TST_SR_IGNORE_ERR 0x10
+
+#define TST_SR_REQUIRED (TST_SR_FAIL_MISSING | TST_SR_FAIL_RO)
+#define TST_SR_COND_ACCESS (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
+
 struct tst_path_val {
         const char *path;
         const char *val;
+	unsigned int flags;
 };
 
-int tst_sys_conf_save_str(const char *path, const char *value);
-int tst_sys_conf_save(const char *path);
-void tst_sys_conf_set(const char *path, const char *value);
+void tst_sys_conf_save_str(const char *path, const char *value);
+int tst_sys_conf_save(const struct tst_path_val *conf);
 void tst_sys_conf_restore(int verbose);
 void tst_sys_conf_dump(void);
 
diff --git a/lib/tst_sys_conf.c b/lib/tst_sys_conf.c
index 003698825..7aa0f46bd 100644
--- a/lib/tst_sys_conf.c
+++ b/lib/tst_sys_conf.c
@@ -20,6 +20,14 @@ struct tst_sys_conf {
 
 static struct tst_sys_conf *save_restore_data;
 
+static void print_error(int info_only, const char *err, const char *path)
+{
+	if (info_only)
+		tst_res(TINFO, err, path);
+	else
+		tst_brk(TBROK | TERRNO, err, path);
+}
+
 void tst_sys_conf_dump(void)
 {
 	struct tst_sys_conf *i;
@@ -28,7 +36,7 @@ void tst_sys_conf_dump(void)
 		tst_res(TINFO, "%s = %s", i->path, i->value);
 }
 
-int tst_sys_conf_save_str(const char *path, const char *value)
+void tst_sys_conf_save_str(const char *path, const char *value)
 {
 	struct tst_sys_conf *n = SAFE_MALLOC(sizeof(*n));
 
@@ -40,45 +48,43 @@ int tst_sys_conf_save_str(const char *path, const char *value)
 
 	n->next = save_restore_data;
 	save_restore_data = n;
-
-	return 0;
 }
 
-int tst_sys_conf_save(const char *path)
+int tst_sys_conf_save(const struct tst_path_val *conf)
 {
 	char line[PATH_MAX];
+	int ttype, iret;
 	FILE *fp;
 	void *ret;
-	char flag;
 
-	if (!path)
+	if (!conf || !conf->path)
 		tst_brk(TBROK, "path is empty");
 
-	flag = path[0];
-	if (flag  == '?' || flag == '!')
-		path++;
-
-	if (access(path, F_OK) != 0) {
-		switch (flag) {
-		case '?':
-			tst_res(TINFO, "Path not found: '%s'", path);
-			break;
-		case '!':
-			tst_brk(TBROK|TERRNO, "Path not found: '%s'", path);
-			break;
-		default:
-			tst_brk(TCONF|TERRNO, "Path not found: '%s'", path);
+	if (access(conf->path, F_OK) != 0) {
+		if (conf->flags & TST_SR_SKIP_MISSING) {
+			tst_res(TINFO, "Path not found: %s", conf->path);
+			return 1;
 		}
-		return 1;
+
+		ttype = (conf->flags & TST_SR_FAIL_MISSING) ? TBROK : TCONF;
+		tst_brk(ttype | TERRNO, "Path not found: %s", conf->path);
 	}
 
-	fp = fopen(path, "r");
-	if (fp == NULL) {
-		if (flag == '?')
+	if (access(conf->path, W_OK) != 0) {
+		if (conf->flags & TST_SR_SKIP_RO) {
+			tst_res(TINFO, "Path is not writable: %s", conf->path);
 			return 1;
+		}
 
-		tst_brk(TBROK | TERRNO, "Failed to open FILE '%s' for reading",
-			path);
+		ttype = (conf->flags & TST_SR_FAIL_RO) ? TBROK : TCONF;
+		tst_brk(ttype | TERRNO, "Path is not writable: %s", conf->path);
+	}
+
+	fp = fopen(conf->path, "r");
+
+	if (fp == NULL) {
+		print_error(conf->flags & TST_SR_IGNORE_ERR,
+			"Failed to open '%s' for reading", conf->path);
 		return 1;
 	}
 
@@ -86,24 +92,41 @@ int tst_sys_conf_save(const char *path)
 	fclose(fp);
 
 	if (ret == NULL) {
-		if (flag == '?')
+		if (conf->flags & TST_SR_IGNORE_ERR)
 			return 1;
 
 		tst_brk(TBROK | TERRNO, "Failed to read anything from '%s'",
-			path);
+			conf->path);
 	}
 
-	return tst_sys_conf_save_str(path, line);
-}
+	tst_sys_conf_save_str(conf->path, line);
 
-void tst_sys_conf_set(const char *path, const char *value)
-{
-	char flag = path[0];
-	if (flag  == '?' || flag == '!')
-		path++;
+	if (!conf->val)
+		return 0;
+
+	fp = fopen(conf->path, "w");
+
+	if (fp == NULL) {
+		print_error(conf->flags & TST_SR_IGNORE_ERR,
+			"Failed to open '%s' for writing", conf->path);
+		return 0;
+	}
+
+	iret = fputs(conf->val, fp);
 
-	if (value)
-		SAFE_FILE_PRINTF(path, "%s", value);
+	if (iret < 0) {
+		print_error(conf->flags & TST_SR_IGNORE_ERR,
+			"Failed to write into '%s'", conf->path);
+	}
+
+	iret = fclose(fp);
+
+	if (iret < 0) {
+		print_error(conf->flags & TST_SR_IGNORE_ERR,
+			"Failed to close '%s'", conf->path);
+	}
+
+	return 0;
 }
 
 void tst_sys_conf_restore(int verbose)
diff --git a/lib/tst_test.c b/lib/tst_test.c
index b225ba082..d3e1c514d 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1203,8 +1203,7 @@ static void do_setup(int argc, char *argv[])
 		const struct tst_path_val *pvl = tst_test->save_restore;
 
 		while (pvl->path) {
-			if (!tst_sys_conf_save(pvl->path))
-				tst_sys_conf_set(pvl->path, pvl->val);
+			tst_sys_conf_save(pvl);
 			pvl++;
 		}
 	}
diff --git a/testcases/cve/icmp_rate_limit01.c b/testcases/cve/icmp_rate_limit01.c
index 1263762d2..061239474 100644
--- a/testcases/cve/icmp_rate_limit01.c
+++ b/testcases/cve/icmp_rate_limit01.c
@@ -269,7 +269,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/containers/userns/userns08.c b/testcases/kernel/containers/userns/userns08.c
index 0f1cb54a9..97545d275 100644
--- a/testcases/kernel/containers/userns/userns08.c
+++ b/testcases/kernel/containers/userns/userns08.c
@@ -135,7 +135,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/kvm/kvm_pagefault01.c b/testcases/kernel/kvm/kvm_pagefault01.c
index e355fa448..27145129b 100644
--- a/testcases/kernel/kvm/kvm_pagefault01.c
+++ b/testcases/kernel/kvm/kvm_pagefault01.c
@@ -217,7 +217,8 @@ static struct tst_test test = {
 	.cleanup = tst_kvm_cleanup,
 	.needs_root = 1,
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/sys/module/kvm/parameters/tdp_mmu", "0"},
+		{"/sys/module/kvm/parameters/tdp_mmu", "0",
+			TST_SR_SKIP_MISSING},
 		{}
 	},
 	.supported_archs = (const char *const []) {
diff --git a/testcases/kernel/mem/ksm/ksm01.c b/testcases/kernel/mem/ksm/ksm01.c
index 1f3852fc3..d4e7aaf67 100644
--- a/testcases/kernel/mem/ksm/ksm01.c
+++ b/testcases/kernel/mem/ksm/ksm01.c
@@ -80,10 +80,12 @@ static struct tst_test test = {
 	},
 	.setup = setup,
 	.save_restore = (const struct tst_path_val[]) {
-		{"!/sys/kernel/mm/ksm/run", NULL},
-		{"!/sys/kernel/mm/ksm/sleep_millisecs", NULL},
-		{"?/sys/kernel/mm/ksm/max_page_sharing", NULL},
-		{"?/sys/kernel/mm/ksm/merge_across_nodes", "1"},
+		{"/sys/kernel/mm/ksm/run", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+			TST_SR_SKIP_MISSING},
+		{"/sys/kernel/mm/ksm/merge_across_nodes", "1",
+			TST_SR_SKIP_MISSING},
 		{}
 	},
 	.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm02.c b/testcases/kernel/mem/ksm/ksm02.c
index 4183108d5..2eba753cd 100644
--- a/testcases/kernel/mem/ksm/ksm02.c
+++ b/testcases/kernel/mem/ksm/ksm02.c
@@ -101,10 +101,12 @@ static struct tst_test test = {
 	},
 	.setup = setup,
 	.save_restore = (const struct tst_path_val[]) {
-		{"!/sys/kernel/mm/ksm/run", NULL},
-		{"!/sys/kernel/mm/ksm/sleep_millisecs", NULL},
-		{"?/sys/kernel/mm/ksm/max_page_sharing", NULL},
-		{"?/sys/kernel/mm/ksm/merge_across_nodes", "1"},
+		{"/sys/kernel/mm/ksm/run", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+			TST_SR_SKIP_MISSING},
+		{"/sys/kernel/mm/ksm/merge_across_nodes", "1",
+			TST_SR_SKIP_MISSING},
 		{}
 	},
 	.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm03.c b/testcases/kernel/mem/ksm/ksm03.c
index 1cf2e4954..ce297ea46 100644
--- a/testcases/kernel/mem/ksm/ksm03.c
+++ b/testcases/kernel/mem/ksm/ksm03.c
@@ -83,10 +83,12 @@ static struct tst_test test = {
 	},
 	.setup = setup,
 	.save_restore = (const struct tst_path_val[]) {
-		{"!/sys/kernel/mm/ksm/run", NULL},
-		{"!/sys/kernel/mm/ksm/sleep_millisecs", NULL},
-		{"?/sys/kernel/mm/ksm/max_page_sharing", NULL},
-		{"?/sys/kernel/mm/ksm/merge_across_nodes", "1"},
+		{"/sys/kernel/mm/ksm/run", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+			TST_SR_SKIP_MISSING},
+		{"/sys/kernel/mm/ksm/merge_across_nodes", "1",
+			TST_SR_SKIP_MISSING},
 		{}
 	},
 	.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm04.c b/testcases/kernel/mem/ksm/ksm04.c
index 9fe9d6dd5..b9af0d55d 100644
--- a/testcases/kernel/mem/ksm/ksm04.c
+++ b/testcases/kernel/mem/ksm/ksm04.c
@@ -103,10 +103,12 @@ static struct tst_test test = {
 	},
 	.setup = setup,
 	.save_restore = (const struct tst_path_val[]) {
-		{"!/sys/kernel/mm/ksm/run", NULL},
-		{"!/sys/kernel/mm/ksm/sleep_millisecs", NULL},
-		{"?/sys/kernel/mm/ksm/max_page_sharing", NULL},
-		{"?/sys/kernel/mm/ksm/merge_across_nodes", "1"},
+		{"/sys/kernel/mm/ksm/run", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+			TST_SR_SKIP_MISSING},
+		{"/sys/kernel/mm/ksm/merge_across_nodes", "1",
+			TST_SR_SKIP_MISSING},
 		{}
 	},
 	.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm05.c b/testcases/kernel/mem/ksm/ksm05.c
index 146a9a3b7..8ea9bd187 100644
--- a/testcases/kernel/mem/ksm/ksm05.c
+++ b/testcases/kernel/mem/ksm/ksm05.c
@@ -89,7 +89,7 @@ static struct tst_test test = {
 	.test_all = test_ksm,
 	.min_kver = "2.6.32",
 	.save_restore = (const struct tst_path_val[]) {
-		{"!/sys/kernel/mm/ksm/run", "1"},
+		{"/sys/kernel/mm/ksm/run", "1", TST_SR_REQUIRED},
 		{}
 	},
 	.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm06.c b/testcases/kernel/mem/ksm/ksm06.c
index 21c82edc1..63c1dba37 100644
--- a/testcases/kernel/mem/ksm/ksm06.c
+++ b/testcases/kernel/mem/ksm/ksm06.c
@@ -137,10 +137,11 @@ static struct tst_test test = {
 	},
 	.setup = setup,
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/sys/kernel/mm/ksm/max_page_sharing", NULL},
-		{"!/sys/kernel/mm/ksm/run", NULL},
-		{"!/sys/kernel/mm/ksm/sleep_millisecs", NULL},
-		{"/sys/kernel/mm/ksm/merge_across_nodes", NULL},
+		{"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+			TST_SR_SKIP_MISSING},
+		{"/sys/kernel/mm/ksm/run", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_REQUIRED},
+		{"/sys/kernel/mm/ksm/merge_across_nodes", NULL, 0},
 		{}
 	},
 	.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
index 63cb262bb..8cba5f492 100644
--- a/testcases/kernel/syscalls/add_key/add_key05.c
+++ b/testcases/kernel/syscalls/add_key/add_key05.c
@@ -220,9 +220,10 @@ static struct tst_test test = {
 	.forks_child = 1,
 	.cleanup = cleanup,
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/kernel/keys/gc_delay", "1"},
-		{"?/proc/sys/kernel/keys/maxkeys", "200"},
-		{"?/proc/sys/kernel/keys/maxbytes", "20000"},
+		{"/proc/sys/kernel/keys/gc_delay", "1", TST_SR_SKIP_MISSING},
+		{"/proc/sys/kernel/keys/maxkeys", "200", TST_SR_SKIP_MISSING},
+		{"/proc/sys/kernel/keys/maxbytes", "20000",
+			TST_SR_SKIP_MISSING},
 		{}
 	},
 	.bufs = (struct tst_buffers []) {
diff --git a/testcases/kernel/syscalls/bind/bind06.c b/testcases/kernel/syscalls/bind/bind06.c
index 618cfce46..34284c753 100644
--- a/testcases/kernel/syscalls/bind/bind06.c
+++ b/testcases/kernel/syscalls/bind/bind06.c
@@ -110,7 +110,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/fork/fork13.c b/testcases/kernel/syscalls/fork/fork13.c
index fe30d1e9c..baa1825ae 100644
--- a/testcases/kernel/syscalls/fork/fork13.c
+++ b/testcases/kernel/syscalls/fork/fork13.c
@@ -113,7 +113,7 @@ static struct tst_test test = {
 	.max_runtime = 600,
 	.test_all = check,
 	.save_restore = (const struct tst_path_val[]) {
-		{"!/proc/sys/kernel/pid_max", PID_MAX_STR},
+		{"/proc/sys/kernel/pid_max", PID_MAX_STR, TST_SR_REQUIRED},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/ipc/msgget/msgget03.c b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
index 2257ae0f9..272e58e3a 100644
--- a/testcases/kernel/syscalls/ipc/msgget/msgget03.c
+++ b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
@@ -76,7 +76,7 @@ static struct tst_test test = {
 	.cleanup = cleanup,
 	.test_all = verify_msgget,
 	.save_restore = (const struct tst_path_val[]){
-		{"/proc/sys/kernel/msgmni", NULL},
+		{"/proc/sys/kernel/msgmni", NULL, 0},
 		{}
 	}
 };
diff --git a/testcases/kernel/syscalls/madvise/madvise06.c b/testcases/kernel/syscalls/madvise/madvise06.c
index 27aff18f1..259ebff1e 100644
--- a/testcases/kernel/syscalls/madvise/madvise06.c
+++ b/testcases/kernel/syscalls/madvise/madvise06.c
@@ -239,7 +239,7 @@ static struct tst_test test = {
 	.needs_tmpdir = 1,
 	.needs_root = 1,
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/vm/swappiness", NULL},
+		{"/proc/sys/vm/swappiness", NULL, TST_SR_SKIP_MISSING},
 		{}
 	},
 	.needs_cgroup_ctrls = (const char *const []){ "memory", NULL },
diff --git a/testcases/kernel/syscalls/madvise/madvise08.c b/testcases/kernel/syscalls/madvise/madvise08.c
index 10549f4b4..75d76f725 100644
--- a/testcases/kernel/syscalls/madvise/madvise08.c
+++ b/testcases/kernel/syscalls/madvise/madvise08.c
@@ -213,7 +213,7 @@ static struct tst_test test = {
 	.needs_root = 1,
 	.forks_child = 1,
 	.save_restore = (const struct tst_path_val[]) {
-		{CORE_PATTERN, NULL},
+		{CORE_PATTERN, NULL, 0},
 		{}
 	},
 };
diff --git a/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c b/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
index 32da57042..c706dde69 100644
--- a/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
+++ b/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
@@ -327,7 +327,7 @@ static struct tst_test test = {
 	.test_all = run,
 	.setup = setup,
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/kernel/numa_balancing", "0"},
+		{"/proc/sys/kernel/numa_balancing", "0", TST_SR_SKIP_MISSING},
 		{}
 	},
 };
diff --git a/testcases/kernel/syscalls/sendto/sendto03.c b/testcases/kernel/syscalls/sendto/sendto03.c
index 5d2c1e112..8f383786a 100644
--- a/testcases/kernel/syscalls/sendto/sendto03.c
+++ b/testcases/kernel/syscalls/sendto/sendto03.c
@@ -218,7 +218,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
index 651583fb6..823b20481 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
@@ -102,7 +102,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt06.c b/testcases/kernel/syscalls/setsockopt/setsockopt06.c
index 9c818646b..6f245fb2c 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt06.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt06.c
@@ -130,7 +130,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt07.c b/testcases/kernel/syscalls/setsockopt/setsockopt07.c
index 616159a90..bf15c151d 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt07.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt07.c
@@ -143,7 +143,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
index 563444635..e9130934b 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt08.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
@@ -159,7 +159,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt09.c b/testcases/kernel/syscalls/setsockopt/setsockopt09.c
index 98f7fd00e..05cbbf005 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt09.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt09.c
@@ -124,7 +124,8 @@ static struct tst_test test = {
 		NULL
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"?/proc/sys/user/max_user_namespaces", NULL},
+		{"/proc/sys/user/max_user_namespaces", NULL,
+			TST_SR_COND_ACCESS},
 		{}
 	},
 	.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/syslog/syslog11.c b/testcases/kernel/syscalls/syslog/syslog11.c
index ca1ecbbe3..01a5eefa9 100644
--- a/testcases/kernel/syscalls/syslog/syslog11.c
+++ b/testcases/kernel/syscalls/syslog/syslog11.c
@@ -58,7 +58,7 @@ static void run(unsigned int n)
 static struct tst_test test = {
 	.test = run,
 	.save_restore = (const struct tst_path_val[]) {
-		{"!/proc/sys/kernel/printk", NULL},
+		{"/proc/sys/kernel/printk", NULL, TST_SR_REQUIRED},
 		{}
 	},
 	.needs_root = 1,
-- 
2.37.3


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

Re: [LTP] [PATCH v3] save_restore: Introduce new struct field for flags

[Cyril Hrubis]

Hi!
> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 8844d9f2f..fa7e2d3b0 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -1605,35 +1605,33 @@ LTP library can be instructed to save and restore value of specified
>  field 'save_restore'. It is a NULL-terminated array of struct
>  'tst_path_val' where each tst_path_val.path represents a file, whose
>  value is saved at the beginning and restored at the end of the test.
> -If non-NULL value is passed it is written to the respective file at
> -the beginning of the test. Only the first line of a specified file
> -is saved and restored.
> -
> -Pathnames can be optionally prefixed to specify how strictly (during
> -'store') are handled errors:
> -
> -* (no prefix) - test ends with 'TCONF', if file doesn't exist
> -* '?'         - test prints info message and continues,
> -                if file doesn't exist or open/read fails
> -* '!'         - test ends with 'TBROK', if file doesn't exist
> +If non-NULL string is passed in tst_path_val.val, it is written
> +to the respective file at the beginning of the test. Only the first line
> +of a specified file is saved and restored.
> +
> +By default, the test will end with TCONF if the file is read-only or
> +does not exist. If the optional write of new value fails, the test will end
> +with 'TBROK'. This behavior can be changed using tst_path_val.flags:

I'm not sure if it's a good idea to have default without any constants
passed. When I look at the file_restore records in the tests it all
makes sense until I reach a part where 0 is passed to the flags. I think
that it would make sense to make everything as self describing as
possible.

Would you consider adding TST_SR_TCONF_MISSING and TST_SR_TCONF_RO?

> +* 'TST_SR_FAIL_MISSING' – End test with 'TBROK' if the file does not exist

This FAIL part in this name is quite misleading since the test ends with
TBROK. I would say that it would be much more clear if it was named
TST_SR_TBROK_MISSING.

> +* 'TST_SR_SKIP_MISSING' – Continue without saving the file if it does not exist
> +* 'TST_SR_FAIL_RO' – End test with 'TBROK' if the file is read-only
> +* 'TST_SR_SKIP_RO' – Continue without saving the file if it is read-only
> +* 'TST_SR_IGNORE_ERR' – Ignore errors when writing new value into the file
> +* 'TST_SR_REQUIRED' – Equivalent to 'TST_SR_FAIL_MISSING | TST_SR_FAIL_RO'
> +* 'TST_SR_COND_ACCESS' – Equivalent to 'TST_SR_SKIP_MISSING | TST_SR_SKIP_RO'
>  
>  'restore' is always strict and will TWARN if it encounters any error.
>  
>  [source,c]
>  -------------------------------------------------------------------------------
> -static void setup(void)
> -{
> -	FILE_PRINTF("/proc/sys/kernel/core_pattern", "/mypath");
> -	SAFE_TRY_FILE_PRINTF("/proc/sys/user/max_user_namespaces", "%d", 10);
> -}
> -
>  static struct tst_test test = {
>  	...
>  	.setup = setup,
>  	.save_restore = (const struct tst_path_val[]) {
> -		{"/proc/sys/kernel/core_pattern", NULL},
> -		{"?/proc/sys/user/max_user_namespaces", NULL},
> -		{"!/sys/kernel/mm/ksm/run", "1"},
> +		{"/proc/sys/kernel/core_pattern", NULL, 0},
> +		{"/proc/sys/user/max_user_namespaces", NULL, TST_SR_COND_ACCESS},
> +		{"/sys/kernel/mm/ksm/run", "1", TST_SR_REQUIRED},
>  		{}
>  	},
>  };
> diff --git a/include/tst_sys_conf.h b/include/tst_sys_conf.h
> index b7bbe36fc..b52b8913e 100644
> --- a/include/tst_sys_conf.h
> +++ b/include/tst_sys_conf.h
> @@ -5,14 +5,23 @@
>  #ifndef TST_SYS_CONF_H__
>  #define TST_SYS_CONF_H__
>  
> +#define TST_SR_FAIL_MISSING 0x1
> +#define TST_SR_SKIP_MISSING 0x2
> +#define TST_SR_FAIL_RO 0x4
> +#define TST_SR_SKIP_RO 0x8
> +#define TST_SR_IGNORE_ERR 0x10
> +
> +#define TST_SR_REQUIRED (TST_SR_FAIL_MISSING | TST_SR_FAIL_RO)
> +#define TST_SR_COND_ACCESS (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
> +
>  struct tst_path_val {
>          const char *path;
>          const char *val;
> +	unsigned int flags;
>  };
>  
> -int tst_sys_conf_save_str(const char *path, const char *value);
> -int tst_sys_conf_save(const char *path);
> -void tst_sys_conf_set(const char *path, const char *value);
> +void tst_sys_conf_save_str(const char *path, const char *value);
> +int tst_sys_conf_save(const struct tst_path_val *conf);
>  void tst_sys_conf_restore(int verbose);
>  void tst_sys_conf_dump(void);
>  
> diff --git a/lib/tst_sys_conf.c b/lib/tst_sys_conf.c
> index 003698825..7aa0f46bd 100644
> --- a/lib/tst_sys_conf.c
> +++ b/lib/tst_sys_conf.c
> @@ -20,6 +20,14 @@ struct tst_sys_conf {
>  
>  static struct tst_sys_conf *save_restore_data;
>  
> +static void print_error(int info_only, const char *err, const char *path)
> +{
> +	if (info_only)
> +		tst_res(TINFO, err, path);
> +	else
> +		tst_brk(TBROK | TERRNO, err, path);
> +}

Can we please either make this a macro or pass a __LINE__ so that the
line number in the error message is correct?

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v3] save_restore: Introduce new struct field for flags

[Martin Doucha]

On 14. 11. 22 14:25, Cyril Hrubis wrote:
> I'm not sure if it's a good idea to have default without any constants
> passed. When I look at the file_restore records in the tests it all
> makes sense until I reach a part where 0 is passed to the flags. I think
> that it would make sense to make everything as self describing as
> possible.
> 
> Would you consider adding TST_SR_TCONF_MISSING and TST_SR_TCONF_RO?

I could set both to 0, if that's OK.

>> +* 'TST_SR_FAIL_MISSING' – End test with 'TBROK' if the file does not exist
> 
> This FAIL part in this name is quite misleading since the test ends with
> TBROK. I would say that it would be much more clear if it was named
> TST_SR_TBROK_MISSING.

Then I should also rename TST_SR_REQUIRED to TST_SR_TBROK, rename 
TST_SR_COND_ACCESS to TST_SR_SKIP and add TST_SR_TCONF for the two new 
flags.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v3] save_restore: Introduce new struct field for flags

[Cyril Hrubis]

Hi!
> > Would you consider adding TST_SR_TCONF_MISSING and TST_SR_TCONF_RO?
> 
> I could set both to 0, if that's OK.

I suppose that would work fine.

> >> +* 'TST_SR_FAIL_MISSING' – End test with 'TBROK' if the file does not exist
> > 
> > This FAIL part in this name is quite misleading since the test ends with
> > TBROK. I would say that it would be much more clear if it was named
> > TST_SR_TBROK_MISSING.
> 
> Then I should also rename TST_SR_REQUIRED to TST_SR_TBROK, rename 
> TST_SR_COND_ACCESS to TST_SR_SKIP and add TST_SR_TCONF for the two new 
> flags.

That sounds good to me, it's much closely mapped to the actual test
result types and should be clear enough.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v3] save_restore: Introduce new struct field for flags

[Richard Palethorpe]

Hello,

Martin Doucha <mdoucha@suse.cz> writes:

> Tests using the .save_restore functionality currently cannot run
> without root privileges at all because the test will write
> into the path at least at the end and trigger error, even when
> the config paths are flagged as optional.
>
> Introduce new tst_path_val field for flags and replace path prefix flags
> with bit flags. Also introduce new flags to control handling of read/write
> errors and read-only sysfiles and rewrite save_restore implementation
> accordingly.
>
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> Acked-by: Jan Stancek <jstancek@redhat.com>
> ---
>
> Changes since v1:
> - TST_SR_IF_ACCESS => TST_SR_COND_ACCESS
> - TST_SR_IGNORE_RO => TST_SR_SKIP_RO
> - TST_SR_IGNORE_MISSING => TST_SR_SKIP_MISSING (to match the IGNORE_RO change)
>
> Changes since v2:
> - Fixed save_restore example in C Test API docs
>
> I'll send a follow-up patchset to replace setup() code which requires root
> privileges without good reason after this patch gets merged. Here I've kept
> test changes to the minimum needed to maintain current save_restore behavior
> with the new flags system. The only change in behavior is the use of read-only
> handling flags where it's clear that the change is desired.
>
> Though a few tests should get closer attention during review:
> - all KSM tests
> - add_key05
> - migrate_pages02
>
>
>  doc/c-test-api.txt                            | 38 ++++----
>  include/tst_sys_conf.h                        | 15 ++-
>  lib/tst_sys_conf.c                            | 97 ++++++++++++-------
>  lib/tst_test.c                                |  3 +-
>  testcases/cve/icmp_rate_limit01.c             |  3 +-
>  testcases/kernel/containers/userns/userns08.c |  3 +-
>  testcases/kernel/kvm/kvm_pagefault01.c        |  3 +-
>  testcases/kernel/mem/ksm/ksm01.c              | 10 +-
>  testcases/kernel/mem/ksm/ksm02.c              | 10 +-
>  testcases/kernel/mem/ksm/ksm03.c              | 10 +-
>  testcases/kernel/mem/ksm/ksm04.c              | 10 +-
>  testcases/kernel/mem/ksm/ksm05.c              |  2 +-
>  testcases/kernel/mem/ksm/ksm06.c              |  9 +-
>  testcases/kernel/syscalls/add_key/add_key05.c |  7 +-
>  testcases/kernel/syscalls/bind/bind06.c       |  3 +-
>  testcases/kernel/syscalls/fork/fork13.c       |  2 +-
>  .../kernel/syscalls/ipc/msgget/msgget03.c     |  2 +-
>  testcases/kernel/syscalls/madvise/madvise06.c |  2 +-
>  testcases/kernel/syscalls/madvise/madvise08.c |  2 +-
>  .../syscalls/migrate_pages/migrate_pages02.c  |  2 +-
>  testcases/kernel/syscalls/sendto/sendto03.c   |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt05.c |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt06.c |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt07.c |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt08.c |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt09.c |  3 +-
>  testcases/kernel/syscalls/syslog/syslog11.c   |  2 +-
>  27 files changed, 151 insertions(+), 102 deletions(-)
>
> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 8844d9f2f..fa7e2d3b0 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -1605,35 +1605,33 @@ LTP library can be instructed to save and restore value of specified
>  field 'save_restore'. It is a NULL-terminated array of struct
>  'tst_path_val' where each tst_path_val.path represents a file, whose
>  value is saved at the beginning and restored at the end of the test.
> -If non-NULL value is passed it is written to the respective file at
> -the beginning of the test. Only the first line of a specified file
> -is saved and restored.
> -
> -Pathnames can be optionally prefixed to specify how strictly (during
> -'store') are handled errors:
> -
> -* (no prefix) - test ends with 'TCONF', if file doesn't exist
> -* '?'         - test prints info message and continues,
> -                if file doesn't exist or open/read fails
> -* '!'         - test ends with 'TBROK', if file doesn't exist
> +If non-NULL string is passed in tst_path_val.val, it is written
> +to the respective file at the beginning of the test. Only the first line
> +of a specified file is saved and restored.
> +
> +By default, the test will end with TCONF if the file is read-only or
> +does not exist. If the optional write of new value fails, the test will end
> +with 'TBROK'. This behavior can be changed using tst_path_val.flags:
> +
> +* 'TST_SR_FAIL_MISSING' – End test with 'TBROK' if the file does not exist
> +* 'TST_SR_SKIP_MISSING' – Continue without saving the file if it does not exist
> +* 'TST_SR_FAIL_RO' – End test with 'TBROK' if the file is read-only
> +* 'TST_SR_SKIP_RO' – Continue without saving the file if it is read-only

Agree with Cyril; FAIL -> TBROK.

> +* 'TST_SR_IGNORE_ERR' – Ignore errors when writing new value into the file
> +* 'TST_SR_REQUIRED' – Equivalent to 'TST_SR_FAIL_MISSING | TST_SR_FAIL_RO'
> +* 'TST_SR_COND_ACCESS' – Equivalent to 'TST_SR_SKIP_MISSING | TST_SR_SKIP_RO'

It's obvious to me what required means, but not what cond_access
means. Yet they appear to be opposites. The opposite of 'required' is
'optional'. So TST_SR_COND_ACCESS -> TST_SR_OPTIONAL.

>  
>  'restore' is always strict and will TWARN if it encounters any error.
>  
>  [source,c]
>  -------------------------------------------------------------------------------
> -static void setup(void)
> -{
> -	FILE_PRINTF("/proc/sys/kernel/core_pattern", "/mypath");
> -	SAFE_TRY_FILE_PRINTF("/proc/sys/user/max_user_namespaces", "%d", 10);
> -}
> -
>  static struct tst_test test = {
>  	...
>  	.setup = setup,
>  	.save_restore = (const struct tst_path_val[]) {
> -		{"/proc/sys/kernel/core_pattern", NULL},
> -		{"?/proc/sys/user/max_user_namespaces", NULL},
> -		{"!/sys/kernel/mm/ksm/run", "1"},
> +		{"/proc/sys/kernel/core_pattern", NULL, 0},
> +		{"/proc/sys/user/max_user_namespaces", NULL, TST_SR_COND_ACCESS},
> +		{"/sys/kernel/mm/ksm/run", "1", TST_SR_REQUIRED},
>  		{}
>  	},
>  };
> diff --git a/include/tst_sys_conf.h b/include/tst_sys_conf.h
> index b7bbe36fc..b52b8913e 100644
> --- a/include/tst_sys_conf.h
> +++ b/include/tst_sys_conf.h
> @@ -5,14 +5,23 @@
>  #ifndef TST_SYS_CONF_H__
>  #define TST_SYS_CONF_H__
>  
> +#define TST_SR_FAIL_MISSING 0x1
> +#define TST_SR_SKIP_MISSING 0x2
> +#define TST_SR_FAIL_RO 0x4
> +#define TST_SR_SKIP_RO 0x8
> +#define TST_SR_IGNORE_ERR 0x10
> +
> +#define TST_SR_REQUIRED (TST_SR_FAIL_MISSING | TST_SR_FAIL_RO)
> +#define TST_SR_COND_ACCESS (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
> +
>  struct tst_path_val {
>          const char *path;
>          const char *val;
> +	unsigned int flags;
>  };
>  
> -int tst_sys_conf_save_str(const char *path, const char *value);
> -int tst_sys_conf_save(const char *path);
> -void tst_sys_conf_set(const char *path, const char *value);
> +void tst_sys_conf_save_str(const char *path, const char *value);
> +int tst_sys_conf_save(const struct tst_path_val *conf);
>  void tst_sys_conf_restore(int verbose);
>  void tst_sys_conf_dump(void);
>  
> diff --git a/lib/tst_sys_conf.c b/lib/tst_sys_conf.c
> index 003698825..7aa0f46bd 100644
> --- a/lib/tst_sys_conf.c
> +++ b/lib/tst_sys_conf.c
> @@ -20,6 +20,14 @@ struct tst_sys_conf {
>  
>  static struct tst_sys_conf *save_restore_data;
>  
> +static void print_error(int info_only, const char *err, const char *path)
> +{
> +	if (info_only)
> +		tst_res(TINFO, err, path);

No TERRNO in info messages?

> +	else
> +		tst_brk(TBROK | TERRNO, err, path);
> +}
> +
>  void tst_sys_conf_dump(void)
>  {
>  	struct tst_sys_conf *i;
> @@ -28,7 +36,7 @@ void tst_sys_conf_dump(void)
>  		tst_res(TINFO, "%s = %s", i->path, i->value);
>  }
>  
> -int tst_sys_conf_save_str(const char *path, const char *value)
> +void tst_sys_conf_save_str(const char *path, const char *value)
>  {
>  	struct tst_sys_conf *n = SAFE_MALLOC(sizeof(*n));
>  
> @@ -40,45 +48,43 @@ int tst_sys_conf_save_str(const char *path, const char *value)
>  
>  	n->next = save_restore_data;
>  	save_restore_data = n;
> -
> -	return 0;
>  }
>  
> -int tst_sys_conf_save(const char *path)
> +int tst_sys_conf_save(const struct tst_path_val *conf)
>  {
>  	char line[PATH_MAX];
> +	int ttype, iret;
>  	FILE *fp;
>  	void *ret;
> -	char flag;
>  
> -	if (!path)
> +	if (!conf || !conf->path)
>  		tst_brk(TBROK, "path is empty");
>  
> -	flag = path[0];
> -	if (flag  == '?' || flag == '!')
> -		path++;
> -
> -	if (access(path, F_OK) != 0) {
> -		switch (flag) {
> -		case '?':
> -			tst_res(TINFO, "Path not found: '%s'", path);
> -			break;
> -		case '!':
> -			tst_brk(TBROK|TERRNO, "Path not found: '%s'", path);
> -			break;
> -		default:
> -			tst_brk(TCONF|TERRNO, "Path not found: '%s'", path);
> +	if (access(conf->path, F_OK) != 0) {
> +		if (conf->flags & TST_SR_SKIP_MISSING) {
> +			tst_res(TINFO, "Path not found: %s", conf->path);
>

We can use TERRNO here as well.

> +			return 1;
>  		}
> -		return 1;
> +
> +		ttype = (conf->flags & TST_SR_FAIL_MISSING) ? TBROK : TCONF;
> +		tst_brk(ttype | TERRNO, "Path not found: %s", conf->path);
>  	}
>  
> -	fp = fopen(path, "r");
> -	if (fp == NULL) {
> -		if (flag == '?')
> +	if (access(conf->path, W_OK) != 0) {
> +		if (conf->flags & TST_SR_SKIP_RO) {
> +			tst_res(TINFO, "Path is not writable: %s", conf->path);

Again TERRNO.

Otherwise makes a lot of sense.

Setting to changes requested, given the other discussion as well.

-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v3] save_restore: Introduce new struct field for flags

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > Would you consider adding TST_SR_TCONF_MISSING and TST_SR_TCONF_RO?
>> 
>> I could set both to 0, if that's OK.
>
> I suppose that would work fine.
>
>> >> +* 'TST_SR_FAIL_MISSING' – End test with 'TBROK' if the file does not exist
>> > 
>> > This FAIL part in this name is quite misleading since the test ends with
>> > TBROK. I would say that it would be much more clear if it was named
>> > TST_SR_TBROK_MISSING.
>> 
>> Then I should also rename TST_SR_REQUIRED to TST_SR_TBROK, rename 
>> TST_SR_COND_ACCESS to TST_SR_SKIP and add TST_SR_TCONF for the two new 
>> flags.
>
> That sounds good to me, it's much closely mapped to the actual test
> result types and should be clear enough.

Uh, OK, please ignore my comment on the same subject.

>
> -- 
> Cyril Hrubis
> chrubis@suse.cz


-- 
Thank you,
Richard.

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