[LTP] [PATCH 0/5] First new shell library converted test

[LTP] [PATCH 0/5] First new shell library converted test

[Cyril Hrubis]

This patchset contains a few fixes for the new shell library and first
converted test.

Cyril Hrubis (5):
  tst_run_shell: Add save_restore parser
  libs/ujson: Fix "Garbage after JSON string!" in strict mode
  tst_run_shell: Better errors for metadata extractor
  lib/tst_res_.c: Add TBROK handler + more verbose errors
  mem/vma05.sh: Convert to the new shell library

 libs/ujson/ujson_reader.c         |  6 +-
 testcases/kernel/mem/vma/vma05.sh | 97 ++++++++++++++++---------------
 testcases/lib/tst_res_.c          | 22 ++++---
 testcases/lib/tst_run_shell.c     | 97 +++++++++++++++++++++++++++++--
 4 files changed, 162 insertions(+), 60 deletions(-)

-- 
2.45.2


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

[LTP] [PATCH 1/5] tst_run_shell: Add save_restore parser

[Cyril Hrubis]

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/lib/tst_run_shell.c | 72 +++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/testcases/lib/tst_run_shell.c b/testcases/lib/tst_run_shell.c
index 95cac0d60..836e01f18 100644
--- a/testcases/lib/tst_run_shell.c
+++ b/testcases/lib/tst_run_shell.c
@@ -74,6 +74,7 @@ enum test_attr_ids {
 	NEEDS_ROOT,
 	NEEDS_TMPDIR,
 	RESTORE_WALLCLOCK,
+	SAVE_RESTORE,
 	SKIP_FILESYSTEMS,
 	SKIP_IN_COMPAT,
 	SKIP_IN_LOCKDOWN,
@@ -106,6 +107,7 @@ static ujson_obj_attr test_attrs[] = {
 	UJSON_OBJ_ATTR_IDX(NEEDS_ROOT, "needs_root", UJSON_BOOL),
 	UJSON_OBJ_ATTR_IDX(NEEDS_TMPDIR, "needs_tmpdir", UJSON_BOOL),
 	UJSON_OBJ_ATTR_IDX(RESTORE_WALLCLOCK, "restore_wallclock", UJSON_BOOL),
+	UJSON_OBJ_ATTR_IDX(SAVE_RESTORE, "save_restore", UJSON_ARR),
 	UJSON_OBJ_ATTR_IDX(SKIP_FILESYSTEMS, "skip_filesystems", UJSON_ARR),
 	UJSON_OBJ_ATTR_IDX(SKIP_IN_COMPAT, "skip_in_compat", UJSON_BOOL),
 	UJSON_OBJ_ATTR_IDX(SKIP_IN_LOCKDOWN, "skip_in_lockdown", UJSON_BOOL),
@@ -299,6 +301,73 @@ static struct tst_tag *parse_tags(ujson_reader *reader, ujson_val *val)
 	return ret;
 }
 
+static struct tst_path_val *parse_save_restore(ujson_reader *reader, ujson_val *val)
+{
+	unsigned int i = 0, cnt = 0;
+	struct tst_path_val *ret;
+
+	ujson_reader_state state = ujson_reader_state_save(reader);
+
+	UJSON_ARR_FOREACH(reader, val) {
+		if (val->type != UJSON_ARR) {
+			ujson_err(reader, "Expected array!");
+			return NULL;
+		}
+		ujson_arr_skip(reader);
+		cnt++;
+	}
+
+	ujson_reader_state_load(reader, state);
+
+	ret = SAFE_MALLOC(sizeof(struct tst_path_val) * (cnt + 1));
+	memset(&ret[cnt], 0, sizeof(ret[cnt]));
+
+	UJSON_ARR_FOREACH(reader, val) {
+		char *path = NULL;
+		int flags_set = 0;
+		unsigned int flags = 0;
+
+		UJSON_ARR_FOREACH(reader, val) {
+			if (val->type != UJSON_STR) {
+				ujson_err(reader, "Expected string!");
+				return NULL;
+			}
+
+			if (!path) {
+				path = strdup(val->val_str);
+			} else if (!flags_set) {
+				if (!strcmp(val->val_str, "TCONF")) {
+					flags = TST_SR_TCONF;
+				} else if (!strcmp(val->val_str, "TBROK")) {
+					flags = TST_SR_TBROK;
+				} else if (!strcmp(val->val_str, "SKIP")) {
+					flags = TST_SR_SKIP;
+				} else {
+					ujson_err(reader, "Invalid flags!");
+					return NULL;
+				}
+
+				flags_set = 1;
+			} else {
+				ujson_err(reader, "Expected only two members!");
+				return NULL;
+			}
+		}
+
+		if (!path || !flags_set) {
+			ujson_err(reader, "Expected [\"/{proc,sys}/path\", {\"TCONF\", \"TBROK\", \"TSKIP\"}]!");
+			return NULL;
+		}
+
+		ret[i].path = path;
+		ret[i].val = NULL;
+		ret[i].flags = flags;
+		i++;
+	}
+
+	return ret;
+}
+
 static void parse_metadata(void)
 {
 	ujson_reader reader = UJSON_READER_INIT(metadata, metadata_used, UJSON_READER_STRICT);
@@ -385,6 +454,9 @@ static void parse_metadata(void)
 		case RESTORE_WALLCLOCK:
 			test.restore_wallclock = val.val_bool;
 		break;
+		case SAVE_RESTORE:
+			test.save_restore = parse_save_restore(&reader, &val);
+		break;
 		case SKIP_FILESYSTEMS:
 			test.skip_filesystems = parse_strarr(&reader, &val);
 		break;
-- 
2.45.2


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

[LTP] [PATCH 2/5] libs/ujson: Fix "Garbage after JSON string!" in strict mode

[Cyril Hrubis]

In strict mode warnings are converted into errors and filled into the
error buffer rather than being printed right away. That means that we
have to print error buffer after we issue a warning so that it's printed
in the strict mode (UJSON_READER_STRICT) as well.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 libs/ujson/ujson_reader.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libs/ujson/ujson_reader.c b/libs/ujson/ujson_reader.c
index d508f00d3..9f86f25b7 100644
--- a/libs/ujson/ujson_reader.c
+++ b/libs/ujson/ujson_reader.c
@@ -1049,8 +1049,12 @@ void ujson_reader_finish(ujson_reader *self)
 {
 	if (ujson_reader_err(self))
 		ujson_err_print(self);
-	else if (!ujson_reader_consumed(self))
+	else if (!ujson_reader_consumed(self)) {
 		ujson_warn(self, "Garbage after JSON string!");
+
+		if (ujson_reader_err(self))
+			ujson_err_print(self);
+	}
 }
 
 void ujson_reader_free(ujson_reader *buf)
-- 
2.45.2


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

[LTP] [PATCH 3/5] tst_run_shell: Better errors for metadata extractor

[Cyril Hrubis]

- Add filename and lineno to error messages

- Make sure that we are stil in the comment part and TBROK if any of the
  block wasn't terminated properly

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/lib/tst_run_shell.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/testcases/lib/tst_run_shell.c b/testcases/lib/tst_run_shell.c
index 836e01f18..24172c877 100644
--- a/testcases/lib/tst_run_shell.c
+++ b/testcases/lib/tst_run_shell.c
@@ -506,6 +506,7 @@ static void extract_metadata(void)
 	char line[4096];
 	char path[4096];
 	enum parser_state state = PAR_NONE;
+	unsigned int lineno = 1;
 
 	if (tst_get_path(shell_filename, path, sizeof(path)) == -1)
 		tst_brk(TBROK, "Failed to find %s in $PATH", shell_filename);
@@ -519,24 +520,40 @@ static void extract_metadata(void)
 				state = PAR_ESC;
 		break;
 		case PAR_ESC:
-			if (!strcmp(line, "# env\n"))
+			if (!strcmp(line, "# env\n")) {
 				state = PAR_ENV;
-			else if (!strcmp(line, "# doc\n"))
+			} else if (!strcmp(line, "# doc\n")) {
 				state = PAR_DOC;
-			else
-				tst_brk(TBROK, "Unknown comment block %s", line);
+			} else {
+				tst_brk(TBROK, "%s: %u: Unknown comment block %s",
+				        path, lineno, line);
+			}
 		break;
 		case PAR_ENV:
+			if (line[0] != '#') {
+				tst_brk(TBROK,
+					"%s: %u: Unexpected end of comment block!",
+					path, lineno);
+			}
+
 			if (!strcmp(line, "# ---\n"))
 				state = PAR_NONE;
 			else
 				metadata_append(line + 2);
 		break;
 		case PAR_DOC:
+			if (line[0] != '#') {
+				tst_brk(TBROK,
+					"%s: %u: Unexpected end of comment block!",
+					path, lineno);
+			}
+
 			if (!strcmp(line, "# ---\n"))
 				state = PAR_NONE;
 		break;
 		}
+
+		lineno++;
 	}
 
 	fclose(f);
-- 
2.45.2


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

[LTP] [PATCH 4/5] lib/tst_res_.c: Add TBROK handler + more verbose errors

[Cyril Hrubis]

We use the tst_res_ helper for tst_brk_ as well so we need to be able to
handle TBROK type as well.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/lib/tst_res_.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/testcases/lib/tst_res_.c b/testcases/lib/tst_res_.c
index a43920f36..fd9b8e841 100644
--- a/testcases/lib/tst_res_.c
+++ b/testcases/lib/tst_res_.c
@@ -8,28 +8,34 @@
 
 static void print_help(void)
 {
-	printf("Usage: tst_res_ filename lineno [TPASS|TFAIL|TCONF|TINFO|TDEBUG] 'A short description'\n");
+	printf("Usage: tst_{res,brk} filename lineno [TPASS|TBROK|TFAIL|TCONF|TINFO|TDEBUG] 'A short description'\n");
 }
 
 int main(int argc, char *argv[])
 {
 	int type, i;
 
-	if (argc < 5)
+	if (argc < 5) {
+		printf("argc = %i expected 5\n", argc);
 		goto help;
+	}
 
-	if (!strcmp(argv[3], "TPASS"))
+	if (!strcmp(argv[3], "TPASS")) {
 		type = TPASS;
-	else if (!strcmp(argv[3], "TFAIL"))
+	} else if (!strcmp(argv[3], "TFAIL")) {
 		type = TFAIL;
-	else if (!strcmp(argv[3], "TCONF"))
+	} else if (!strcmp(argv[3], "TCONF")) {
 		type = TCONF;
-	else if (!strcmp(argv[3], "TINFO"))
+	} else if (!strcmp(argv[3], "TINFO")) {
 		type = TINFO;
-	else if (!strcmp(argv[3], "TDEBUG"))
+	} else if (!strcmp(argv[3], "TDEBUG")) {
 		type = TDEBUG;
-	else
+	} else if (!strcmp(argv[3], "TBROK")) {
+		type = TBROK;
+	} else {
+		printf("Wrong type '%s'\n", argv[3]);
 		goto help;
+	}
 
 	size_t len = 0;
 
-- 
2.45.2


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

[LTP] [PATCH 5/5] mem/vma05.sh: Convert to the new shell library

[Cyril Hrubis]

PATH=$PATH:$PWD/../../../lib/:$PWD/testcases/lib/:: ./vma05.sh

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/mem/vma/vma05.sh | 97 ++++++++++++++++---------------
 1 file changed, 50 insertions(+), 47 deletions(-)

diff --git a/testcases/kernel/mem/vma/vma05.sh b/testcases/kernel/mem/vma/vma05.sh
index e1ef1014e..908a05850 100755
--- a/testcases/kernel/mem/vma/vma05.sh
+++ b/testcases/kernel/mem/vma/vma05.sh
@@ -1,6 +1,15 @@
 #!/bin/sh
+#
 # SPDX-License-Identifier: GPL-2.0-or-later
+#
 # Copyright (C) 2017 Red Hat, Inc.
+# Copyright (C) 2024 Cyril Hrubis <chrubis@suse.cz>
+#
+# ---
+# doc
+#
+# [Description]
+#
 # Regression test if the vsyscall and vdso VMA regions are reported correctly.
 #
 # While [vsyscall] is mostly deprecated with newer systems, there is
@@ -15,58 +24,52 @@
 # VM_ALWAYSDUMP)). As a consequence of this bug, VMAs were not included
 # in core dumps which resulted in eg. incomplete backtraces and invalid
 # core dump files created by gdb.
+# ---
+#
+# ---
+# env
+# {
+#  "needs_root": true,
+#  "needs_tmpdir": true,
+#  "needs_cmds": ["gdb"],
+#  "save_restore": [
+#   ["/proc/sys/kernel/core_pattern", "TBROK"],
+#   ["/proc/sys/kernel/core_uses_pid", "TBROK"]
+#  ],
+#  "tags": [
+#   ["linux-git", "103efcd9aac1"],
+#   ["linux-git", "b6558c4a2378"],
+#   ["linux-git", "e5b97dde514f"]
+#  ]
+# }
+# ---
 
-TST_SETUP=setup
-TST_CLEANUP=cleanup
-TST_TESTFUNC=vma_report_check
-TST_NEEDS_ROOT=1
-TST_NEEDS_TMPDIR=1
-TST_NEEDS_CMDS="gdb"
-
-CORE_LIMIT=$(ulimit -c)
-CORE_PATTERN=$(cat /proc/sys/kernel/core_pattern)
-CORE_USES_PID=$(cat /proc/sys/kernel/core_uses_pid)
-
-setup()
-{
-	ulimit -c unlimited
-	echo "core" > /proc/sys/kernel/core_pattern
-	echo 0 > /proc/sys/kernel/core_uses_pid
-	unset DEBUGINFOD_URLS
-}
+. tst_loader.sh
 
-cleanup()
-{
-	ulimit -c "$CORE_LIMIT"
-	echo "$CORE_PATTERN" > /proc/sys/kernel/core_pattern
-	echo $CORE_USES_PID > /proc/sys/kernel/core_uses_pid
-}
+ulimit -c unlimited
+echo "core" > /proc/sys/kernel/core_pattern
+echo 0 > /proc/sys/kernel/core_uses_pid
 
-vma_report_check()
-{
-	if [ $(uname -m) = "x86_64" ]; then
-		if LINE=$(grep "vsyscall" /proc/self/maps); then
-			RIGHT="ffffffffff600000-ffffffffff601000[[:space:]][r-]-xp"
-			if echo "$LINE" | grep -q "$RIGHT"; then
-				tst_res TPASS "[vsyscall] reported correctly"
-			else
-				tst_res TFAIL "[vsyscall] reporting wrong"
-			fi
+if [ $(uname -m) = "x86_64" ]; then
+	if LINE=$(grep "vsyscall" /proc/self/maps); then
+		RIGHT="ffffffffff600000-ffffffffff601000[[:space:]][r-]-xp"
+		if echo "$LINE" | grep -q "$RIGHT"; then
+			tst_res TPASS "[vsyscall] reported correctly"
+		else
+			tst_res TFAIL "[vsyscall] reporting wrong"
 		fi
 	fi
+fi
 
-	rm -rf core*
-	{ vma05_vdso; } > /dev/null 2>&1
-	[ -f core ] || tst_brk TBROK "missing core file"
+rm -rf core*
+{ vma05_vdso; } > /dev/null 2>&1
+[ -f core ] || tst_brk TBROK "missing core file"
 
-	TRACE=$(gdb -silent -ex="thread apply all backtrace" -ex="quit"\
-		vma05_vdso ./core* 2> /dev/null)
-	if echo "$TRACE" | grep -qF "??"; then
-		tst_res TFAIL "[vdso] bug not patched"
-	else
-		tst_res TPASS "[vdso] backtrace complete"
-	fi
-}
+TRACE=$(gdb -silent -ex="thread apply all backtrace" -ex="quit"\
+	vma05_vdso ./core* 2> /dev/null)
 
-. tst_test.sh
-tst_run
+if echo "$TRACE" | grep -qF "??"; then
+	tst_res TFAIL "[vdso] bug not patched"
+else
+	tst_res TPASS "[vdso] backtrace complete"
+fi
-- 
2.45.2


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

Re: [LTP] [PATCH 5/5] mem/vma05.sh: Convert to the new shell library

[Andrea Cervesato via ltp]

Hi!

On 12/3/24 16:15, Cyril Hrubis wrote:
> PATH=$PATH:$PWD/../../../lib/:$PWD/testcases/lib/:: ./vma05.sh
Is this an error? It doesn't look like a correct commit message.
> Signed-off-by: Cyril Hrubis<chrubis@suse.cz>
> ---
>   testcases/kernel/mem/vma/vma05.sh | 97 ++++++++++++++++---------------
>   1 file changed, 50 insertions(+), 47 deletions(-)
Andrea

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

Re: [LTP] [PATCH 5/5] mem/vma05.sh: Convert to the new shell library

[Andrea Cervesato via ltp]

Hi!

On 12/3/24 16:15, Cyril Hrubis wrote:
> PATH=$PATH:$PWD/../../../lib/:$PWD/testcases/lib/:: ./vma05.sh
Is this an error? It doesn't look like a correct commit message.
> Signed-off-by: Cyril Hrubis<chrubis@suse.cz>
> ---
>   testcases/kernel/mem/vma/vma05.sh | 97 ++++++++++++++++---------------
>   1 file changed, 50 insertions(+), 47 deletions(-)
Andrea

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

Re: [LTP] [PATCH 5/5] mem/vma05.sh: Convert to the new shell library

[Cyril Hrubis]

Hi!
> > PATH=$PATH:$PWD/../../../lib/:$PWD/testcases/lib/:: ./vma05.sh
> Is this an error? It doesn't look like a correct commit message.

Ah, I forget to add "To test the testcase run:" or something along the
lines.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 5/5] mem/vma05.sh: Convert to the new shell library

[Petr Vorel]

Hi Cyril,

> +# env
> +# {
> +#  "needs_root": true,
> +#  "needs_tmpdir": true,
> +#  "needs_cmds": ["gdb"],
> +#  "save_restore": [
> +#   ["/proc/sys/kernel/core_pattern", "TBROK"],
> +#   ["/proc/sys/kernel/core_uses_pid", "TBROK"]
C API .save_restore has 3 members:

struct tst_path_val {
        const char *path;
        const char *val;
	unsigned int flags;
};

Why don't you use it here? (e.g. NULL).

Kind regards,
Petr

> +#  ],
> +#  "tags": [
> +#   ["linux-git", "103efcd9aac1"],
> +#   ["linux-git", "b6558c4a2378"],
> +#   ["linux-git", "e5b97dde514f"]
> +#  ]
> +# }
> +# ---

> -TST_SETUP=setup
> -TST_CLEANUP=cleanup
> -TST_TESTFUNC=vma_report_check
> -TST_NEEDS_ROOT=1
> -TST_NEEDS_TMPDIR=1
> -TST_NEEDS_CMDS="gdb"
> -
> -CORE_LIMIT=$(ulimit -c)
> -CORE_PATTERN=$(cat /proc/sys/kernel/core_pattern)
> -CORE_USES_PID=$(cat /proc/sys/kernel/core_uses_pid)
> -
> -setup()
> -{
> -	ulimit -c unlimited
> -	echo "core" > /proc/sys/kernel/core_pattern
> -	echo 0 > /proc/sys/kernel/core_uses_pid
> -	unset DEBUGINFOD_URLS
> -}
> +. tst_loader.sh

> -cleanup()
> -{
> -	ulimit -c "$CORE_LIMIT"
> -	echo "$CORE_PATTERN" > /proc/sys/kernel/core_pattern
> -	echo $CORE_USES_PID > /proc/sys/kernel/core_uses_pid
> -}
> +ulimit -c unlimited
> +echo "core" > /proc/sys/kernel/core_pattern
> +echo 0 > /proc/sys/kernel/core_uses_pid

> -vma_report_check()
> -{
> -	if [ $(uname -m) = "x86_64" ]; then
> -		if LINE=$(grep "vsyscall" /proc/self/maps); then
> -			RIGHT="ffffffffff600000-ffffffffff601000[[:space:]][r-]-xp"
> -			if echo "$LINE" | grep -q "$RIGHT"; then
> -				tst_res TPASS "[vsyscall] reported correctly"
> -			else
> -				tst_res TFAIL "[vsyscall] reporting wrong"
> -			fi
> +if [ $(uname -m) = "x86_64" ]; then
> +	if LINE=$(grep "vsyscall" /proc/self/maps); then
> +		RIGHT="ffffffffff600000-ffffffffff601000[[:space:]][r-]-xp"
> +		if echo "$LINE" | grep -q "$RIGHT"; then
> +			tst_res TPASS "[vsyscall] reported correctly"
> +		else
> +			tst_res TFAIL "[vsyscall] reporting wrong"
>  		fi
>  	fi
> +fi

> -	rm -rf core*
> -	{ vma05_vdso; } > /dev/null 2>&1
> -	[ -f core ] || tst_brk TBROK "missing core file"
> +rm -rf core*
> +{ vma05_vdso; } > /dev/null 2>&1
> +[ -f core ] || tst_brk TBROK "missing core file"

> -	TRACE=$(gdb -silent -ex="thread apply all backtrace" -ex="quit"\
> -		vma05_vdso ./core* 2> /dev/null)
> -	if echo "$TRACE" | grep -qF "??"; then
> -		tst_res TFAIL "[vdso] bug not patched"
> -	else
> -		tst_res TPASS "[vdso] backtrace complete"
> -	fi
> -}
> +TRACE=$(gdb -silent -ex="thread apply all backtrace" -ex="quit"\
> +	vma05_vdso ./core* 2> /dev/null)

> -. tst_test.sh
> -tst_run
> +if echo "$TRACE" | grep -qF "??"; then
> +	tst_res TFAIL "[vdso] bug not patched"
> +else
> +	tst_res TPASS "[vdso] backtrace complete"
> +fi

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

Re: [LTP] [PATCH 1/5] tst_run_shell: Add save_restore parser

[Petr Vorel]

Hi Cyril,

...
> +		UJSON_ARR_FOREACH(reader, val) {
> +			if (val->type != UJSON_STR) {
> +				ujson_err(reader, "Expected string!");
> +				return NULL;
> +			}
> +
> +			if (!path) {
> +				path = strdup(val->val_str);
> +			} else if (!flags_set) {
> +				if (!strcmp(val->val_str, "TCONF")) {
> +					flags = TST_SR_TCONF;
> +				} else if (!strcmp(val->val_str, "TBROK")) {
> +					flags = TST_SR_TBROK;
> +				} else if (!strcmp(val->val_str, "SKIP")) {
> +					flags = TST_SR_SKIP;
> +				} else {
> +					ujson_err(reader, "Invalid flags!");
> +					return NULL;
> +				}
> +
> +				flags_set = 1;
> +			} else {
> +				ujson_err(reader, "Expected only two members!");

As I noted at vma05.sh, C API supports 3 members (also value).
Maybe vma05.sh does not need it, but wouldn't be better to support it?

Otherwise LGTM.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Also, while you at it:

-struct tst_test test = {
+static struct tst_test test = {


Kind regards,
Petr

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

Re: [LTP] [PATCH 5/5] mem/vma05.sh: Convert to the new shell library

[Petr Vorel]

Hi Cyril,

> +# env
> +# {
> +#  "needs_root": true,
> +#  "needs_tmpdir": true,
> +#  "needs_cmds": ["gdb"],
Maybe to add here also "uname" ?

> +#  "save_restore": [
> +#   ["/proc/sys/kernel/core_pattern", "TBROK"],
> +#   ["/proc/sys/kernel/core_uses_pid", "TBROK"]
> +#  ],
> +#  "tags": [
> +#   ["linux-git", "103efcd9aac1"],
> +#   ["linux-git", "b6558c4a2378"],
> +#   ["linux-git", "e5b97dde514f"]
> +#  ]
> +# }
> +# ---

...
> -vma_report_check()
> -{
> -	if [ $(uname -m) = "x86_64" ]; then
> -		if LINE=$(grep "vsyscall" /proc/self/maps); then
> -			RIGHT="ffffffffff600000-ffffffffff601000[[:space:]][r-]-xp"
> -			if echo "$LINE" | grep -q "$RIGHT"; then
> -				tst_res TPASS "[vsyscall] reported correctly"
> -			else
> -				tst_res TFAIL "[vsyscall] reporting wrong"
> -			fi
> +if [ $(uname -m) = "x86_64" ]; then
> +	if LINE=$(grep "vsyscall" /proc/self/maps); then
> +		RIGHT="ffffffffff600000-ffffffffff601000[[:space:]][r-]-xp"
> +		if echo "$LINE" | grep -q "$RIGHT"; then
> +			tst_res TPASS "[vsyscall] reported correctly"
> +		else
> +			tst_res TFAIL "[vsyscall] reporting wrong"
>  		fi
>  	fi
> +fi

> -	rm -rf core*
> -	{ vma05_vdso; } > /dev/null 2>&1
> -	[ -f core ] || tst_brk TBROK "missing core file"
> +rm -rf core*
> +{ vma05_vdso; } > /dev/null 2>&1
> +[ -f core ] || tst_brk TBROK "missing core file"

Test timeouts for me:
# PATH=.:~/ltp.git/testcases/lib/:$PATH vma05.sh
tst_tmpdir.c:316: TINFO: Using /tmp/LTP_vmaN8CQtx as tmpdir (tmpfs filesystem)
tst_test.c:1890: TINFO: LTP version: 20240930-113-gffde06520
tst_test.c:1894: TINFO: Tested kernel: 6.13.0-rc1-1.g492f944-default #1 SMP PREEMPT_DYNAMIC Mon Dec  2 08:55:00 UTC 2024 (492f944) x86_64
tst_test.c:1725: TINFO: Timeout per run is 0h 00m 30s
vma05.sh:57: TPASS: [vsyscall] reported correctly

Test timeouted, sending SIGKILL!
tst_test.c:1775: TINFO: Killed the leftover descendant processes
tst_test.c:1781: TINFO: If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1
tst_test.c:1783: TBROK: Test killed! (timeout?)

HINT: You _MAY_ be missing kernel fixes:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=103efcd9aac1
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b6558c4a2378
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e5b97dde514f

Summary:
passed   1
failed   0
broken   1
skipped  0
warnings 0

But it fails on master, therefore problem on the system I test. Although on
master it was more obvious that the problem is due core dump file not being
created. In new version it just timeouts.

PATH=.:~/ltp.git/testcases/lib/:$PATH LTPROOT=$LTPROOT vma05.sh
vma05 1 TINFO: Running: vma05.sh
vma05 1 TINFO: Tested kernel: Linux ts 6.13.0-rc1-1.g492f944-default #1 SMP PREEMPT_DYNAMIC Mon Dec  2 08:55:00 UTC 2024 (492f944) x86_64 x86_64 x86_64 GNU/Linux
vma05 1 TINFO: Using /tmp/LTP_vma05.I8VXfhyFt6 as tmpdir (tmpfs filesystem)
vma05 1 TINFO: timeout per run is 0h 5m 0s
vma05 1 TPASS: [vsyscall] reported correctly
vma05 1 TBROK: missing core file
vma05 1 TINFO: AppArmor enabled, this may affect test results
vma05 1 TINFO: it can be disabled with TST_DISABLE_APPARMOR=1 (requires super/root)
vma05 1 TINFO: loaded AppArmor profiles: none

Summary:
passed   1
failed   0
broken   1
skipped  0
warnings 0

Kind regards,
Petr

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

Re: [LTP] [PATCH 3/5] tst_run_shell: Better errors for metadata extractor

[Petr Vorel]

Hi Cyril,

> - Add filename and lineno to error messages

> - Make sure that we are stil in the comment part and TBROK if any of the
nit: s/stil/still/

>   block wasn't terminated properly

good improvement, thanks!
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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

Re: [LTP] [PATCH 4/5] lib/tst_res_.c: Add TBROK handler + more verbose errors

[Petr Vorel]

> We use the tst_res_ helper for tst_brk_ as well so we need to be able to
> handle TBROK type as well.

How can we call tst_brk_() via tst_res_() ?
	tst_res_(argv[1], atoi(argv[2]), type, "%s", msg);

Also we have TST_RES_SUPPORTS_TCONF_TDEBUG_TFAIL_TINFO_TPASS_TWARN check to not
add TBROK to tst_res_().

...
> -	else if (!strcmp(argv[3], "TINFO"))
> +	} else if (!strcmp(argv[3], "TINFO")) {
>  		type = TINFO;
> -	else if (!strcmp(argv[3], "TDEBUG"))
> +	} else if (!strcmp(argv[3], "TDEBUG")) {
>  		type = TDEBUG;
> -	else
> +	} else if (!strcmp(argv[3], "TBROK")) {
> +		type = TBROK;
> +	} else {
> +		printf("Wrong type '%s'\n", argv[3]);
>  		goto help;

Also tst_brk allows only TBROK and TCONF in C:

#define tst_brk(ttype, arg_fmt, ...)						\
	({									\
		TST_BRK_SUPPORTS_ONLY_TCONF_TBROK(!((ttype) &			\
			(TBROK | TCONF | TFAIL)));				\
		tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
	})

... and shell:

	if [ "$res" != TBROK -a "$res" != TCONF ]; then
		tst_res TBROK "tst_brk can be called only with TBROK or TCONF ($res)"
	else
		tst_res "$res" "$@"
	fi

Kind regards,
Petr

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

Re: [LTP] [PATCH 0/5] First new shell library converted test

[Andrea Cervesato via ltp]

Hi Cyril,

I had time to take a look at it closer and it looks really promising.
Thanks for this support, it simplifies tests quite a lot.

Andrea

On 12/3/24 16:15, Cyril Hrubis wrote:
> This patchset contains a few fixes for the new shell library and first
> converted test.
>
> Cyril Hrubis (5):
>    tst_run_shell: Add save_restore parser
>    libs/ujson: Fix "Garbage after JSON string!" in strict mode
>    tst_run_shell: Better errors for metadata extractor
>    lib/tst_res_.c: Add TBROK handler + more verbose errors
>    mem/vma05.sh: Convert to the new shell library
>
>   libs/ujson/ujson_reader.c         |  6 +-
>   testcases/kernel/mem/vma/vma05.sh | 97 ++++++++++++++++---------------
>   testcases/lib/tst_res_.c          | 22 ++++---
>   testcases/lib/tst_run_shell.c     | 97 +++++++++++++++++++++++++++++--
>   4 files changed, 162 insertions(+), 60 deletions(-)
>

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

Re: [LTP] [PATCH 4/5] lib/tst_res_.c: Add TBROK handler + more verbose errors

[Cyril Hrubis]

Hi!
> > We use the tst_res_ helper for tst_brk_ as well so we need to be able to
> > handle TBROK type as well.
> 
> How can we call tst_brk_() via tst_res_() ?
> 	tst_res_(argv[1], atoi(argv[2]), type, "%s", msg);

In the end both of these functions increment counters, but in this case
we need to return to the shell so we cannot call tst_brk() in the
helper. It's a very special situation here.

> Also we have TST_RES_SUPPORTS_TCONF_TDEBUG_TFAIL_TINFO_TPASS_TWARN check to not
> add TBROK to tst_res_().

That only works when the value is constant, if you pass via variable
that is not constant at build time you can pass whatever you want. Which
is another reason why it makes sense to relax the constraints.

> ...
> > -	else if (!strcmp(argv[3], "TINFO"))
> > +	} else if (!strcmp(argv[3], "TINFO")) {
> >  		type = TINFO;
> > -	else if (!strcmp(argv[3], "TDEBUG"))
> > +	} else if (!strcmp(argv[3], "TDEBUG")) {
> >  		type = TDEBUG;
> > -	else
> > +	} else if (!strcmp(argv[3], "TBROK")) {
> > +		type = TBROK;
> > +	} else {
> > +		printf("Wrong type '%s'\n", argv[3]);
> >  		goto help;
> 
> Also tst_brk allows only TBROK and TCONF in C:
> 
> #define tst_brk(ttype, arg_fmt, ...)						\
> 	({									\
> 		TST_BRK_SUPPORTS_ONLY_TCONF_TBROK(!((ttype) &			\
> 			(TBROK | TCONF | TFAIL)));				\
> 		tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
> 	})

See above.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 4/5] lib/tst_res_.c: Add TBROK handler + more verbose errors

[Petr Vorel]

Hi Cyril,

> Hi!
> > > We use the tst_res_ helper for tst_brk_ as well so we need to be able to
> > > handle TBROK type as well.

> > How can we call tst_brk_() via tst_res_() ?
> > 	tst_res_(argv[1], atoi(argv[2]), type, "%s", msg);

> In the end both of these functions increment counters, but in this case
> we need to return to the shell so we cannot call tst_brk() in the
> helper. It's a very special situation here.

Thanks for info. Maybe a little note anywhere (e.g. commit message) would help.

> > Also we have TST_RES_SUPPORTS_TCONF_TDEBUG_TFAIL_TINFO_TPASS_TWARN check to not
> > add TBROK to tst_res_().

> That only works when the value is constant, if you pass via variable
> that is not constant at build time you can pass whatever you want. Which
> is another reason why it makes sense to relax the constraints.

Good point. So you plan to remove these build time checks?

Also we have nice docs from you include/tst_res_flags.h.
ATM include/tst_test.h and doc/old/C-Test-API.asciidoc are outdated,
but if you relax allowed ttype, than it would not need to be updated.

Also, any reason to not support TWARN?

I suppose we don't need TERRNO, TTERRNO, TRERRNO (not supported by tst_test.sh).

Anyway, LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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

Re: [LTP] [PATCH 4/5] lib/tst_res_.c: Add TBROK handler + more verbose errors

[Cyril Hrubis]

Hi!
> > In the end both of these functions increment counters, but in this case
> > we need to return to the shell so we cannot call tst_brk() in the
> > helper. It's a very special situation here.
> 
> Thanks for info. Maybe a little note anywhere (e.g. commit message) would help.

Will do.

> > > Also we have TST_RES_SUPPORTS_TCONF_TDEBUG_TFAIL_TINFO_TPASS_TWARN check to not
> > > add TBROK to tst_res_().
> 
> > That only works when the value is constant, if you pass via variable
> > that is not constant at build time you can pass whatever you want. Which
> > is another reason why it makes sense to relax the constraints.
> 
> Good point. So you plan to remove these build time checks?

For tst_brk() these checks are removed in my patchset that changes how
TBROK is propagated, so that tst_brk() works with TFAIL and TPASS as
well.

We also had a discussion about removing TWARN since there is not a big
difference between TBROK and TWARN and replacing TWARN with
tst_res(TBROK, ...) as that would make things simpler. Currently there
is no good rule where to use TBROK and where TWARN so removing TWARN
sounds like a good option.

> Also we have nice docs from you include/tst_res_flags.h.
> ATM include/tst_test.h and doc/old/C-Test-API.asciidoc are outdated,
> but if you relax allowed ttype, than it would not need to be updated.
> 
> Also, any reason to not support TWARN?

See above.

> I suppose we don't need TERRNO, TTERRNO, TRERRNO (not supported by tst_test.sh).

Yes, errno is not defined in shell.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 2/5] libs/ujson: Fix "Garbage after JSON string!" in strict mode

[Petr Vorel]

Hi Cyril,

> In strict mode warnings are converted into errors and filled into the
> error buffer rather than being printed right away. That means that we
> have to print error buffer after we issue a warning so that it's printed
> in the strict mode (UJSON_READER_STRICT) as well.

Acked-by: Petr Vorel <pvorel@suse.cz>

nit: maybe add Fixes: before merge?

Kind regards,
Petr

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

Re: [LTP] [PATCH 5/5] mem/vma05.sh: Convert to the new shell library

[Cyril Hrubis]

Hi!
> > +# env
> > +# {
> > +#  "needs_root": true,
> > +#  "needs_tmpdir": true,
> > +#  "needs_cmds": ["gdb"],
> > +#  "save_restore": [
> > +#   ["/proc/sys/kernel/core_pattern", "TBROK"],
> > +#   ["/proc/sys/kernel/core_uses_pid", "TBROK"]
> C API .save_restore has 3 members:
> 
> struct tst_path_val {
>         const char *path;
>         const char *val;
> 	unsigned int flags;
> };
> 
> Why don't you use it here? (e.g. NULL).

I did look at the parsed metadata in the ltp.json to check if we match
the format there and it seems that the parsing is broken, so there is no
point in designing the interface before we fix the C parser I guess.

This is there for the ksm01:

   "save_restore": [
     [
      "/sys/kernel/mm/ksm/run",
      "TST_SR_TBROK"
     ],
     [
      "/sys/kernel/mm/ksm/sleep_millisecs",
      "TST_SR_TBROK"
     ],
     [
      "/sys/kernel/mm/ksm/max_page_sharing",
      "TST_SR_SKIP_MISSING",
      "TST_SR_TCONF_RO"
     ],
     [
      "/sys/kernel/mm/ksm/merge_across_nodes",
      "1",
      "TST_SR_SKIP_MISSING",
      "TST_SR_TCONF_RO"
     ],
     [
      "/sys/kernel/mm/ksm/smart_scan",
      "0",
      "TST_SR_SKIP_MISSING",
      "TST_SR_TBROK_RO"
     ]
    ],

This is because the anonymous structures are parsed by the array parsing
code in the metaparse.c which is overly simplistics. And there are more
bugs in there unfortunately. I can fix the most pressing problems there:

- missing macro expansion
- missing NULL in the middle of intiliaziation
- properly concatenate entries
- limited recursive #include directive

Which will produce much saner output, but we will likely never be able
to produce clean enough output without using a proper tooling that can
do compile time arithmetics. Yes we have things like .needs_hugepages
with number of hugepages defined as (5 + 1) * 5 after a macro expansion.
Which ends up in the JSON as "(ARSZ + 1) * LOOP" which ends up as
"(50 + 1)*5" after this patch. Which is stil way better than the mess we
had there before.


Anyways, I will send a patch for the metaparse.c tomorrow, so that we
can fix that first.

-- 
Cyril Hrubis
chrubis@suse.cz

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