* [LTP] [PATCH v5] save_restore: Introduce new struct field for flags
@ 2022-11-16 16:07 Martin Doucha
2022-11-17 3:59 ` Li Wang
0 siblings, 1 reply; 7+ messages in thread
From: Martin Doucha @ 2022-11-16 16:07 UTC (permalink / raw)
To: Li Wang, Jan Stancek, ltp
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>
---
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
Changes since v3:
- Renamed flags to match tst_brk() constants
- Added TERRNO to tst_res(TINFO) messages in tst_sys_conf.c
- Added save_restore flags to newly added hugemmap08
Changes since v4:
- Print caller line number in print_error()
I've removed Jan Stancek's ack because the changes are rather significant.
Please review again.
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 | 40 +++----
include/tst_sys_conf.h | 18 +++-
lib/tst_sys_conf.c | 100 +++++++++++-------
lib/tst_test.c | 3 +-
testcases/cve/icmp_rate_limit01.c | 2 +-
testcases/kernel/containers/userns/userns08.c | 2 +-
testcases/kernel/kvm/kvm_pagefault01.c | 3 +-
.../kernel/mem/hugetlb/hugemmap/hugemmap08.c | 2 +-
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 | 9 +-
testcases/kernel/syscalls/bind/bind06.c | 2 +-
testcases/kernel/syscalls/fork/fork13.c | 2 +-
.../kernel/syscalls/ipc/msgget/msgget03.c | 2 +-
testcases/kernel/syscalls/madvise/madvise06.c | 3 +-
testcases/kernel/syscalls/madvise/madvise08.c | 2 +-
.../syscalls/migrate_pages/migrate_pages02.c | 3 +-
testcases/kernel/syscalls/sendto/sendto03.c | 2 +-
.../kernel/syscalls/setsockopt/setsockopt05.c | 2 +-
.../kernel/syscalls/setsockopt/setsockopt06.c | 2 +-
.../kernel/syscalls/setsockopt/setsockopt07.c | 2 +-
.../kernel/syscalls/setsockopt/setsockopt08.c | 2 +-
.../kernel/syscalls/setsockopt/setsockopt09.c | 2 +-
testcases/kernel/syscalls/syslog/syslog11.c | 2 +-
28 files changed, 157 insertions(+), 101 deletions(-)
diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 834a342fd..e6d121dce 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -1602,35 +1602,39 @@ 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.
+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.
-Pathnames can be optionally prefixed to specify how strictly (during
-'store') are handled errors:
+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:
-* (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
+* 'TST_SR_TBROK_MISSING' – End test with 'TBROK' if the file does not exist
+* 'TST_SR_TCONF_MISSING' – End test with 'TCONF' if the file does not exist
+* 'TST_SR_SKIP_MISSING' – Continue without saving the file if it does not exist
+* 'TST_SR_TBROK_RO' – End test with 'TBROK' if the file is read-only
+* 'TST_SR_TCONF_RO' – End test with 'TCONF' 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
+
+Common flag combinations also have shortcuts:
+
+* 'TST_SR_TCONF' – Equivalent to 'TST_SR_TCONF_MISSING | TST_SR_TCONF_RO'
+* 'TST_SR_TBROK' – Equivalent to 'TST_SR_TBROK_MISSING | TST_SR_TBROK_RO'
+* 'TST_SR_SKIP' – 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, TST_SR_TCONF},
+ {"/proc/sys/user/max_user_namespaces", NULL, TST_SR_SKIP},
+ {"/sys/kernel/mm/ksm/run", "1", TST_SR_TBROK},
{}
},
};
diff --git a/include/tst_sys_conf.h b/include/tst_sys_conf.h
index b7bbe36fc..4c85767be 100644
--- a/include/tst_sys_conf.h
+++ b/include/tst_sys_conf.h
@@ -5,14 +5,26 @@
#ifndef TST_SYS_CONF_H__
#define TST_SYS_CONF_H__
+#define TST_SR_TCONF_MISSING 0x0
+#define TST_SR_TBROK_MISSING 0x1
+#define TST_SR_SKIP_MISSING 0x2
+#define TST_SR_TCONF_RO 0x0
+#define TST_SR_TBROK_RO 0x4
+#define TST_SR_SKIP_RO 0x8
+#define TST_SR_IGNORE_ERR 0x10
+
+#define TST_SR_TCONF (TST_SR_TCONF_MISSING | TST_SR_TCONF_RO)
+#define TST_SR_TBROK (TST_SR_TBROK_MISSING | TST_SR_TBROK_RO)
+#define TST_SR_SKIP (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..c0981dcb1 100644
--- a/lib/tst_sys_conf.c
+++ b/lib/tst_sys_conf.c
@@ -20,6 +20,15 @@ struct tst_sys_conf {
static struct tst_sys_conf *save_restore_data;
+static void print_error(const int lineno, int info_only, const char *err,
+ const char *path)
+{
+ if (info_only)
+ tst_res_(__FILE__, lineno, TINFO | TERRNO, err, path);
+ else
+ tst_brk_(__FILE__, lineno, TBROK | TERRNO, err, path);
+}
+
void tst_sys_conf_dump(void)
{
struct tst_sys_conf *i;
@@ -28,7 +37,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 +49,45 @@ 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 | TERRNO, "Path not found: %s",
+ conf->path);
+ return 1;
}
- return 1;
+
+ ttype = (conf->flags & TST_SR_TBROK_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 | TERRNO, "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_TBROK_RO) ? TBROK : TCONF;
+ tst_brk(ttype | TERRNO, "Path is not writable: %s", conf->path);
+ }
+
+ fp = fopen(conf->path, "r");
+
+ if (fp == NULL) {
+ print_error(__LINE__, conf->flags & TST_SR_IGNORE_ERR,
+ "Failed to open '%s' for reading", conf->path);
return 1;
}
@@ -86,24 +95,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(__LINE__, 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(__LINE__, conf->flags & TST_SR_IGNORE_ERR,
+ "Failed to write into '%s'", conf->path);
+ }
+
+ iret = fclose(fp);
+
+ if (iret < 0) {
+ print_error(__LINE__, 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 1732fd058..4976dded8 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..8f876722f 100644
--- a/testcases/cve/icmp_rate_limit01.c
+++ b/testcases/cve/icmp_rate_limit01.c
@@ -269,7 +269,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/containers/userns/userns08.c b/testcases/kernel/containers/userns/userns08.c
index 0f1cb54a9..afdad6cad 100644
--- a/testcases/kernel/containers/userns/userns08.c
+++ b/testcases/kernel/containers/userns/userns08.c
@@ -135,7 +135,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/kvm/kvm_pagefault01.c b/testcases/kernel/kvm/kvm_pagefault01.c
index e355fa448..91891848a 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 | TST_SR_TCONF_RO},
{}
},
.supported_archs = (const char *const []) {
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
index 8b3bca5e3..bacd443d8 100644
--- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
@@ -131,7 +131,7 @@ static struct tst_test test = {
.needs_hugetlbfs = 1,
.forks_child = 1,
.save_restore = (const struct tst_path_val[]) {
- {PATH_OC_HPAGES, NULL},
+ {PATH_OC_HPAGES, NULL, TST_SR_TCONF},
{}
},
.tcnt = 2,
diff --git a/testcases/kernel/mem/ksm/ksm01.c b/testcases/kernel/mem/ksm/ksm01.c
index 1f3852fc3..fafa2da71 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_TBROK},
+ {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
+ {"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
+ {"/sys/kernel/mm/ksm/merge_across_nodes", "1",
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
{}
},
.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm02.c b/testcases/kernel/mem/ksm/ksm02.c
index 4183108d5..b5c90464e 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_TBROK},
+ {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
+ {"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
+ {"/sys/kernel/mm/ksm/merge_across_nodes", "1",
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
{}
},
.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm03.c b/testcases/kernel/mem/ksm/ksm03.c
index 1cf2e4954..94029054f 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_TBROK},
+ {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
+ {"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
+ {"/sys/kernel/mm/ksm/merge_across_nodes", "1",
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
{}
},
.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm04.c b/testcases/kernel/mem/ksm/ksm04.c
index 9fe9d6dd5..2302a2a1d 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_TBROK},
+ {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
+ {"/sys/kernel/mm/ksm/max_page_sharing", NULL,
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
+ {"/sys/kernel/mm/ksm/merge_across_nodes", "1",
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
{}
},
.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm05.c b/testcases/kernel/mem/ksm/ksm05.c
index 146a9a3b7..c4cb779aa 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_TBROK},
{}
},
.needs_kconfigs = (const char *const[]){
diff --git a/testcases/kernel/mem/ksm/ksm06.c b/testcases/kernel/mem/ksm/ksm06.c
index 21c82edc1..0b159e5c7 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 | TST_SR_TCONF_RO},
+ {"/sys/kernel/mm/ksm/run", NULL, TST_SR_TBROK},
+ {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
+ {"/sys/kernel/mm/ksm/merge_across_nodes", NULL, TST_SR_TCONF},
{}
},
.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..74b0b54dd 100644
--- a/testcases/kernel/syscalls/add_key/add_key05.c
+++ b/testcases/kernel/syscalls/add_key/add_key05.c
@@ -220,9 +220,12 @@ 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 | TST_SR_TCONF_RO},
+ {"/proc/sys/kernel/keys/maxkeys", "200",
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
+ {"/proc/sys/kernel/keys/maxbytes", "20000",
+ TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
{}
},
.bufs = (struct tst_buffers []) {
diff --git a/testcases/kernel/syscalls/bind/bind06.c b/testcases/kernel/syscalls/bind/bind06.c
index 618cfce46..f7813d26a 100644
--- a/testcases/kernel/syscalls/bind/bind06.c
+++ b/testcases/kernel/syscalls/bind/bind06.c
@@ -110,7 +110,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/fork/fork13.c b/testcases/kernel/syscalls/fork/fork13.c
index fe30d1e9c..bbfbf5c3b 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_TBROK},
{}
},
.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..9e6c66cb4 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, TST_SR_TCONF},
{}
}
};
diff --git a/testcases/kernel/syscalls/madvise/madvise06.c b/testcases/kernel/syscalls/madvise/madvise06.c
index 27aff18f1..c1c55bbc2 100644
--- a/testcases/kernel/syscalls/madvise/madvise06.c
+++ b/testcases/kernel/syscalls/madvise/madvise06.c
@@ -239,7 +239,8 @@ 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 | TST_SR_TCONF_RO},
{}
},
.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..0996cf91b 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, TST_SR_TCONF},
{}
},
};
diff --git a/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c b/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
index 32da57042..4d5b2b8d5 100644
--- a/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
+++ b/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
@@ -327,7 +327,8 @@ 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 | TST_SR_TCONF_RO},
{}
},
};
diff --git a/testcases/kernel/syscalls/sendto/sendto03.c b/testcases/kernel/syscalls/sendto/sendto03.c
index 5d2c1e112..3709b287c 100644
--- a/testcases/kernel/syscalls/sendto/sendto03.c
+++ b/testcases/kernel/syscalls/sendto/sendto03.c
@@ -218,7 +218,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
index 651583fb6..580467dc8 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
@@ -102,7 +102,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt06.c b/testcases/kernel/syscalls/setsockopt/setsockopt06.c
index 9c818646b..e67996a78 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt06.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt06.c
@@ -130,7 +130,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt07.c b/testcases/kernel/syscalls/setsockopt/setsockopt07.c
index 616159a90..1c5a0ed6d 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt07.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt07.c
@@ -143,7 +143,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
index 563444635..a29125fd7 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt08.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
@@ -159,7 +159,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt09.c b/testcases/kernel/syscalls/setsockopt/setsockopt09.c
index 98f7fd00e..b49b17e7d 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt09.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt09.c
@@ -124,7 +124,7 @@ 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_SKIP},
{}
},
.tags = (const struct tst_tag[]) {
diff --git a/testcases/kernel/syscalls/syslog/syslog11.c b/testcases/kernel/syscalls/syslog/syslog11.c
index ca1ecbbe3..733da5f96 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_TBROK},
{}
},
.needs_root = 1,
--
2.37.3
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH v5] save_restore: Introduce new struct field for flags
2022-11-16 16:07 [LTP] [PATCH v5] save_restore: Introduce new struct field for flags Martin Doucha
@ 2022-11-17 3:59 ` Li Wang
2022-11-18 13:31 ` Cyril Hrubis
2022-11-21 15:36 ` Martin Doucha
0 siblings, 2 replies; 7+ messages in thread
From: Li Wang @ 2022-11-17 3:59 UTC (permalink / raw)
To: Martin Doucha; +Cc: ltp
[-- Attachment #1.1: Type: text/plain, Size: 32260 bytes --]
On Thu, Nov 17, 2022 at 12:07 AM Martin Doucha <mdoucha@suse.cz> wrote:
> 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>
> ---
> 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
>
> Changes since v3:
> - Renamed flags to match tst_brk() constants
> - Added TERRNO to tst_res(TINFO) messages in tst_sys_conf.c
> - Added save_restore flags to newly added hugemmap08
>
> Changes since v4:
> - Print caller line number in print_error()
>
> I've removed Jan Stancek's ack because the changes are rather significant.
> Please review again.
>
> 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 | 40 +++----
> include/tst_sys_conf.h | 18 +++-
> lib/tst_sys_conf.c | 100 +++++++++++-------
> lib/tst_test.c | 3 +-
> testcases/cve/icmp_rate_limit01.c | 2 +-
> testcases/kernel/containers/userns/userns08.c | 2 +-
> testcases/kernel/kvm/kvm_pagefault01.c | 3 +-
> .../kernel/mem/hugetlb/hugemmap/hugemmap08.c | 2 +-
> 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 | 9 +-
> testcases/kernel/syscalls/bind/bind06.c | 2 +-
> testcases/kernel/syscalls/fork/fork13.c | 2 +-
> .../kernel/syscalls/ipc/msgget/msgget03.c | 2 +-
> testcases/kernel/syscalls/madvise/madvise06.c | 3 +-
> testcases/kernel/syscalls/madvise/madvise08.c | 2 +-
> .../syscalls/migrate_pages/migrate_pages02.c | 3 +-
> testcases/kernel/syscalls/sendto/sendto03.c | 2 +-
> .../kernel/syscalls/setsockopt/setsockopt05.c | 2 +-
> .../kernel/syscalls/setsockopt/setsockopt06.c | 2 +-
> .../kernel/syscalls/setsockopt/setsockopt07.c | 2 +-
> .../kernel/syscalls/setsockopt/setsockopt08.c | 2 +-
> .../kernel/syscalls/setsockopt/setsockopt09.c | 2 +-
> testcases/kernel/syscalls/syslog/syslog11.c | 2 +-
> 28 files changed, 157 insertions(+), 101 deletions(-)
>
> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 834a342fd..e6d121dce 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -1602,35 +1602,39 @@ 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.
> +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.
>
> -Pathnames can be optionally prefixed to specify how strictly (during
> -'store') are handled errors:
> +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:
>
> -* (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
> +* 'TST_SR_TBROK_MISSING' – End test with 'TBROK' if the file does not
> exist
> +* 'TST_SR_TCONF_MISSING' – End test with 'TCONF' if the file does not
> exist
> +* 'TST_SR_SKIP_MISSING' – Continue without saving the file if it does not
> exist
> +* 'TST_SR_TBROK_RO' – End test with 'TBROK' if the file is read-only
> +* 'TST_SR_TCONF_RO' – End test with 'TCONF' 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
> +
> +Common flag combinations also have shortcuts:
> +
> +* 'TST_SR_TCONF' – Equivalent to 'TST_SR_TCONF_MISSING | TST_SR_TCONF_RO'
> +* 'TST_SR_TBROK' – Equivalent to 'TST_SR_TBROK_MISSING | TST_SR_TBROK_RO'
> +* 'TST_SR_SKIP' – 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, TST_SR_TCONF},
> + {"/proc/sys/user/max_user_namespaces", NULL, TST_SR_SKIP},
> + {"/sys/kernel/mm/ksm/run", "1", TST_SR_TBROK},
> {}
> },
> };
> diff --git a/include/tst_sys_conf.h b/include/tst_sys_conf.h
> index b7bbe36fc..4c85767be 100644
> --- a/include/tst_sys_conf.h
> +++ b/include/tst_sys_conf.h
> @@ -5,14 +5,26 @@
> #ifndef TST_SYS_CONF_H__
> #define TST_SYS_CONF_H__
>
> +#define TST_SR_TCONF_MISSING 0x0
> +#define TST_SR_TBROK_MISSING 0x1
> +#define TST_SR_SKIP_MISSING 0x2
> +#define TST_SR_TCONF_RO 0x0
> +#define TST_SR_TBROK_RO 0x4
> +#define TST_SR_SKIP_RO 0x8
>
> +#define TST_SR_IGNORE_ERR 0x10
>
I didn't see any test using TST_SR_IGNORE_ERR flag in this patch,
is this prepared for the coming patchset?
> +
> +#define TST_SR_TCONF (TST_SR_TCONF_MISSING | TST_SR_TCONF_RO)
> +#define TST_SR_TBROK (TST_SR_TBROK_MISSING | TST_SR_TBROK_RO)
> +#define TST_SR_SKIP (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
>
I'd suggest using "_TSKIP" to replace "_SKIP' to be consistent.
Then the whole format will look quite perfect and easy to memorize:).
Reviewed-by: Li Wang <liwang@redhat.com>
> +
> 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..c0981dcb1 100644
> --- a/lib/tst_sys_conf.c
> +++ b/lib/tst_sys_conf.c
> @@ -20,6 +20,15 @@ struct tst_sys_conf {
>
> static struct tst_sys_conf *save_restore_data;
>
> +static void print_error(const int lineno, int info_only, const char *err,
> + const char *path)
> +{
> + if (info_only)
> + tst_res_(__FILE__, lineno, TINFO | TERRNO, err, path);
> + else
> + tst_brk_(__FILE__, lineno, TBROK | TERRNO, err, path);
> +}
> +
> void tst_sys_conf_dump(void)
> {
> struct tst_sys_conf *i;
> @@ -28,7 +37,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 +49,45 @@ 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 | TERRNO, "Path not found: %s",
> + conf->path);
> + return 1;
> }
> - return 1;
> +
> + ttype = (conf->flags & TST_SR_TBROK_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 | TERRNO, "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_TBROK_RO) ? TBROK : TCONF;
> + tst_brk(ttype | TERRNO, "Path is not writable: %s",
> conf->path);
> + }
> +
> + fp = fopen(conf->path, "r");
> +
> + if (fp == NULL) {
> + print_error(__LINE__, conf->flags & TST_SR_IGNORE_ERR,
> + "Failed to open '%s' for reading", conf->path);
> return 1;
> }
>
> @@ -86,24 +95,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(__LINE__, 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(__LINE__, conf->flags & TST_SR_IGNORE_ERR,
> + "Failed to write into '%s'", conf->path);
> + }
> +
> + iret = fclose(fp);
> +
> + if (iret < 0) {
> + print_error(__LINE__, 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 1732fd058..4976dded8 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..8f876722f 100644
> --- a/testcases/cve/icmp_rate_limit01.c
> +++ b/testcases/cve/icmp_rate_limit01.c
> @@ -269,7 +269,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/containers/userns/userns08.c
> b/testcases/kernel/containers/userns/userns08.c
> index 0f1cb54a9..afdad6cad 100644
> --- a/testcases/kernel/containers/userns/userns08.c
> +++ b/testcases/kernel/containers/userns/userns08.c
> @@ -135,7 +135,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/kvm/kvm_pagefault01.c
> b/testcases/kernel/kvm/kvm_pagefault01.c
> index e355fa448..91891848a 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 | TST_SR_TCONF_RO},
> {}
> },
> .supported_archs = (const char *const []) {
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> index 8b3bca5e3..bacd443d8 100644
> --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> @@ -131,7 +131,7 @@ static struct tst_test test = {
> .needs_hugetlbfs = 1,
> .forks_child = 1,
> .save_restore = (const struct tst_path_val[]) {
> - {PATH_OC_HPAGES, NULL},
> + {PATH_OC_HPAGES, NULL, TST_SR_TCONF},
> {}
> },
> .tcnt = 2,
> diff --git a/testcases/kernel/mem/ksm/ksm01.c
> b/testcases/kernel/mem/ksm/ksm01.c
> index 1f3852fc3..fafa2da71 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_TBROK},
> + {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
> + {"/sys/kernel/mm/ksm/max_page_sharing", NULL,
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> + {"/sys/kernel/mm/ksm/merge_across_nodes", "1",
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> {}
> },
> .needs_kconfigs = (const char *const[]){
> diff --git a/testcases/kernel/mem/ksm/ksm02.c
> b/testcases/kernel/mem/ksm/ksm02.c
> index 4183108d5..b5c90464e 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_TBROK},
> + {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
> + {"/sys/kernel/mm/ksm/max_page_sharing", NULL,
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> + {"/sys/kernel/mm/ksm/merge_across_nodes", "1",
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> {}
> },
> .needs_kconfigs = (const char *const[]){
> diff --git a/testcases/kernel/mem/ksm/ksm03.c
> b/testcases/kernel/mem/ksm/ksm03.c
> index 1cf2e4954..94029054f 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_TBROK},
> + {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
> + {"/sys/kernel/mm/ksm/max_page_sharing", NULL,
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> + {"/sys/kernel/mm/ksm/merge_across_nodes", "1",
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> {}
> },
> .needs_kconfigs = (const char *const[]){
> diff --git a/testcases/kernel/mem/ksm/ksm04.c
> b/testcases/kernel/mem/ksm/ksm04.c
> index 9fe9d6dd5..2302a2a1d 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_TBROK},
> + {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
> + {"/sys/kernel/mm/ksm/max_page_sharing", NULL,
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> + {"/sys/kernel/mm/ksm/merge_across_nodes", "1",
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> {}
> },
> .needs_kconfigs = (const char *const[]){
> diff --git a/testcases/kernel/mem/ksm/ksm05.c
> b/testcases/kernel/mem/ksm/ksm05.c
> index 146a9a3b7..c4cb779aa 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_TBROK},
> {}
> },
> .needs_kconfigs = (const char *const[]){
> diff --git a/testcases/kernel/mem/ksm/ksm06.c
> b/testcases/kernel/mem/ksm/ksm06.c
> index 21c82edc1..0b159e5c7 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 | TST_SR_TCONF_RO},
> + {"/sys/kernel/mm/ksm/run", NULL, TST_SR_TBROK},
> + {"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
> + {"/sys/kernel/mm/ksm/merge_across_nodes", NULL,
> TST_SR_TCONF},
> {}
> },
> .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..74b0b54dd 100644
> --- a/testcases/kernel/syscalls/add_key/add_key05.c
> +++ b/testcases/kernel/syscalls/add_key/add_key05.c
> @@ -220,9 +220,12 @@ 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 | TST_SR_TCONF_RO},
> + {"/proc/sys/kernel/keys/maxkeys", "200",
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> + {"/proc/sys/kernel/keys/maxbytes", "20000",
> + TST_SR_SKIP_MISSING | TST_SR_TCONF_RO},
> {}
> },
> .bufs = (struct tst_buffers []) {
> diff --git a/testcases/kernel/syscalls/bind/bind06.c
> b/testcases/kernel/syscalls/bind/bind06.c
> index 618cfce46..f7813d26a 100644
> --- a/testcases/kernel/syscalls/bind/bind06.c
> +++ b/testcases/kernel/syscalls/bind/bind06.c
> @@ -110,7 +110,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/syscalls/fork/fork13.c
> b/testcases/kernel/syscalls/fork/fork13.c
> index fe30d1e9c..bbfbf5c3b 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_TBROK},
> {}
> },
> .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..9e6c66cb4 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, TST_SR_TCONF},
> {}
> }
> };
> diff --git a/testcases/kernel/syscalls/madvise/madvise06.c
> b/testcases/kernel/syscalls/madvise/madvise06.c
> index 27aff18f1..c1c55bbc2 100644
> --- a/testcases/kernel/syscalls/madvise/madvise06.c
> +++ b/testcases/kernel/syscalls/madvise/madvise06.c
> @@ -239,7 +239,8 @@ 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 | TST_SR_TCONF_RO},
> {}
> },
> .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..0996cf91b 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, TST_SR_TCONF},
> {}
> },
> };
> diff --git a/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
> b/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
> index 32da57042..4d5b2b8d5 100644
> --- a/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
> +++ b/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
> @@ -327,7 +327,8 @@ 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 | TST_SR_TCONF_RO},
> {}
> },
> };
> diff --git a/testcases/kernel/syscalls/sendto/sendto03.c
> b/testcases/kernel/syscalls/sendto/sendto03.c
> index 5d2c1e112..3709b287c 100644
> --- a/testcases/kernel/syscalls/sendto/sendto03.c
> +++ b/testcases/kernel/syscalls/sendto/sendto03.c
> @@ -218,7 +218,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> index 651583fb6..580467dc8 100644
> --- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> +++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> @@ -102,7 +102,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt06.c
> b/testcases/kernel/syscalls/setsockopt/setsockopt06.c
> index 9c818646b..e67996a78 100644
> --- a/testcases/kernel/syscalls/setsockopt/setsockopt06.c
> +++ b/testcases/kernel/syscalls/setsockopt/setsockopt06.c
> @@ -130,7 +130,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt07.c
> b/testcases/kernel/syscalls/setsockopt/setsockopt07.c
> index 616159a90..1c5a0ed6d 100644
> --- a/testcases/kernel/syscalls/setsockopt/setsockopt07.c
> +++ b/testcases/kernel/syscalls/setsockopt/setsockopt07.c
> @@ -143,7 +143,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c
> b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
> index 563444635..a29125fd7 100644
> --- a/testcases/kernel/syscalls/setsockopt/setsockopt08.c
> +++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
> @@ -159,7 +159,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt09.c
> b/testcases/kernel/syscalls/setsockopt/setsockopt09.c
> index 98f7fd00e..b49b17e7d 100644
> --- a/testcases/kernel/syscalls/setsockopt/setsockopt09.c
> +++ b/testcases/kernel/syscalls/setsockopt/setsockopt09.c
> @@ -124,7 +124,7 @@ 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_SKIP},
> {}
> },
> .tags = (const struct tst_tag[]) {
> diff --git a/testcases/kernel/syscalls/syslog/syslog11.c
> b/testcases/kernel/syscalls/syslog/syslog11.c
> index ca1ecbbe3..733da5f96 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_TBROK},
> {}
> },
> .needs_root = 1,
> --
> 2.37.3
>
>
--
Regards,
Li Wang
[-- Attachment #1.2: Type: text/html, Size: 39423 bytes --]
[-- Attachment #2: Type: text/plain, Size: 60 bytes --]
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH v5] save_restore: Introduce new struct field for flags
2022-11-17 3:59 ` Li Wang
@ 2022-11-18 13:31 ` Cyril Hrubis
2022-11-22 8:02 ` Jan Stancek
2022-11-21 15:36 ` Martin Doucha
1 sibling, 1 reply; 7+ messages in thread
From: Cyril Hrubis @ 2022-11-18 13:31 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
Hi!
> > +#define TST_SR_TCONF (TST_SR_TCONF_MISSING | TST_SR_TCONF_RO)
> > +#define TST_SR_TBROK (TST_SR_TBROK_MISSING | TST_SR_TBROK_RO)
> > +#define TST_SR_SKIP (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
> >
>
> I'd suggest using "_TSKIP" to replace "_SKIP' to be consistent.
> Then the whole format will look quite perfect and easy to memorize:).
I think that it makse sense to keep it as _SKIP because that is the
option that does not map directly to the test result flags. If we named
it TSKIP people will be confused that there is no TSKIP in the test
result flags at all.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH v5] save_restore: Introduce new struct field for flags
2022-11-17 3:59 ` Li Wang
2022-11-18 13:31 ` Cyril Hrubis
@ 2022-11-21 15:36 ` Martin Doucha
1 sibling, 0 replies; 7+ messages in thread
From: Martin Doucha @ 2022-11-21 15:36 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
On 17. 11. 22 4:59, Li Wang wrote:
> I didn't see any test using TST_SR_IGNORE_ERR flag in this patch,
> is this prepared for the coming patchset?
No, I've added TST_SR_IGNORE_ERR just in case we need to work around
some sysfile kernel bugs in the future. I don't have any use for it so far.
> +
> +#define TST_SR_TCONF (TST_SR_TCONF_MISSING | TST_SR_TCONF_RO)
> +#define TST_SR_TBROK (TST_SR_TBROK_MISSING | TST_SR_TBROK_RO)
> +#define TST_SR_SKIP (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
>
>
> I'd suggest using "_TSKIP" to replace "_SKIP' to be consistent.
> Then the whole format will look quite perfect and easy to memorize:).
I agree with Cyril, _SKIP does not refer to any result flag so it should
not look like one.
Thanks for review.
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH v5] save_restore: Introduce new struct field for flags
2022-11-18 13:31 ` Cyril Hrubis
@ 2022-11-22 8:02 ` Jan Stancek
2022-11-24 3:09 ` Li Wang
0 siblings, 1 reply; 7+ messages in thread
From: Jan Stancek @ 2022-11-22 8:02 UTC (permalink / raw)
To: Martin Doucha; +Cc: ltp
On Fri, Nov 18, 2022 at 2:30 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > > +#define TST_SR_TCONF (TST_SR_TCONF_MISSING | TST_SR_TCONF_RO)
> > > +#define TST_SR_TBROK (TST_SR_TBROK_MISSING | TST_SR_TBROK_RO)
> > > +#define TST_SR_SKIP (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
> > >
> >
> > I'd suggest using "_TSKIP" to replace "_SKIP' to be consistent.
> > Then the whole format will look quite perfect and easy to memorize:).
>
> I think that it makse sense to keep it as _SKIP because that is the
> option that does not map directly to the test result flags. If we named
> it TSKIP people will be confused that there is no TSKIP in the test
> result flags at all.
+1 for SKIP
Feel free to add my Acked-by back.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH v5] save_restore: Introduce new struct field for flags
2022-11-22 8:02 ` Jan Stancek
@ 2022-11-24 3:09 ` Li Wang
2022-11-24 10:10 ` Martin Doucha
0 siblings, 1 reply; 7+ messages in thread
From: Li Wang @ 2022-11-24 3:09 UTC (permalink / raw)
To: Jan Stancek, Martin Doucha; +Cc: ltp
[-- Attachment #1.1: Type: text/plain, Size: 3334 bytes --]
Hi All,
On Tue, Nov 22, 2022 at 4:02 PM Jan Stancek <jstancek@redhat.com> wrote:
> On Fri, Nov 18, 2022 at 2:30 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> >
> > Hi!
> > > > +#define TST_SR_TCONF (TST_SR_TCONF_MISSING | TST_SR_TCONF_RO)
> > > > +#define TST_SR_TBROK (TST_SR_TBROK_MISSING | TST_SR_TBROK_RO)
> > > > +#define TST_SR_SKIP (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
> > > >
> > >
> > > I'd suggest using "_TSKIP" to replace "_SKIP' to be consistent.
> > > Then the whole format will look quite perfect and easy to memorize:).
> >
> > I think that it makse sense to keep it as _SKIP because that is the
> > option that does not map directly to the test result flags. If we named
> > it TSKIP people will be confused that there is no TSKIP in the test
> > result flags at all.
>
> +1 for SKIP
>
> Feel free to add my Acked-by back.
>
Sure, no problem.
But before applying patch-v5 as it is, I found additional two tiny issues:
1. the usage in newlib_tests should be corrected as well
2. tst_hugepage need to update the argument in tst_sys_conf_save()
If everyone agrees on the improvements, I will merge with below fixes:
--- a/lib/newlib_tests/test19.c
+++ b/lib/newlib_tests/test19.c
@@ -24,9 +24,9 @@ static struct tst_test test = {
.test_all = run,
.setup = setup,
.save_restore = (const struct tst_path_val[]) {
- {"?/proc/nonexistent", NULL},
- {"!/proc/sys/kernel/numa_balancing", NULL},
- {"/proc/sys/kernel/core_pattern", NULL},
+ {"/proc/nonexistent", NULL, TST_SR_SKIP},
+ {"/proc/sys/kernel/numa_balancing", NULL, TST_SR_TBROK},
+ {"/proc/sys/kernel/core_pattern", NULL, TST_SR_TCONF},
{}
},
};
diff --git a/lib/newlib_tests/test20.c b/lib/newlib_tests/test20.c
index 3982ab7..3726cea 100644
--- a/lib/newlib_tests/test20.c
+++ b/lib/newlib_tests/test20.c
@@ -39,7 +39,7 @@ static struct tst_test test = {
.test_all = do_test,
.hugepages = {2, TST_NEEDS},
.save_restore = (const struct tst_path_val[]) {
- {"!/proc/sys/kernel/numa_balancing", "0"},
+ {"/proc/sys/kernel/numa_balancing", "0", TST_SR_TBROK},
{}
},
};
diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
index e97cc56..41f65d7 100644
--- a/lib/tst_hugepage.c
+++ b/lib/tst_hugepage.c
@@ -7,6 +7,7 @@
#include "tst_test.h"
#include "tst_hugepage.h"
+#include "tst_sys_conf.h"
unsigned long tst_hugepages;
char *nr_opt;
@@ -24,6 +25,12 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage
*hp)
{
unsigned long val, max_hpages;
+ struct tst_path_val pvl = {
+ .path = PATH_NR_HPAGES,
+ .val = NULL,
+ .flags = TST_SR_SKIP,
+ };
+
if (access(PATH_HUGEPAGES, F_OK)) {
if (hp->policy == TST_NEEDS)
tst_brk(TCONF, "hugetlbfs is not supported");
@@ -59,7 +66,7 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage
*hp)
}
set_hugepages:
- tst_sys_conf_save("?/proc/sys/vm/nr_hugepages");
+ tst_sys_conf_save(&pvl);
SAFE_FILE_PRINTF(PATH_NR_HPAGES, "%lu", tst_hugepages);
SAFE_FILE_SCANF(PATH_NR_HPAGES, "%lu", &val);
if (val != tst_hugepages)
--
Regards,
Li Wang
[-- Attachment #1.2: Type: text/html, Size: 5279 bytes --]
[-- Attachment #2: Type: text/plain, Size: 60 bytes --]
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH v5] save_restore: Introduce new struct field for flags
2022-11-24 3:09 ` Li Wang
@ 2022-11-24 10:10 ` Martin Doucha
0 siblings, 0 replies; 7+ messages in thread
From: Martin Doucha @ 2022-11-24 10:10 UTC (permalink / raw)
To: Li Wang, Jan Stancek; +Cc: ltp
On 24. 11. 22 4:09, Li Wang wrote:
> But before applying patch-v5 as it is, I found additional two tiny issues:
>
> 1. the usage in newlib_tests should be corrected as well
> 2. tst_hugepage need to update the argument in tst_sys_conf_save()
Good catch. I'm surprised that tst_hugepage.c compiles with only a
warning...
>
> If everyone agrees on the improvements, I will merge with below fixes:
>
> --- a/lib/newlib_tests/test19.c
> +++ b/lib/newlib_tests/test19.c
> @@ -24,9 +24,9 @@ static struct tst_test test = {
> .test_all = run,
> .setup = setup,
> .save_restore = (const struct tst_path_val[]) {
> - {"?/proc/nonexistent", NULL},
> - {"!/proc/sys/kernel/numa_balancing", NULL},
> - {"/proc/sys/kernel/core_pattern", NULL},
> + {"/proc/nonexistent", NULL, TST_SR_SKIP},
> + {"/proc/sys/kernel/numa_balancing", NULL, TST_SR_TBROK},
> + {"/proc/sys/kernel/core_pattern", NULL, TST_SR_TCONF},
> {}
> },
> };
> diff --git a/lib/newlib_tests/test20.c b/lib/newlib_tests/test20.c
> index 3982ab7..3726cea 100644
> --- a/lib/newlib_tests/test20.c
> +++ b/lib/newlib_tests/test20.c
> @@ -39,7 +39,7 @@ static struct tst_test test = {
> .test_all = do_test,
> .hugepages = {2, TST_NEEDS},
> .save_restore = (const struct tst_path_val[]) {
> - {"!/proc/sys/kernel/numa_balancing", "0"},
> + {"/proc/sys/kernel/numa_balancing", "0", TST_SR_TBROK},
> {}
> },
> };
> diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
> index e97cc56..41f65d7 100644
> --- a/lib/tst_hugepage.c
> +++ b/lib/tst_hugepage.c
> @@ -7,6 +7,7 @@
>
> #include "tst_test.h"
> #include "tst_hugepage.h"
> +#include "tst_sys_conf.h"
tst_sys_conf.h is already included through tst_test.h
> unsigned long tst_hugepages;
> char *nr_opt;
> @@ -24,6 +25,12 @@ unsigned long tst_reserve_hugepages(struct
> tst_hugepage *hp)
> {
> unsigned long val, max_hpages;
>
> + struct tst_path_val pvl = {
> + .path = PATH_NR_HPAGES,
> + .val = NULL,
> + .flags = TST_SR_SKIP,
> + };
The save() is followed by unconditional SAFE_FILE_PRINTF() so the flags
should be TST_SR_SKIP_MISSING | TST_SR_TCONF_RO instead. I'll send a v6.
> +
> if (access(PATH_HUGEPAGES, F_OK)) {
> if (hp->policy == TST_NEEDS)
> tst_brk(TCONF, "hugetlbfs is not supported");
> @@ -59,7 +66,7 @@ unsigned long tst_reserve_hugepages(struct
> tst_hugepage *hp)
> }
>
> set_hugepages:
> - tst_sys_conf_save("?/proc/sys/vm/nr_hugepages");
> + tst_sys_conf_save(&pvl);
> SAFE_FILE_PRINTF(PATH_NR_HPAGES, "%lu", tst_hugepages);
> SAFE_FILE_SCANF(PATH_NR_HPAGES, "%lu", &val);
> if (val != tst_hugepages)
>
>
> --
> Regards,
> Li Wang
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-24 10:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-16 16:07 [LTP] [PATCH v5] save_restore: Introduce new struct field for flags Martin Doucha
2022-11-17 3:59 ` Li Wang
2022-11-18 13:31 ` Cyril Hrubis
2022-11-22 8:02 ` Jan Stancek
2022-11-24 3:09 ` Li Wang
2022-11-24 10:10 ` Martin Doucha
2022-11-21 15:36 ` Martin Doucha
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.