[LTP] [PATCH 1/2] lib: add cmd parse handler in needs_cmd

[LTP] [PATCH 1/2] lib: add cmd parse handler in needs_cmd

[Yang Xu]

Testcase ie statx05 needs mkfs.ext4 >= 1.43.0 because of encrypt feature.

As Cyril suggested, add cmd parser handler in needs_cmd.

The difference logic about from before, we don't report not-found error and think it is
cmd version string(need to use tst_version_parser) if tst_get_path fails in tst_test.c.

In tst_version_parser function, use strtok_r to split cmd_token,op_token,version_token.
It only supports six operations '>=' '<=' '>' '<' '==' '!='.

Currently, this tst_version_parser only supports mkfs.ext4 command. If you want to support
more commands, just add your own .parser and .table_get methond in version_parsers structure.

Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 doc/c-test-api.txt                       |  14 +++
 include/tst_test.h                       |   1 +
 include/tst_version_parser.h             |  15 +++
 lib/newlib_tests/.gitignore              |   5 +
 lib/newlib_tests/test_version_parser01.c |  25 ++++
 lib/newlib_tests/test_version_parser02.c |  24 ++++
 lib/newlib_tests/test_version_parser03.c |  24 ++++
 lib/newlib_tests/test_version_parser04.c |  24 ++++
 lib/newlib_tests/test_version_parser05.c |  24 ++++
 lib/tst_test.c                           |   2 +-
 lib/tst_version_parser.c                 | 148 +++++++++++++++++++++++
 11 files changed, 305 insertions(+), 1 deletion(-)
 create mode 100644 include/tst_version_parser.h
 create mode 100644 lib/newlib_tests/test_version_parser01.c
 create mode 100644 lib/newlib_tests/test_version_parser02.c
 create mode 100644 lib/newlib_tests/test_version_parser03.c
 create mode 100644 lib/newlib_tests/test_version_parser04.c
 create mode 100644 lib/newlib_tests/test_version_parser05.c
 create mode 100644 lib/tst_version_parser.c

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 64d0630ce..fde6dce0c 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -2013,6 +2013,20 @@ terminated array of strings such as:
 },
 -------------------------------------------------------------------------------
 
+Also can check required commands version whether is satisfied by using 'needs_cmds',
+
+[source,c]
+-------------------------------------------------------------------------------
+.needs_cmds = (const char *const []) {
+	"mkfs.ext4 >= 1.43.0",
+	NULL
+},
++-------------------------------------------------------------------------------
+
+Currently, we only support mkfs.ext4 command. If you want to support more commands,
+please fill your own .parser and .table_get method in the version_parsers structure
+of lib/tst_version_parser.c.
+
 1.36 Assert sys or proc file value
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 Using TST_ASSERT_INT/STR(path, val) to assert that integer value or string stored in
diff --git a/include/tst_test.h b/include/tst_test.h
index c06a4729b..fd3d4cfee 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -44,6 +44,7 @@
 #include "tst_taint.h"
 #include "tst_memutils.h"
 #include "tst_arch.h"
+#include "tst_version_parser.h"
 
 /*
  * Reports testcase result.
diff --git a/include/tst_version_parser.h b/include/tst_version_parser.h
new file mode 100644
index 000000000..145043929
--- /dev/null
+++ b/include/tst_version_parser.h
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+#ifndef TST_VERSION_PARSER_H__
+#define TST_VERSION_PARSER_H__
+
+/*
+ * Parse the cmd version requirement in needs_cmds member of tst_test
+ * structure whether is satisfied.
+ */
+void tst_version_parser(const char *cmd);
+
+#endif /* TST_VERSION_PARSER_H__ */
diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
index cf467b5a0..ef8694d08 100644
--- a/lib/newlib_tests/.gitignore
+++ b/lib/newlib_tests/.gitignore
@@ -46,4 +46,9 @@ test_macros06
 tst_fuzzy_sync01
 tst_fuzzy_sync02
 tst_fuzzy_sync03
+test_version_parser01
+test_version_parser02
+test_version_parser03
+test_version_parser04
+test_version_parser05
 test_zero_hugepage
diff --git a/lib/newlib_tests/test_version_parser01.c b/lib/newlib_tests/test_version_parser01.c
new file mode 100644
index 000000000..9e1bd3962
--- /dev/null
+++ b/lib/newlib_tests/test_version_parser01.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TPASS, "Tesing tst_version_parser() functionality OK.");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4",
+		"mkfs.ext4 >= 1.43.0",
+		"mkfs.ext4 <= 2.0.0",
+		"mkfs.ext4 != 2.0.0",
+		"mkfs.ext4 > 1.43.0",
+		"mkfs.ext4 < 2.0.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_version_parser02.c b/lib/newlib_tests/test_version_parser02.c
new file mode 100644
index 000000000..5aedaa28f
--- /dev/null
+++ b/lib/newlib_tests/test_version_parser02.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using Illegal cmd.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext45 >= 1.43.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_version_parser03.c b/lib/newlib_tests/test_version_parser03.c
new file mode 100644
index 000000000..8f96e68d2
--- /dev/null
+++ b/lib/newlib_tests/test_version_parser03.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using Illegal operation.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 ! 1.43.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_version_parser04.c b/lib/newlib_tests/test_version_parser04.c
new file mode 100644
index 000000000..461f673df
--- /dev/null
+++ b/lib/newlib_tests/test_version_parser04.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using Illegal version.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 > 1.43",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_version_parser05.c b/lib/newlib_tests/test_version_parser05.c
new file mode 100644
index 000000000..1bfe24f73
--- /dev/null
+++ b/lib/newlib_tests/test_version_parser05.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test non-existed cmd whether still can be detected.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext45",
+		NULL
+	}
+};
diff --git a/lib/tst_test.c b/lib/tst_test.c
index a79275722..a9e95a3d7 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -992,7 +992,7 @@ static void do_setup(int argc, char *argv[])
 
 		for (i = 0; (cmd = tst_test->needs_cmds[i]); ++i)
 			if (tst_get_path(cmd, path, sizeof(path)))
-				tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd);
+				tst_version_parser(cmd);
 	}
 
 	if (tst_test->needs_drivers) {
diff --git a/lib/tst_version_parser.c b/lib/tst_version_parser.c
new file mode 100644
index 000000000..296e25ea2
--- /dev/null
+++ b/lib/tst_version_parser.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+#define TST_NO_DEFAULT_MAIN
+#include "tst_version_parser.h"
+#include "tst_test.h"
+
+static int mkfs_ext4_version_parser(void)
+{
+	FILE *f;
+	int rc, major, minor, patch;
+
+	f = popen("mkfs.ext4 -V 2>&1", "r");
+	if (!f) {
+		tst_res(TWARN, "Could not run mkfs.ext4 -V 2>&1 cmd");
+		return -1;
+	}
+	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
+	pclose(f);
+	if (rc != 3) {
+		tst_res(TWARN, "Unable to parse mkfs.ext4 version");
+		return -1;
+	}
+
+	return major * 10000 +  minor * 100 + patch;
+}
+
+static int mkfs_ext4_version_table_get(char *version)
+{
+	char *major, *minor, *patch, *next, *str;
+	char table_version[100];
+
+	strcpy(table_version, version);
+
+	major = strtok_r(table_version, ".", &next);
+	minor = strtok_r(NULL, ".", &next);
+	patch = strtok_r(NULL, ".", &next);
+	str = strtok_r(NULL, ".", &next);
+	if (!major || !minor || !patch || str) {
+		tst_res(TWARN, "Illegal version(%s), should use format like 1.43.0", version);
+		return -1;
+	}
+
+	return atoi(major) * 10000 + atoi(minor) * 100 + atoi(patch);
+}
+
+static struct version_parser {
+	const char *cmd;
+	int (*parser)(void);
+	int (*table_get)(char *version);
+} version_parsers[] = {
+	{.cmd = "mkfs.ext4", .parser = mkfs_ext4_version_parser, .table_get = mkfs_ext4_version_table_get},
+	{},
+};
+
+void tst_version_parser(const char *cmd)
+{
+	struct version_parser *p;
+	char *cmd_token, *op, *version, *next, *str;
+	char path[PATH_MAX];
+	char parser_cmd[100];
+	int ver_parser, ver_get;
+	int op_flag = 0;
+
+	strcpy(parser_cmd, cmd);
+
+	cmd_token = strtok_r(parser_cmd, " ", &next);
+	op = strtok_r(NULL, " ", &next);
+	version = strtok_r(NULL, " ", &next);
+	str = strtok_r(NULL, " ", &next);
+	if (!cmd_token || !op || !version || str)
+		tst_brk(TCONF,
+			"Illegal fomart(%s), should use format like mkfs.ext4 >= 1.43.0", cmd);
+
+	if (tst_get_path(cmd_token, path, sizeof(path)))
+		tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd_token);
+
+	if (!strcmp(op, ">="))
+		op_flag = 1;
+
+	if (!strcmp(op, ">"))
+		op_flag = 2;
+
+	if (!strcmp(op, "<="))
+		op_flag = 3;
+
+	if (!strcmp(op, "<"))
+		op_flag = 4;
+
+	if (!strcmp(op, "=="))
+		op_flag = 5;
+
+	if (!strcmp(op, "!="))
+		op_flag = 6;
+
+	if (!op_flag)
+		tst_brk(TCONF, "Invalid op(%s)", op);
+
+	for (p = &version_parsers[0]; p; p++) {
+		if (!strcmp(p->cmd, cmd_token))
+			tst_res(TINFO, "Parsing %s version", p->cmd);
+			break;
+	}
+
+	if (!p->cmd)
+		tst_brk(TBROK, "No version parser for %s implemented!", cmd_token);
+
+	ver_parser = p->parser();
+	if (ver_parser < 0)
+		tst_brk(TBROK, "Failed to parse %s version", p->cmd);
+
+	ver_get = p->table_get(version);
+	if (ver_get < 0)
+		tst_brk(TBROK, "Failed to get %s version", p->cmd);
+
+	switch (op_flag) {
+	case 1:
+		if (ver_parser < ver_get)
+			tst_brk(TCONF, "cmd(%s) expected >= %d, but got %d", cmd, ver_get, ver_parser);
+		break;
+	case 2:
+		if (ver_parser <= ver_get)
+			tst_brk(TCONF, "cmd(%s) expected > %d, but got %d", cmd, ver_get, ver_parser);
+		break;
+	case 3:
+		if (ver_parser > ver_get)
+			tst_brk(TCONF, "cmd(%s) expected <= %d, but got %d", cmd, ver_get, ver_parser);
+		break;
+	case 4:
+		if (ver_parser >= ver_get)
+			tst_brk(TCONF, "cmd(%s) expected < %d, but got %d", cmd, ver_get, ver_parser);
+		break;
+	case 5:
+		if (ver_parser != ver_get)
+			tst_brk(TCONF, "cmd(%s) expected == %d, but got %d", cmd, ver_get, ver_parser);
+		break;
+	case 6:
+		if (ver_parser == ver_get)
+			tst_brk(TCONF, "cmd(%s) expected != %d, but got %d", cmd, ver_get, ver_parser);
+		break;
+	}
+}
-- 
2.23.0


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

[LTP] [PATCH 2/2] testcase: make use of needs_cmd version parser

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 testcases/kernel/syscalls/quotactl/quotactl04.c | 11 +----------
 testcases/kernel/syscalls/statx/statx05.c       | 13 ++-----------
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/testcases/kernel/syscalls/quotactl/quotactl04.c b/testcases/kernel/syscalls/quotactl/quotactl04.c
index dab61cf4d..62c94d596 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl04.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl04.c
@@ -121,18 +121,9 @@ static void do_mount(const char *source, const char *target,
 
 static void setup(void)
 {
-	FILE *f;
 	const char *const fs_opts[] = {"-I 256", "-O quota,project", NULL};
-	int rc, major, minor, patch;
 
 	test_id = geteuid();
-	f = SAFE_POPEN("mkfs.ext4 -V 2>&1", "r");
-	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
-	if (rc != 3)
-		tst_res(TWARN, "Unable parse version number");
-	else if (major * 10000 + minor * 100 + patch < 14300)
-		tst_brk(TCONF, "Test needs mkfs.ext4 >= 1.43 for quota,project option, test skipped");
-	pclose(f);
 	SAFE_MKFS(tst_device->dev, tst_device->fs_type, fs_opts, NULL);
 	do_mount(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, NULL);
 }
@@ -184,7 +175,7 @@ static struct tst_test test = {
 	.dev_fs_type = "ext4",
 	.mntpoint = MNTPOINT,
 	.needs_cmds = (const char *[]) {
-		"mkfs.ext4",
+		"mkfs.ext4 >= 1.43.0",
 		NULL
 	}
 };
diff --git a/testcases/kernel/syscalls/statx/statx05.c b/testcases/kernel/syscalls/statx/statx05.c
index a948a30b0..6b9a1db59 100644
--- a/testcases/kernel/syscalls/statx/statx05.c
+++ b/testcases/kernel/syscalls/statx/statx05.c
@@ -85,18 +85,9 @@ static void run(unsigned int i)
 
 static void setup(void)
 {
-	FILE *f;
 	char opt_bsize[32];
 	const char *const fs_opts[] = {"-O encrypt", opt_bsize, NULL};
-	int ret, rc, major, minor, patch;
-
-	f = SAFE_POPEN("mkfs.ext4 -V 2>&1", "r");
-	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
-	if (rc != 3)
-		tst_res(TWARN, "Unable parse version number");
-	else if (major * 10000 + minor * 100 + patch < 14300)
-		tst_brk(TCONF, "Test needs mkfs.ext4 >= 1.43 for encrypt option, test skipped");
-	pclose(f);
+	int ret;
 
 	snprintf(opt_bsize, sizeof(opt_bsize), "-b %i", getpagesize());
 
@@ -129,7 +120,7 @@ static struct tst_test test = {
 	.mntpoint = MNTPOINT,
 	.dev_fs_type = "ext4",
 	.needs_cmds = (const char *[]) {
-		"mkfs.ext4",
+		"mkfs.ext4 >= 1.43.0",
 		"e4crypt",
 		NULL
 	}
-- 
2.23.0


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

Re: [LTP] [PATCH 2/2] testcase: make use of needs_cmd version parser

[Cyril Hrubis]

Hi!
This looks great!

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/2] lib: add cmd parse handler in needs_cmd

[Cyril Hrubis]

Hi!
> Testcase ie statx05 needs mkfs.ext4 >= 1.43.0 because of encrypt feature.
> 
> As Cyril suggested, add cmd parser handler in needs_cmd.
> 
> The difference logic about from before, we don't report not-found error and think it is
> cmd version string(need to use tst_version_parser) if tst_get_path fails in tst_test.c.
> 
> In tst_version_parser function, use strtok_r to split cmd_token,op_token,version_token.
> It only supports six operations '>=' '<=' '>' '<' '==' '!='.
> 
> Currently, this tst_version_parser only supports mkfs.ext4 command. If you want to support
> more commands, just add your own .parser and .table_get methond in version_parsers structure.
> 
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  doc/c-test-api.txt                       |  14 +++
>  include/tst_test.h                       |   1 +
>  include/tst_version_parser.h             |  15 +++
>  lib/newlib_tests/.gitignore              |   5 +
>  lib/newlib_tests/test_version_parser01.c |  25 ++++
>  lib/newlib_tests/test_version_parser02.c |  24 ++++
>  lib/newlib_tests/test_version_parser03.c |  24 ++++
>  lib/newlib_tests/test_version_parser04.c |  24 ++++
>  lib/newlib_tests/test_version_parser05.c |  24 ++++
>  lib/tst_test.c                           |   2 +-
>  lib/tst_version_parser.c                 | 148 +++++++++++++++++++++++
>  11 files changed, 305 insertions(+), 1 deletion(-)
>  create mode 100644 include/tst_version_parser.h
>  create mode 100644 lib/newlib_tests/test_version_parser01.c
>  create mode 100644 lib/newlib_tests/test_version_parser02.c
>  create mode 100644 lib/newlib_tests/test_version_parser03.c
>  create mode 100644 lib/newlib_tests/test_version_parser04.c
>  create mode 100644 lib/newlib_tests/test_version_parser05.c
>  create mode 100644 lib/tst_version_parser.c
> 
> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 64d0630ce..fde6dce0c 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -2013,6 +2013,20 @@ terminated array of strings such as:
>  },
>  -------------------------------------------------------------------------------
>  
> +Also can check required commands version whether is satisfied by using 'needs_cmds',
> +
> +[source,c]
> +-------------------------------------------------------------------------------
> +.needs_cmds = (const char *const []) {
> +	"mkfs.ext4 >= 1.43.0",
> +	NULL
> +},
> ++-------------------------------------------------------------------------------
> +
> +Currently, we only support mkfs.ext4 command. If you want to support more commands,
> +please fill your own .parser and .table_get method in the version_parsers structure
> +of lib/tst_version_parser.c.
> +
>  1.36 Assert sys or proc file value
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  Using TST_ASSERT_INT/STR(path, val) to assert that integer value or string stored in
> diff --git a/include/tst_test.h b/include/tst_test.h
> index c06a4729b..fd3d4cfee 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -44,6 +44,7 @@
>  #include "tst_taint.h"
>  #include "tst_memutils.h"
>  #include "tst_arch.h"
> +#include "tst_version_parser.h"

Since this is internal functionality that does not make sense to be
exposed to the tests there is no point in exposing this here, we should
include it in the tst_test.c instead.

>  /*
>   * Reports testcase result.
> diff --git a/include/tst_version_parser.h b/include/tst_version_parser.h
> new file mode 100644
> index 000000000..145043929
> --- /dev/null
> +++ b/include/tst_version_parser.h
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +#ifndef TST_VERSION_PARSER_H__
> +#define TST_VERSION_PARSER_H__
> +
> +/*
> + * Parse the cmd version requirement in needs_cmds member of tst_test
> + * structure whether is satisfied.
> + */
> +void tst_version_parser(const char *cmd);

This should probably be called tst_cmd_check() or something similar.

> +#endif /* TST_VERSION_PARSER_H__ */
> diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
> index cf467b5a0..ef8694d08 100644
> --- a/lib/newlib_tests/.gitignore
> +++ b/lib/newlib_tests/.gitignore
> @@ -46,4 +46,9 @@ test_macros06
>  tst_fuzzy_sync01
>  tst_fuzzy_sync02
>  tst_fuzzy_sync03
> +test_version_parser01
> +test_version_parser02
> +test_version_parser03
> +test_version_parser04
> +test_version_parser05
>  test_zero_hugepage
> diff --git a/lib/newlib_tests/test_version_parser01.c b/lib/newlib_tests/test_version_parser01.c
> new file mode 100644
> index 000000000..9e1bd3962
> --- /dev/null
> +++ b/lib/newlib_tests/test_version_parser01.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +#include <stdio.h>
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +	tst_res(TPASS, "Tesing tst_version_parser() functionality OK.");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_cmds = (const char *[]) {
> +		"mkfs.ext4",
> +		"mkfs.ext4 >= 1.43.0",
> +		"mkfs.ext4 <= 2.0.0",
> +		"mkfs.ext4 != 2.0.0",
> +		"mkfs.ext4 > 1.43.0",
> +		"mkfs.ext4 < 2.0.0",
> +		NULL
> +	}
> +};
> diff --git a/lib/newlib_tests/test_version_parser02.c b/lib/newlib_tests/test_version_parser02.c
> new file mode 100644
> index 000000000..5aedaa28f
> --- /dev/null
> +++ b/lib/newlib_tests/test_version_parser02.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +/*
> + * Test Illegal format by using Illegal cmd.
> + */
> +
> +#include <stdio.h>
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
                        ^
			"Nonexisting command is present!"


Or something along these line.

> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_cmds = (const char *[]) {
> +		"mkfs.ext45 >= 1.43.0",
> +		NULL
> +	}
> +};
> diff --git a/lib/newlib_tests/test_version_parser03.c b/lib/newlib_tests/test_version_parser03.c
> new file mode 100644
> index 000000000..8f96e68d2
> --- /dev/null
> +++ b/lib/newlib_tests/test_version_parser03.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +/*
> + * Test Illegal format by using Illegal operation.
> + */
> +
> +#include <stdio.h>
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
                          ^
			  "Wrong operator was evaluated!"

> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_cmds = (const char *[]) {
> +		"mkfs.ext4 ! 1.43.0",
> +		NULL
> +	}
> +};
> diff --git a/lib/newlib_tests/test_version_parser04.c b/lib/newlib_tests/test_version_parser04.c
> new file mode 100644
> index 000000000..461f673df
> --- /dev/null
> +++ b/lib/newlib_tests/test_version_parser04.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +/*
> + * Test Illegal format by using Illegal version.
> + */
> +
> +#include <stdio.h>
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
                          ^
			  "Incomplete version was parsed!"
> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_cmds = (const char *[]) {
> +		"mkfs.ext4 > 1.43",
> +		NULL
> +	}
> +};
> diff --git a/lib/newlib_tests/test_version_parser05.c b/lib/newlib_tests/test_version_parser05.c
> new file mode 100644
> index 000000000..1bfe24f73
> --- /dev/null
> +++ b/lib/newlib_tests/test_version_parser05.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +/*
> + * Test non-existed cmd whether still can be detected.
> + */
> +
> +#include <stdio.h>
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
                        ^
			"Nonexisting command is present!"
> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_cmds = (const char *[]) {
> +		"mkfs.ext45",
> +		NULL
> +	}
> +};
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index a79275722..a9e95a3d7 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -992,7 +992,7 @@ static void do_setup(int argc, char *argv[])
>  
>  		for (i = 0; (cmd = tst_test->needs_cmds[i]); ++i)
>  			if (tst_get_path(cmd, path, sizeof(path)))
> -				tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd);
> +				tst_version_parser(cmd);

Why do we keep the tst_get_path() here? We can as well get rid of that
and just call the function and handle the case without the version
there.

>  	}
>  
>  	if (tst_test->needs_drivers) {
> diff --git a/lib/tst_version_parser.c b/lib/tst_version_parser.c
> new file mode 100644
> index 000000000..296e25ea2
> --- /dev/null
> +++ b/lib/tst_version_parser.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_version_parser.h"
> +#include "tst_test.h"
> +
> +static int mkfs_ext4_version_parser(void)
> +{
> +	FILE *f;
> +	int rc, major, minor, patch;
> +
> +	f = popen("mkfs.ext4 -V 2>&1", "r");
> +	if (!f) {
> +		tst_res(TWARN, "Could not run mkfs.ext4 -V 2>&1 cmd");
> +		return -1;
> +	}
> +	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
> +	pclose(f);
> +	if (rc != 3) {
> +		tst_res(TWARN, "Unable to parse mkfs.ext4 version");
> +		return -1;
> +	}
> +
> +	return major * 10000 +  minor * 100 + patch;
> +}
> +
> +static int mkfs_ext4_version_table_get(char *version)
> +{
> +	char *major, *minor, *patch, *next, *str;
> +	char table_version[100];
> +
> +	strcpy(table_version, version);
> +
> +	major = strtok_r(table_version, ".", &next);
> +	minor = strtok_r(NULL, ".", &next);
> +	patch = strtok_r(NULL, ".", &next);
> +	str = strtok_r(NULL, ".", &next);

I wonder if we should do a sscanf() here. Note that with the atoi() we
will pass on strings that have garbage after the numbers such as:

	"1ab.2c.3dd"

What about:

	unsigned int maj, min, patch;
	int len;

	if (sscanf(version, "%u.%u.%u %n", &maj, &min, &patch, len) != 3) {
		tst_res(TWARN, "Illegal version ...");
		return -1;
	}

	if (len != strlen(version)) {
		tst_res(TWARN, "Garbage after version");
		return -1;
	}

	return maj * 10000 + min * 100 + patch;

> +	if (!major || !minor || !patch || str) {
> +		tst_res(TWARN, "Illegal version(%s), should use format like 1.43.0", version);
> +		return -1;
> +	}
> +
> +	return atoi(major) * 10000 + atoi(minor) * 100 + atoi(patch);
> +}
> +
> +static struct version_parser {
> +	const char *cmd;
> +	int (*parser)(void);
> +	int (*table_get)(char *version);
> +} version_parsers[] = {
> +	{.cmd = "mkfs.ext4", .parser = mkfs_ext4_version_parser, .table_get = mkfs_ext4_version_table_get},
> +	{},
> +};
> +
> +void tst_version_parser(const char *cmd)
> +{
> +	struct version_parser *p;
> +	char *cmd_token, *op, *version, *next, *str;
> +	char path[PATH_MAX];
> +	char parser_cmd[100];
> +	int ver_parser, ver_get;
> +	int op_flag = 0;
> +
> +	strcpy(parser_cmd, cmd);
> +
> +	cmd_token = strtok_r(parser_cmd, " ", &next);
> +	op = strtok_r(NULL, " ", &next);

Shouldn't we just handle the case where op == NULL as a single command
here? That would make the code much cleaner.

I.e.:
	if (tst_get_path(cmd_token, path, sizeof(path)))
		tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd_token);

	if (!op)
		return;

> +	version = strtok_r(NULL, " ", &next);
> +	str = strtok_r(NULL, " ", &next);



> +	if (!cmd_token || !op || !version || str)
> +		tst_brk(TCONF,
> +			"Illegal fomart(%s), should use format like mkfs.ext4 >= 1.43.0", cmd);

Then we can just check the version and str here as:

	if (!version || str)
		tst_brk(TCONF, "Illegal format ...");

> +	if (tst_get_path(cmd_token, path, sizeof(path)))
> +		tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd_token);
> +
> +	if (!strcmp(op, ">="))
> +		op_flag = 1;
> +
> +	if (!strcmp(op, ">"))
> +		op_flag = 2;
> +
> +	if (!strcmp(op, "<="))
> +		op_flag = 3;
> +
> +	if (!strcmp(op, "<"))
> +		op_flag = 4;
> +
> +	if (!strcmp(op, "=="))
> +		op_flag = 5;
> +
> +	if (!strcmp(op, "!="))
> +		op_flag = 6;
> +
> +	if (!op_flag)
> +		tst_brk(TCONF, "Invalid op(%s)", op);

This is usually structured as else if (..) in C as:

	if (!strcmp(op, ">="))
		op_flag = ...
	else if (!strcmp(op, ">")
		op_flag = ...
	else ....

Also maybe an enum would make the code easier to read as:

enum op {
	OP_GE, /* >= */
	OP_GT, /* >  */
	OP_LE, /* <= */
	OP_LT, /* <  */
	OP_EQ, /* == */
	OP_NE, /* != */
};

Then we would do:

	if (!strcmp(op, ">="))
		op_flag = OP_GE;
	else ...

> +	for (p = &version_parsers[0]; p; p++) {
> +		if (!strcmp(p->cmd, cmd_token))
> +			tst_res(TINFO, "Parsing %s version", p->cmd);
> +			break;

This does not work without curly braces around the blok, right?

> +	}
> +
> +	if (!p->cmd)
> +		tst_brk(TBROK, "No version parser for %s implemented!", cmd_token);
> +
> +	ver_parser = p->parser();
> +	if (ver_parser < 0)
> +		tst_brk(TBROK, "Failed to parse %s version", p->cmd);
> +
> +	ver_get = p->table_get(version);
> +	if (ver_get < 0)
> +		tst_brk(TBROK, "Failed to get %s version", p->cmd);
> +
> +	switch (op_flag) {
> +	case 1:
> +		if (ver_parser < ver_get)
> +			tst_brk(TCONF, "cmd(%s) expected >= %d, but got %d", cmd, ver_get, ver_parser);
                                                 ^
					"%s required >= %d, but got %d", ...

The version is required in order run the test.

> +		break;
> +	case 2:
> +		if (ver_parser <= ver_get)
> +			tst_brk(TCONF, "cmd(%s) expected > %d, but got %d", cmd, ver_get, ver_parser);
> +		break;
> +	case 3:
> +		if (ver_parser > ver_get)
> +			tst_brk(TCONF, "cmd(%s) expected <= %d, but got %d", cmd, ver_get, ver_parser);
> +		break;
> +	case 4:
> +		if (ver_parser >= ver_get)
> +			tst_brk(TCONF, "cmd(%s) expected < %d, but got %d", cmd, ver_get, ver_parser);
> +		break;
> +	case 5:
> +		if (ver_parser != ver_get)
> +			tst_brk(TCONF, "cmd(%s) expected == %d, but got %d", cmd, ver_get, ver_parser);
> +		break;
> +	case 6:
> +		if (ver_parser == ver_get)
> +			tst_brk(TCONF, "cmd(%s) expected != %d, but got %d", cmd, ver_get, ver_parser);
> +		break;
> +	}


And the same for the rest of the messages.

> +}

Looks mostly good, minus the minor things I've pointed out.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/2] lib: add cmd parse handler in needs_cmd

[xuyang2018.jy]

Hi Cyril
> Hi!
>> Testcase ie statx05 needs mkfs.ext4>= 1.43.0 because of encrypt feature.
>>
>> As Cyril suggested, add cmd parser handler in needs_cmd.
>>
>> The difference logic about from before, we don't report not-found error and think it is
>> cmd version string(need to use tst_version_parser) if tst_get_path fails in tst_test.c.
>>
>> In tst_version_parser function, use strtok_r to split cmd_token,op_token,version_token.
>> It only supports six operations '>=' '<=' '>''<' '==' '!='.
>>
>> Currently, this tst_version_parser only supports mkfs.ext4 command. If you want to support
>> more commands, just add your own .parser and .table_get methond in version_parsers structure.
>>
>> Suggested-by: Cyril Hrubis<chrubis@suse.cz>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   doc/c-test-api.txt                       |  14 +++
>>   include/tst_test.h                       |   1 +
>>   include/tst_version_parser.h             |  15 +++
>>   lib/newlib_tests/.gitignore              |   5 +
>>   lib/newlib_tests/test_version_parser01.c |  25 ++++
>>   lib/newlib_tests/test_version_parser02.c |  24 ++++
>>   lib/newlib_tests/test_version_parser03.c |  24 ++++
>>   lib/newlib_tests/test_version_parser04.c |  24 ++++
>>   lib/newlib_tests/test_version_parser05.c |  24 ++++
>>   lib/tst_test.c                           |   2 +-
>>   lib/tst_version_parser.c                 | 148 +++++++++++++++++++++++
>>   11 files changed, 305 insertions(+), 1 deletion(-)
>>   create mode 100644 include/tst_version_parser.h
>>   create mode 100644 lib/newlib_tests/test_version_parser01.c
>>   create mode 100644 lib/newlib_tests/test_version_parser02.c
>>   create mode 100644 lib/newlib_tests/test_version_parser03.c
>>   create mode 100644 lib/newlib_tests/test_version_parser04.c
>>   create mode 100644 lib/newlib_tests/test_version_parser05.c
>>   create mode 100644 lib/tst_version_parser.c
>>
>> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
>> index 64d0630ce..fde6dce0c 100644
>> --- a/doc/c-test-api.txt
>> +++ b/doc/c-test-api.txt
>> @@ -2013,6 +2013,20 @@ terminated array of strings such as:
>>   },
>>   -------------------------------------------------------------------------------
>>
>> +Also can check required commands version whether is satisfied by using 'needs_cmds',
>> +
>> +[source,c]
>> +-------------------------------------------------------------------------------
>> +.needs_cmds = (const char *const []) {
>> +	"mkfs.ext4>= 1.43.0",
>> +	NULL
>> +},
>> ++-------------------------------------------------------------------------------
>> +
>> +Currently, we only support mkfs.ext4 command. If you want to support more commands,
>> +please fill your own .parser and .table_get method in the version_parsers structure
>> +of lib/tst_version_parser.c.
>> +
>>   1.36 Assert sys or proc file value
>>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   Using TST_ASSERT_INT/STR(path, val) to assert that integer value or string stored in
>> diff --git a/include/tst_test.h b/include/tst_test.h
>> index c06a4729b..fd3d4cfee 100644
>> --- a/include/tst_test.h
>> +++ b/include/tst_test.h
>> @@ -44,6 +44,7 @@
>>   #include "tst_taint.h"
>>   #include "tst_memutils.h"
>>   #include "tst_arch.h"
>> +#include "tst_version_parser.h"
>
> Since this is internal functionality that does not make sense to be
> exposed to the tests there is no point in exposing this here, we should
> include it in the tst_test.c instead.
>
Ok, will move into tst_test.c.
>>   /*
>>    * Reports testcase result.
>> diff --git a/include/tst_version_parser.h b/include/tst_version_parser.h
>> new file mode 100644
>> index 000000000..145043929
>> --- /dev/null
>> +++ b/include/tst_version_parser.h
>> @@ -0,0 +1,15 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2021 Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +#ifndef TST_VERSION_PARSER_H__
>> +#define TST_VERSION_PARSER_H__
>> +
>> +/*
>> + * Parse the cmd version requirement in needs_cmds member of tst_test
>> + * structure whether is satisfied.
>> + */
>> +void tst_version_parser(const char *cmd);
>
> This should probably be called tst_cmd_check() or something similar.
OK.
>
>> +#endif /* TST_VERSION_PARSER_H__ */
>> diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
>> index cf467b5a0..ef8694d08 100644
>> --- a/lib/newlib_tests/.gitignore
>> +++ b/lib/newlib_tests/.gitignore
>> @@ -46,4 +46,9 @@ test_macros06
>>   tst_fuzzy_sync01
>>   tst_fuzzy_sync02
>>   tst_fuzzy_sync03
>> +test_version_parser01
>> +test_version_parser02
>> +test_version_parser03
>> +test_version_parser04
>> +test_version_parser05
>>   test_zero_hugepage
>> diff --git a/lib/newlib_tests/test_version_parser01.c b/lib/newlib_tests/test_version_parser01.c
>> new file mode 100644
>> index 000000000..9e1bd3962
>> --- /dev/null
>> +++ b/lib/newlib_tests/test_version_parser01.c
>> @@ -0,0 +1,25 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2021 Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +#include<stdio.h>
>> +#include "tst_test.h"
>> +
>> +static void do_test(void)
>> +{
>> +	tst_res(TPASS, "Tesing tst_version_parser() functionality OK.");
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = do_test,
>> +	.needs_cmds = (const char *[]) {
>> +		"mkfs.ext4",
>> +		"mkfs.ext4>= 1.43.0",
>> +		"mkfs.ext4<= 2.0.0",
>> +		"mkfs.ext4 != 2.0.0",
>> +		"mkfs.ext4>  1.43.0",
>> +		"mkfs.ext4<  2.0.0",
>> +		NULL
>> +	}
>> +};
>> diff --git a/lib/newlib_tests/test_version_parser02.c b/lib/newlib_tests/test_version_parser02.c
>> new file mode 100644
>> index 000000000..5aedaa28f
>> --- /dev/null
>> +++ b/lib/newlib_tests/test_version_parser02.c
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2021 Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*
>> + * Test Illegal format by using Illegal cmd.
>> + */
>> +
>> +#include<stdio.h>
>> +#include "tst_test.h"
>> +
>> +static void do_test(void)
>> +{
>> +	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
>                          ^
> 			"Nonexisting command is present!"
>
>
> Or something along these line.
OK.
>
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = do_test,
>> +	.needs_cmds = (const char *[]) {
>> +		"mkfs.ext45>= 1.43.0",
>> +		NULL
>> +	}
>> +};
>> diff --git a/lib/newlib_tests/test_version_parser03.c b/lib/newlib_tests/test_version_parser03.c
>> new file mode 100644
>> index 000000000..8f96e68d2
>> --- /dev/null
>> +++ b/lib/newlib_tests/test_version_parser03.c
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2021 Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*
>> + * Test Illegal format by using Illegal operation.
>> + */
>> +
>> +#include<stdio.h>
>> +#include "tst_test.h"
>> +
>> +static void do_test(void)
>> +{
>> +	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
>                            ^
> 			  "Wrong operator was evaluated!"
OK.
>
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = do_test,
>> +	.needs_cmds = (const char *[]) {
>> +		"mkfs.ext4 ! 1.43.0",
>> +		NULL
>> +	}
>> +};
>> diff --git a/lib/newlib_tests/test_version_parser04.c b/lib/newlib_tests/test_version_parser04.c
>> new file mode 100644
>> index 000000000..461f673df
>> --- /dev/null
>> +++ b/lib/newlib_tests/test_version_parser04.c
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2021 Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*
>> + * Test Illegal format by using Illegal version.
>> + */
>> +
>> +#include<stdio.h>
>> +#include "tst_test.h"
>> +
>> +static void do_test(void)
>> +{
>> +	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
>                            ^
> 			  "Incomplete version was parsed!"
OK.
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = do_test,
>> +	.needs_cmds = (const char *[]) {
>> +		"mkfs.ext4>  1.43",
>> +		NULL
>> +	}
>> +};
>> diff --git a/lib/newlib_tests/test_version_parser05.c b/lib/newlib_tests/test_version_parser05.c
>> new file mode 100644
>> index 000000000..1bfe24f73
>> --- /dev/null
>> +++ b/lib/newlib_tests/test_version_parser05.c
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2021 Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*
>> + * Test non-existed cmd whether still can be detected.
>> + */
>> +
>> +#include<stdio.h>
>> +#include "tst_test.h"
>> +
>> +static void do_test(void)
>> +{
>> +	tst_res(TFAIL, "Tesing tst_version_parser() functionality bad.");
>                          ^
> 			"Nonexisting command is present!"
OK.
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = do_test,
>> +	.needs_cmds = (const char *[]) {
>> +		"mkfs.ext45",
>> +		NULL
>> +	}
>> +};
>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>> index a79275722..a9e95a3d7 100644
>> --- a/lib/tst_test.c
>> +++ b/lib/tst_test.c
>> @@ -992,7 +992,7 @@ static void do_setup(int argc, char *argv[])
>>
>>   		for (i = 0; (cmd = tst_test->needs_cmds[i]); ++i)
>>   			if (tst_get_path(cmd, path, sizeof(path)))
>> -				tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd);
>> +				tst_version_parser(cmd);
>
> Why do we keep the tst_get_path() here? We can as well get rid of that
> and just call the function and handle the case without the version
> there.
Yes, will use tst_cmd_check directly here.
>
>>   	}
>>
>>   	if (tst_test->needs_drivers) {
>> diff --git a/lib/tst_version_parser.c b/lib/tst_version_parser.c
>> new file mode 100644
>> index 000000000..296e25ea2
>> --- /dev/null
>> +++ b/lib/tst_version_parser.c
>> @@ -0,0 +1,148 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2021 Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +#include<stdio.h>
>> +#include<string.h>
>> +#include<stdlib.h>
>> +
>> +#define TST_NO_DEFAULT_MAIN
>> +#include "tst_version_parser.h"
>> +#include "tst_test.h"
>> +
>> +static int mkfs_ext4_version_parser(void)
>> +{
>> +	FILE *f;
>> +	int rc, major, minor, patch;
>> +
>> +	f = popen("mkfs.ext4 -V 2>&1", "r");
>> +	if (!f) {
>> +		tst_res(TWARN, "Could not run mkfs.ext4 -V 2>&1 cmd");
>> +		return -1;
>> +	}
>> +	rc = fscanf(f, "mke2fs %d.%d.%d",&major,&minor,&patch);
>> +	pclose(f);
>> +	if (rc != 3) {
>> +		tst_res(TWARN, "Unable to parse mkfs.ext4 version");
>> +		return -1;
>> +	}
>> +
>> +	return major * 10000 +  minor * 100 + patch;
>> +}
>> +
>> +static int mkfs_ext4_version_table_get(char *version)
>> +{
>> +	char *major, *minor, *patch, *next, *str;
>> +	char table_version[100];
>> +
>> +	strcpy(table_version, version);
>> +
>> +	major = strtok_r(table_version, ".",&next);
>> +	minor = strtok_r(NULL, ".",&next);
>> +	patch = strtok_r(NULL, ".",&next);
>> +	str = strtok_r(NULL, ".",&next);
>
> I wonder if we should do a sscanf() here. Note that with the atoi() we
> will pass on strings that have garbage after the numbers such as:
>
> 	"1ab.2c.3dd"
>
> What about:
>
> 	unsigned int maj, min, patch;
> 	int len;
>
> 	if (sscanf(version, "%u.%u.%u %n",&maj,&min,&patch, len) != 3) {
> 		tst_res(TWARN, "Illegal version ...");
> 		return -1;
> 	}
>
> 	if (len != strlen(version)) {
> 		tst_res(TWARN, "Garbage after version");
> 		return -1;
> 	}
>
> 	return maj * 10000 + min * 100 + patch;
Yes, it looks better. I will try.
>
>> +	if (!major || !minor || !patch || str) {
>> +		tst_res(TWARN, "Illegal version(%s), should use format like 1.43.0", version);
>> +		return -1;
>> +	}
>> +
>> +	return atoi(major) * 10000 + atoi(minor) * 100 + atoi(patch);
>> +}
>> +
>> +static struct version_parser {
>> +	const char *cmd;
>> +	int (*parser)(void);
>> +	int (*table_get)(char *version);
>> +} version_parsers[] = {
>> +	{.cmd = "mkfs.ext4", .parser = mkfs_ext4_version_parser, .table_get = mkfs_ext4_version_table_get},
>> +	{},
>> +};
>> +
>> +void tst_version_parser(const char *cmd)
>> +{
>> +	struct version_parser *p;
>> +	char *cmd_token, *op, *version, *next, *str;
>> +	char path[PATH_MAX];
>> +	char parser_cmd[100];
>> +	int ver_parser, ver_get;
>> +	int op_flag = 0;
>> +
>> +	strcpy(parser_cmd, cmd);
>> +
>> +	cmd_token = strtok_r(parser_cmd, " ",&next);
>> +	op = strtok_r(NULL, " ",&next);
>
> Shouldn't we just handle the case where op == NULL as a single command
> here? That would make the code much cleaner.
>
Yes.
> I.e.:
> 	if (tst_get_path(cmd_token, path, sizeof(path)))
> 		tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd_token);
>
> 	if (!op)
> 		return;
>
>> +	version = strtok_r(NULL, " ",&next);
>> +	str = strtok_r(NULL, " ",&next);
>
>
>
>> +	if (!cmd_token || !op || !version || str)
>> +		tst_brk(TCONF,
>> +			"Illegal fomart(%s), should use format like mkfs.ext4>= 1.43.0", cmd);
>
> Then we can just check the version and str here as:
>
> 	if (!version || str)
> 		tst_brk(TCONF, "Illegal format ...");
>
>> +	if (tst_get_path(cmd_token, path, sizeof(path)))
>> +		tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd_token);
>> +
>> +	if (!strcmp(op, ">="))
>> +		op_flag = 1;
>> +
>> +	if (!strcmp(op, ">"))
>> +		op_flag = 2;
>> +
>> +	if (!strcmp(op, "<="))
>> +		op_flag = 3;
>> +
>> +	if (!strcmp(op, "<"))
>> +		op_flag = 4;
>> +
>> +	if (!strcmp(op, "=="))
>> +		op_flag = 5;
>> +
>> +	if (!strcmp(op, "!="))
>> +		op_flag = 6;
>> +
>> +	if (!op_flag)
>> +		tst_brk(TCONF, "Invalid op(%s)", op);
>
> This is usually structured as else if (..) in C as:
>
> 	if (!strcmp(op, ">="))
> 		op_flag = ...
> 	else if (!strcmp(op, ">")
> 		op_flag = ...
> 	else ....
Ok.
>
> Also maybe an enum would make the code easier to read as:
>
> enum op {
> 	OP_GE, /*>= */
> 	OP_GT, /*>   */
> 	OP_LE, /*<= */
> 	OP_LT, /*<   */
> 	OP_EQ, /* == */
> 	OP_NE, /* != */
> };
>
It looks better.
> Then we would do:
>
> 	if (!strcmp(op, ">="))
> 		op_flag = OP_GE;
> 	else ...
>
>> +	for (p =&version_parsers[0]; p; p++) {
>> +		if (!strcmp(p->cmd, cmd_token))
>> +			tst_res(TINFO, "Parsing %s version", p->cmd);
>> +			break;
>
> This does not work without curly braces around the blok, right?
Yes,
>
>> +	}
>> +
>> +	if (!p->cmd)
>> +		tst_brk(TBROK, "No version parser for %s implemented!", cmd_token);
>> +
>> +	ver_parser = p->parser();
>> +	if (ver_parser<  0)
>> +		tst_brk(TBROK, "Failed to parse %s version", p->cmd);
>> +
>> +	ver_get = p->table_get(version);
>> +	if (ver_get<  0)
>> +		tst_brk(TBROK, "Failed to get %s version", p->cmd);
>> +
>> +	switch (op_flag) {
>> +	case 1:
>> +		if (ver_parser<  ver_get)
>> +			tst_brk(TCONF, "cmd(%s) expected>= %d, but got %d", cmd, ver_get, ver_parser);
>                                                   ^
> 					"%s required>= %d, but got %d", ...
>
> The version is required in order run the test.
Got it.
>
>> +		break;
>> +	case 2:
>> +		if (ver_parser<= ver_get)
>> +			tst_brk(TCONF, "cmd(%s) expected>  %d, but got %d", cmd, ver_get, ver_parser);
>> +		break;
>> +	case 3:
>> +		if (ver_parser>  ver_get)
>> +			tst_brk(TCONF, "cmd(%s) expected<= %d, but got %d", cmd, ver_get, ver_parser);
>> +		break;
>> +	case 4:
>> +		if (ver_parser>= ver_get)
>> +			tst_brk(TCONF, "cmd(%s) expected<  %d, but got %d", cmd, ver_get, ver_parser);
>> +		break;
>> +	case 5:
>> +		if (ver_parser != ver_get)
>> +			tst_brk(TCONF, "cmd(%s) expected == %d, but got %d", cmd, ver_get, ver_parser);
>> +		break;
>> +	case 6:
>> +		if (ver_parser == ver_get)
>> +			tst_brk(TCONF, "cmd(%s) expected != %d, but got %d", cmd, ver_get, ver_parser);
>> +		break;
>> +	}
>
>
> And the same for the rest of the messages.
Wil change.
>
>> +}
>
> Looks mostly good, minus the minor things I've pointed out.
Thanks for your review, will send a v2 as soon as possible.

Best Regards
Yang Xu
>

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

[LTP] [PATCH v2 1/2] library: add cmd check handler in needs_cmds

[Yang Xu]

Testcase ie statx05 needs mkfs.ext4 >= 1.43.0 because of encrypt feature.

As Cyril suggested, add cmd check handler in needs_cmd.

We don't use tst_ prefix ie tst_check_cmd since we don't export this api to user.
This check_cmd not only check cmd whether existed but also check the cmd version whether
meet test's requirement.

In check_cmd function, use strtok_r to split cmd_token,op_token,version_token.
It only supports six operations '>=' '<=' '>' '<' '==' '!='.

Currently, for the command version check, it only supports  mkfs.ext4 command. If you
want to support more commands, just add your own .parser and .table_get methond in
version_parsers structure.

Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
v1->v2
1. rename tst_version_parser to check_cmd
2. For mkfs_ext4_version_table_get method, use sscanf instead of strtok_r
3. use enum for cmd op
4. fix description
5. add more newlib test for this
 doc/c-test-api.txt                   |  14 +++
 lib/newlib_tests/.gitignore          |   8 ++
 lib/newlib_tests/test_needs_cmds01.c |  25 ++++
 lib/newlib_tests/test_needs_cmds02.c |  24 ++++
 lib/newlib_tests/test_needs_cmds03.c |  24 ++++
 lib/newlib_tests/test_needs_cmds04.c |  24 ++++
 lib/newlib_tests/test_needs_cmds05.c |  24 ++++
 lib/newlib_tests/test_needs_cmds06.c |  24 ++++
 lib/newlib_tests/test_needs_cmds07.c |  24 ++++
 lib/newlib_tests/test_needs_cmds08.c |  27 +++++
 lib/tst_test.c                       | 169 ++++++++++++++++++++++++++-
 11 files changed, 384 insertions(+), 3 deletions(-)
 create mode 100644 lib/newlib_tests/test_needs_cmds01.c
 create mode 100644 lib/newlib_tests/test_needs_cmds02.c
 create mode 100644 lib/newlib_tests/test_needs_cmds03.c
 create mode 100644 lib/newlib_tests/test_needs_cmds04.c
 create mode 100644 lib/newlib_tests/test_needs_cmds05.c
 create mode 100644 lib/newlib_tests/test_needs_cmds06.c
 create mode 100644 lib/newlib_tests/test_needs_cmds07.c
 create mode 100644 lib/newlib_tests/test_needs_cmds08.c

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 64d0630ce..d35708516 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -2013,6 +2013,20 @@ terminated array of strings such as:
 },
 -------------------------------------------------------------------------------
 
+Also can check required commands version whether is satisfied by using 'needs_cmds',
+
+[source,c]
+-------------------------------------------------------------------------------
+.needs_cmds = (const char *const []) {
+	"mkfs.ext4 >= 1.43.0",
+	NULL
+},
++-------------------------------------------------------------------------------
+
+Currently, we only support mkfs.ext4 command. If you want to support more commands,
+please fill your own .parser and .table_get method in the version_parsers structure
+of lib/tst_test.c.
+
 1.36 Assert sys or proc file value
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 Using TST_ASSERT_INT/STR(path, val) to assert that integer value or string stored in
diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
index cf467b5a0..a19fa22e8 100644
--- a/lib/newlib_tests/.gitignore
+++ b/lib/newlib_tests/.gitignore
@@ -46,4 +46,12 @@ test_macros06
 tst_fuzzy_sync01
 tst_fuzzy_sync02
 tst_fuzzy_sync03
+test_needs_cmds01
+test_needs_cmds02
+test_needs_cmds03
+test_needs_cmds04
+test_needs_cmds05
+test_needs_cmds06
+test_needs_cmds07
+test_needs_cmds08
 test_zero_hugepage
diff --git a/lib/newlib_tests/test_needs_cmds01.c b/lib/newlib_tests/test_needs_cmds01.c
new file mode 100644
index 000000000..0ce69d61e
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds01.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TPASS, "Tesing check_cmd() functionality OK.");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4",
+		"mkfs.ext4 >= 1.43.0",
+		"mkfs.ext4 <= 2.0.0",
+		"mkfs.ext4 != 2.0.0",
+		"mkfs.ext4 > 1.43.0",
+		"mkfs.ext4 < 2.0.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds02.c b/lib/newlib_tests/test_needs_cmds02.c
new file mode 100644
index 000000000..1eeaf6351
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds02.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using non-existing cmd.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Nonexisting command is present!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext45 >= 1.43.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds03.c b/lib/newlib_tests/test_needs_cmds03.c
new file mode 100644
index 000000000..c50077f4e
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds03.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using Illegal operation.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Wrong operator was evaluated!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 ! 1.43.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds04.c b/lib/newlib_tests/test_needs_cmds04.c
new file mode 100644
index 000000000..5d05ed46d
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds04.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using incomplete version.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Incomplete version was parsed!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 > 1.43",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds05.c b/lib/newlib_tests/test_needs_cmds05.c
new file mode 100644
index 000000000..f4b509b68
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds05.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using version that has garbage.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Garbage version was parsed!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 > 1.43.0-1",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds06.c b/lib/newlib_tests/test_needs_cmds06.c
new file mode 100644
index 000000000..f1234820e
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds06.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format with garbage.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Garbage format was parsed!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 > 1.43.0 2",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds07.c b/lib/newlib_tests/test_needs_cmds07.c
new file mode 100644
index 000000000..e2d2643f4
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds07.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test non-existed cmd whether still can be detected.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Nonexisting command is present!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext45",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds08.c b/lib/newlib_tests/test_needs_cmds08.c
new file mode 100644
index 000000000..342c3716c
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds08.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test mkfs.xfs that it doesn't have own parser and table_get function
+ * at the version_parsers structure in lib/tst_test.c.
+ * So it should report parser function for this cmd is not implemented.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Nonexisting parser function for mkfs.xfs is present!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.xfs",
+		"mkfs.xfs >= 4.20.0",
+		NULL
+	}
+};
diff --git a/lib/tst_test.c b/lib/tst_test.c
index a79275722..7cca209ab 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -65,6 +65,15 @@ struct results {
 	unsigned int timeout;
 };
 
+enum cmd_op {
+	OP_GE, /* >= */
+	OP_GT, /* >  */
+	OP_LE, /* <= */
+	OP_LT, /* <  */
+	OP_EQ, /* == */
+	OP_NE, /* != */
+};
+
 static struct results *results;
 
 static int ipc_fd;
@@ -950,6 +959,162 @@ static void prepare_device(void)
 	}
 }
 
+static int mkfs_ext4_version_parser(void)
+{
+	FILE *f;
+	int rc, major, minor, patch;
+
+	f = popen("mkfs.ext4 -V 2>&1", "r");
+	if (!f) {
+		tst_res(TWARN, "Could not run mkfs.ext4 -V 2>&1 cmd");
+		return -1;
+	}
+	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
+	pclose(f);
+	if (rc != 3) {
+		tst_res(TWARN, "Unable to parse mkfs.ext4 version");
+		return -1;
+	}
+
+	return major * 10000 +  minor * 100 + patch;
+}
+
+static int mkfs_ext4_version_table_get(char *version)
+{
+	int major, minor, patch;
+	int len;
+
+	if (sscanf(version, "%u.%u.%u %n", &major, &minor, &patch, &len) != 3) {
+		tst_res(TWARN, "Illega version(%s), "
+			"should use format like 1.43.0", version);
+		return -1;
+	}
+
+	if (len != (int)strlen(version)) {
+		tst_res(TWARN, "Grabage after version");
+		return -1;
+	}
+
+	return major * 10000 + minor * 100 + patch;
+}
+
+static struct version_parser {
+	const char *cmd;
+	int (*parser)(void);
+	int (*table_get)(char *version);
+} version_parsers[] = {
+	{"mkfs.ext4", mkfs_ext4_version_parser, mkfs_ext4_version_table_get},
+	{},
+};
+
+static void check_cmd(const char *cmd)
+{
+	struct version_parser *p;
+	char *cmd_token, *op_token, *version_token, *next, *str;
+	char path[PATH_MAX];
+	char parser_cmd[100];
+	int ver_parser, ver_get;
+	int op_flag = 0;
+
+	strcpy(parser_cmd, cmd);
+
+	cmd_token = strtok_r(parser_cmd, " ", &next);
+	op_token = strtok_r(NULL, " ", &next);
+	version_token = strtok_r(NULL, " ", &next);
+	str = strtok_r(NULL, " ", &next);
+
+	if (tst_get_path(cmd_token, path, sizeof(path)))
+		tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd_token);
+
+	if (!op_token)
+		return;
+
+	if (!strcmp(op_token, ">="))
+		op_flag = OP_GE;
+	else if (!strcmp(op_token, ">"))
+		op_flag = OP_GT;
+	else if (!strcmp(op_token, "<="))
+		op_flag = OP_LE;
+	else if (!strcmp(op_token, "<"))
+		op_flag = OP_LT;
+	else if (!strcmp(op_token, "=="))
+		op_flag = OP_EQ;
+	else if (!strcmp(op_token, "!="))
+		op_flag = OP_NE;
+	else
+		tst_brk(TCONF, "Invalid op(%s)", op_token);
+
+	if (!version_token || str) {
+		tst_brk(TCONF, "Illegal format(%s), should use format like "
+			"mkfs.ext4 >= 1.43.0", cmd);
+	}
+
+	for (p = &version_parsers[0]; p->cmd; p++) {
+		if (!strcmp(p->cmd, cmd_token)) {
+			tst_res(TINFO, "Parsing %s version", p->cmd);
+			break;
+		}
+	}
+
+	if (!p->cmd) {
+		tst_brk(TBROK, "No version parser for %s implemented!",
+			cmd_token);
+	}
+
+	ver_parser = p->parser();
+	if (ver_parser < 0)
+		tst_brk(TBROK, "Failed to parse %s version", p->cmd);
+
+	ver_get = p->table_get(version_token);
+	if (ver_get < 0)
+		tst_brk(TBROK, "Failed to get %s version", p->cmd);
+
+	switch (op_flag) {
+	case OP_GE:
+		if (ver_parser < ver_get) {
+			tst_brk(TCONF, "%s required >= %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_GT:
+		if (ver_parser <= ver_get) {
+			tst_brk(TCONF, "%s required > %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_LE:
+		if (ver_parser > ver_get) {
+			tst_brk(TCONF, "%s required <= %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_LT:
+		if (ver_parser >= ver_get) {
+			tst_brk(TCONF, "%s required < %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_EQ:
+		if (ver_parser != ver_get) {
+			tst_brk(TCONF, "%s required == %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_NE:
+		if (ver_parser == ver_get) {
+			tst_brk(TCONF, "%s required != %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	}
+}
+
 static void do_setup(int argc, char *argv[])
 {
 	if (!tst_test)
@@ -987,12 +1152,10 @@ static void do_setup(int argc, char *argv[])
 
 	if (tst_test->needs_cmds) {
 		const char *cmd;
-		char path[PATH_MAX];
 		int i;
 
 		for (i = 0; (cmd = tst_test->needs_cmds[i]); ++i)
-			if (tst_get_path(cmd, path, sizeof(path)))
-				tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd);
+			check_cmd(cmd);
 	}
 
 	if (tst_test->needs_drivers) {
-- 
2.23.0


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

[LTP] [PATCH v2 2/2] testcase: make use of needs_cmd version check

[Yang Xu]

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 testcases/kernel/syscalls/quotactl/quotactl04.c | 11 +----------
 testcases/kernel/syscalls/statx/statx05.c       | 13 ++-----------
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/testcases/kernel/syscalls/quotactl/quotactl04.c b/testcases/kernel/syscalls/quotactl/quotactl04.c
index dab61cf4d..62c94d596 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl04.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl04.c
@@ -121,18 +121,9 @@ static void do_mount(const char *source, const char *target,
 
 static void setup(void)
 {
-	FILE *f;
 	const char *const fs_opts[] = {"-I 256", "-O quota,project", NULL};
-	int rc, major, minor, patch;
 
 	test_id = geteuid();
-	f = SAFE_POPEN("mkfs.ext4 -V 2>&1", "r");
-	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
-	if (rc != 3)
-		tst_res(TWARN, "Unable parse version number");
-	else if (major * 10000 + minor * 100 + patch < 14300)
-		tst_brk(TCONF, "Test needs mkfs.ext4 >= 1.43 for quota,project option, test skipped");
-	pclose(f);
 	SAFE_MKFS(tst_device->dev, tst_device->fs_type, fs_opts, NULL);
 	do_mount(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, NULL);
 }
@@ -184,7 +175,7 @@ static struct tst_test test = {
 	.dev_fs_type = "ext4",
 	.mntpoint = MNTPOINT,
 	.needs_cmds = (const char *[]) {
-		"mkfs.ext4",
+		"mkfs.ext4 >= 1.43.0",
 		NULL
 	}
 };
diff --git a/testcases/kernel/syscalls/statx/statx05.c b/testcases/kernel/syscalls/statx/statx05.c
index 86bd3ace1..bdee98c22 100644
--- a/testcases/kernel/syscalls/statx/statx05.c
+++ b/testcases/kernel/syscalls/statx/statx05.c
@@ -87,18 +87,9 @@ static void run(unsigned int i)
 
 static void setup(void)
 {
-	FILE *f;
 	char opt_bsize[32];
 	const char *const fs_opts[] = {"-O encrypt", opt_bsize, NULL};
-	int ret, rc, major, minor, patch;
-
-	f = SAFE_POPEN("mkfs.ext4 -V 2>&1", "r");
-	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
-	if (rc != 3)
-		tst_res(TWARN, "Unable parse version number");
-	else if (major * 10000 + minor * 100 + patch < 14300)
-		tst_brk(TCONF, "Test needs mkfs.ext4 >= 1.43 for encrypt option, test skipped");
-	pclose(f);
+	int ret;
 
 	snprintf(opt_bsize, sizeof(opt_bsize), "-b %i", getpagesize());
 
@@ -132,7 +123,7 @@ static struct tst_test test = {
 	.mntpoint = MNTPOINT,
 	.dev_fs_type = "ext4",
 	.needs_cmds = (const char *[]) {
-		"mkfs.ext4",
+		"mkfs.ext4 >= 1.43.0",
 		"e4crypt",
 		NULL
 	}
-- 
2.23.0


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

Re: [LTP] [PATCH v2 1/2] library: add cmd check handler in needs_cmds

[Cyril Hrubis]

Hi!
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index a79275722..7cca209ab 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c

I would rather put the version parsing code to a different source file
than tst_test.c just because the tst_test.c is long enough already. What
I did suggest for v1 is that the entry point function i.e.
tst_check_cmd() now wouldn't get exported as a prototype to the tests by
being defined in a header included in the tst_test.h header.

We already have include/tst_private.h that is expected to be used for
function prototypes that are not supposed to be available to the test
code.

Other than this the code looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2 1/2] library: add cmd check handler in needs_cmds

[xuyang2018.jy]

Hi Cyril
> Hi!
>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>> index a79275722..7cca209ab 100644
>> --- a/lib/tst_test.c
>> +++ b/lib/tst_test.c
>
> I would rather put the version parsing code to a different source file
> than tst_test.c just because the tst_test.c is long enough already. What
> I did suggest for v1 is that the entry point function i.e.
> tst_check_cmd() now wouldn't get exported as a prototype to the tests by
> being defined in a header included in the tst_test.h header.
>
> We already have include/tst_private.h that is expected to be used for
> function prototypes that are not supposed to be available to the test
> code.
>
Now, I understand. so I can just move tst_check_cmd declaration into 
tst_private.h and then tst_test.c included it.

But the source code should move into where, it has two choices:
1) like v1, move a new lib source file ie tst_version_parser.c
2) since it is related to cmd, we can move it into lib/tst_cmd.c


I prefer to use 2nd way. What do you think about this?

Best Regards
Yang Xu
> Other than this the code looks good.
>

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

Re: [LTP] [PATCH v2 1/2] library: add cmd check handler in needs_cmds

[Cyril Hrubis]

Hi!
> Now, I understand. so I can just move tst_check_cmd declaration into 
> tst_private.h and then tst_test.c included it.
> 
> But the source code should move into where, it has two choices:
> 1) like v1, move a new lib source file ie tst_version_parser.c
> 2) since it is related to cmd, we can move it into lib/tst_cmd.c
> 
> 
> I prefer to use 2nd way. What do you think about this?

I would probably put it into a separate file, but tst_cmd.c would work
as well.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2 1/2] library: add cmd check handler in needs_cmds

[Petr Vorel]

Hi Xu, Cyril,

> Testcase ie statx05 needs mkfs.ext4 >= 1.43.0 because of encrypt feature.

> As Cyril suggested, add cmd check handler in needs_cmd.

Great idea, I have something like this in my TODO list as well, glad I can
delete it :).

> We don't use tst_ prefix ie tst_check_cmd since we don't export this api to user.
> This check_cmd not only check cmd whether existed but also check the cmd version whether
> meet test's requirement.

> In check_cmd function, use strtok_r to split cmd_token,op_token,version_token.
> It only supports six operations '>=' '<=' '>' '<' '==' '!='.

> Currently, for the command version check, it only supports  mkfs.ext4 command. If you
> want to support more commands, just add your own .parser and .table_get methond in
> version_parsers structure.

> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
> v1->v2
> 1. rename tst_version_parser to check_cmd
Why not tst_cmd_check(), i.e. using tst_ prefix?

+1 for moving it into tst_cmd.c.


> 2. For mkfs_ext4_version_table_get method, use sscanf instead of strtok_r
> 3. use enum for cmd op
> 4. fix description
> 5. add more newlib test for this
>  doc/c-test-api.txt                   |  14 +++
>  lib/newlib_tests/.gitignore          |   8 ++
>  lib/newlib_tests/test_needs_cmds01.c |  25 ++++
>  lib/newlib_tests/test_needs_cmds02.c |  24 ++++
>  lib/newlib_tests/test_needs_cmds03.c |  24 ++++
>  lib/newlib_tests/test_needs_cmds04.c |  24 ++++
>  lib/newlib_tests/test_needs_cmds05.c |  24 ++++
>  lib/newlib_tests/test_needs_cmds06.c |  24 ++++
>  lib/newlib_tests/test_needs_cmds07.c |  24 ++++
>  lib/newlib_tests/test_needs_cmds08.c |  27 +++++
Also, could you please put tests which expect TPASS or TCONF into
lib/newlib_tests/runtest.sh?


> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index a79275722..7cca209ab 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -65,6 +65,15 @@ struct results {
>  	unsigned int timeout;
>  };

> +enum cmd_op {
> +	OP_GE, /* >= */
> +	OP_GT, /* >  */
> +	OP_LE, /* <= */
> +	OP_LT, /* <  */
> +	OP_EQ, /* == */
> +	OP_NE, /* != */
> +};
> +
>  static struct results *results;

>  static int ipc_fd;
> @@ -950,6 +959,162 @@ static void prepare_device(void)
>  	}
>  }

> +static int mkfs_ext4_version_parser(void)
> +{
> +	FILE *f;
> +	int rc, major, minor, patch;
> +
> +	f = popen("mkfs.ext4 -V 2>&1", "r");
> +	if (!f) {
> +		tst_res(TWARN, "Could not run mkfs.ext4 -V 2>&1 cmd");
> +		return -1;
> +	}
> +	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);

I guess many functions will have X.Y.Z format. Maybe later we could have generic
functions similar to kernel SYSCALL_DEFINEn() macros, passing them just
necessary format string.  At least that was what I had in my mind when thinking
about this.

> +	pclose(f);
> +	if (rc != 3) {
> +		tst_res(TWARN, "Unable to parse mkfs.ext4 version");
> +		return -1;
> +	}
> +
> +	return major * 10000 +  minor * 100 + patch;
> +}
> +
> +static int mkfs_ext4_version_table_get(char *version)
> +{
> +	int major, minor, patch;
> +	int len;
> +
> +	if (sscanf(version, "%u.%u.%u %n", &major, &minor, &patch, &len) != 3) {
> +		tst_res(TWARN, "Illega version(%s), "
typo s/Illega/Illegal/

> +			"should use format like 1.43.0", version);
nit: I'd keep string on single line (easier to grep and it's not too long being
on single line like the others below).

Kind regards,
Petr

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

Re: [LTP] [PATCH v2 1/2] library: add cmd check handler in needs_cmds

[xuyang2018.jy]

Hi Petr
> Hi Xu, Cyril,
>
>> Testcase ie statx05 needs mkfs.ext4>= 1.43.0 because of encrypt feature.
>
>> As Cyril suggested, add cmd check handler in needs_cmd.
>
> Great idea, I have something like this in my TODO list as well, glad I can
> delete it :).
That' great. So We can have time to do other thing in ltp.
>
>> We don't use tst_ prefix ie tst_check_cmd since we don't export this api to user.
>> This check_cmd not only check cmd whether existed but also check the cmd version whether
>> meet test's requirement.
>
>> In check_cmd function, use strtok_r to split cmd_token,op_token,version_token.
>> It only supports six operations '>=' '<=' '>''<' '==' '!='.
>
>> Currently, for the command version check, it only supports  mkfs.ext4 command. If you
>> want to support more commands, just add your own .parser and .table_get methond in
>> version_parsers structure.
>
>> Suggested-by: Cyril Hrubis<chrubis@suse.cz>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>> v1->v2
>> 1. rename tst_version_parser to check_cmd
> Why not tst_cmd_check(), i.e. using tst_ prefix?
I may misunderstand ltp-003 rule. It seems a public library function 
must have tst_ prefix, but a private library function still can have 
tst_ prefix ie tst_kconfig_get.
Is it right?
I also think using tst_cmd_check is better.
>
> +1 for moving it into tst_cmd.c.
Will do.
>
>
>> 2. For mkfs_ext4_version_table_get method, use sscanf instead of strtok_r
>> 3. use enum for cmd op
>> 4. fix description
>> 5. add more newlib test for this
>>   doc/c-test-api.txt                   |  14 +++
>>   lib/newlib_tests/.gitignore          |   8 ++
>>   lib/newlib_tests/test_needs_cmds01.c |  25 ++++
>>   lib/newlib_tests/test_needs_cmds02.c |  24 ++++
>>   lib/newlib_tests/test_needs_cmds03.c |  24 ++++
>>   lib/newlib_tests/test_needs_cmds04.c |  24 ++++
>>   lib/newlib_tests/test_needs_cmds05.c |  24 ++++
>>   lib/newlib_tests/test_needs_cmds06.c |  24 ++++
>>   lib/newlib_tests/test_needs_cmds07.c |  24 ++++
>>   lib/newlib_tests/test_needs_cmds08.c |  27 +++++
> Also, could you please put tests which expect TPASS or TCONF into
> lib/newlib_tests/runtest.sh?
OK. Will do.
>
>
>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>> index a79275722..7cca209ab 100644
>> --- a/lib/tst_test.c
>> +++ b/lib/tst_test.c
>> @@ -65,6 +65,15 @@ struct results {
>>   	unsigned int timeout;
>>   };
>
>> +enum cmd_op {
>> +	OP_GE, /*>= */
>> +	OP_GT, /*>   */
>> +	OP_LE, /*<= */
>> +	OP_LT, /*<   */
>> +	OP_EQ, /* == */
>> +	OP_NE, /* != */
>> +};
>> +
>>   static struct results *results;
>
>>   static int ipc_fd;
>> @@ -950,6 +959,162 @@ static void prepare_device(void)
>>   	}
>>   }
>
>> +static int mkfs_ext4_version_parser(void)
>> +{
>> +	FILE *f;
>> +	int rc, major, minor, patch;
>> +
>> +	f = popen("mkfs.ext4 -V 2>&1", "r");
>> +	if (!f) {
>> +		tst_res(TWARN, "Could not run mkfs.ext4 -V 2>&1 cmd");
>> +		return -1;
>> +	}
>> +	rc = fscanf(f, "mke2fs %d.%d.%d",&major,&minor,&patch);
>
> I guess many functions will have X.Y.Z format. Maybe later we could have generic
> functions similar to kernel SYSCALL_DEFINEn() macros, passing them just
> necessary format string.  At least that was what I had in my mind when thinking
> about this.
Yes, we can have a generic function in the feature if cases have this 
requirement.
>
>> +	pclose(f);
>> +	if (rc != 3) {
>> +		tst_res(TWARN, "Unable to parse mkfs.ext4 version");
>> +		return -1;
>> +	}
>> +
>> +	return major * 10000 +  minor * 100 + patch;
>> +}
>> +
>> +static int mkfs_ext4_version_table_get(char *version)
>> +{
>> +	int major, minor, patch;
>> +	int len;
>> +
>> +	if (sscanf(version, "%u.%u.%u %n",&major,&minor,&patch,&len) != 3) {
>> +		tst_res(TWARN, "Illega version(%s), "
> typo s/Illega/Illegal/
>
>> +			"should use format like 1.43.0", version);
> nit: I'd keep string on single line (easier to grep and it's not too long being
> on single line like the others below).
OK. Will do.

Best Regards
Yang Xu
>
> Kind regards,
> Petr

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

[LTP] [PATCH v3 1/2] library: add cmd check handler in needs_cmds

[Yang Xu]

Testcase ie statx05 needs mkfs.ext4 >= 1.43.0 because of encrypt feature.

As Cyril suggested, add cmd check handler in needs_cmd.

This tst_check_cmd not only check cmd whether exists but also check the cmd
version whether meets test's requirement.

In tst_check_cmd function, use strtok_r to split cmd_token,op_token,version_token.
It only supports six operations '>=' '<=' '>' '<' '==' '!='.

Currently, for the command version check, it only supports mkfs.ext4 command. If you
want to support more commands, just add your own .parser and .table_get methond in
version_parsers structure.

Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
v2-v3:
1. rename check_cmd to tst_check_cmd
2. move code into tst_cmd.c instead of tst_test.c
3. export tst_cmd_check function into tst_private.h
4. add test_needs_cmds cases into runtest.sh
5. fix typo
v1->v2
1. rename tst_version_parser to check_cmd
2. For mkfs_ext4_version_table_get method, use sscanf instead of strtok_r
3. use enum for cmd op
4. fix description
5. add more newlib test for this
 doc/c-test-api.txt                   |  14 +++
 include/tst_private.h                |   9 ++
 lib/newlib_tests/.gitignore          |   8 ++
 lib/newlib_tests/runtest.sh          |   5 +-
 lib/newlib_tests/test_needs_cmds01.c |  25 ++++
 lib/newlib_tests/test_needs_cmds02.c |  24 ++++
 lib/newlib_tests/test_needs_cmds03.c |  24 ++++
 lib/newlib_tests/test_needs_cmds04.c |  24 ++++
 lib/newlib_tests/test_needs_cmds05.c |  24 ++++
 lib/newlib_tests/test_needs_cmds06.c |  24 ++++
 lib/newlib_tests/test_needs_cmds07.c |  24 ++++
 lib/newlib_tests/test_needs_cmds08.c |  27 +++++
 lib/tst_cmd.c                        | 168 +++++++++++++++++++++++++++
 lib/tst_test.c                       |   6 +-
 14 files changed, 400 insertions(+), 6 deletions(-)
 create mode 100644 lib/newlib_tests/test_needs_cmds01.c
 create mode 100644 lib/newlib_tests/test_needs_cmds02.c
 create mode 100644 lib/newlib_tests/test_needs_cmds03.c
 create mode 100644 lib/newlib_tests/test_needs_cmds04.c
 create mode 100644 lib/newlib_tests/test_needs_cmds05.c
 create mode 100644 lib/newlib_tests/test_needs_cmds06.c
 create mode 100644 lib/newlib_tests/test_needs_cmds07.c
 create mode 100644 lib/newlib_tests/test_needs_cmds08.c

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 64d0630ce..0555cd614 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -2013,6 +2013,20 @@ terminated array of strings such as:
 },
 -------------------------------------------------------------------------------
 
+Also can check required commands version whether is satisfied by using 'needs_cmds',
+
+[source,c]
+-------------------------------------------------------------------------------
+.needs_cmds = (const char *const []) {
+	"mkfs.ext4 >= 1.43.0",
+	NULL
+},
++-------------------------------------------------------------------------------
+
+Currently, we only support mkfs.ext4 command version check.
+If you want to support more commands, please fill your own .parser and .table_get
+method in the version_parsers structure of lib/tst_cmd.c.
+
 1.36 Assert sys or proc file value
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 Using TST_ASSERT_INT/STR(path, val) to assert that integer value or string stored in
diff --git a/include/tst_private.h b/include/tst_private.h
index fe0955f3b..b02f91228 100644
--- a/include/tst_private.h
+++ b/include/tst_private.h
@@ -37,4 +37,13 @@ int tst_get_prefix(const char *ip_str, int is_ipv6);
  */
 char tst_kconfig_get(const char *confname);
 
+/*
+ * If cmd argument is a single command, this function just checks command
+ * whether exists. If not, case skips.
+ * If cmd argument is a complex string ie 'mkfs.ext4 >= 1.43.0', this
+ * function checks command version whether meets this requirement.
+ * If not, case skips.
+ */
+void tst_check_cmd(const char *cmd);
+
 #endif
diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
index cf467b5a0..a19fa22e8 100644
--- a/lib/newlib_tests/.gitignore
+++ b/lib/newlib_tests/.gitignore
@@ -46,4 +46,12 @@ test_macros06
 tst_fuzzy_sync01
 tst_fuzzy_sync02
 tst_fuzzy_sync03
+test_needs_cmds01
+test_needs_cmds02
+test_needs_cmds03
+test_needs_cmds04
+test_needs_cmds05
+test_needs_cmds06
+test_needs_cmds07
+test_needs_cmds08
 test_zero_hugepage
diff --git a/lib/newlib_tests/runtest.sh b/lib/newlib_tests/runtest.sh
index 05c76228b..da01f36d9 100755
--- a/lib/newlib_tests/runtest.sh
+++ b/lib/newlib_tests/runtest.sh
@@ -2,8 +2,9 @@
 # Copyright (c) 2021 Petr Vorel <pvorel@suse.cz>
 
 LTP_C_API_TESTS="${LTP_C_API_TESTS:-test05 test07 test09 test12 test15 test18
-tst_bool_expr test_exec test_timer tst_res_hexd tst_strstatus tst_fuzzy_sync03
-test_zero_hugepage.sh}"
+test_needs_cmds01 test_needs_cmds02 test_needs_cmds03 test_needs_cmds06
+test_needs_cmds07 tst_bool_expr test_exec test_timer tst_res_hexd tst_strstatus
+tst_fuzzy_sync03 test_zero_hugepage.sh}"
 
 LTP_SHELL_API_TESTS="${LTP_SHELL_API_TESTS:-shell/tst_check_driver.sh shell/net/*.sh}"
 
diff --git a/lib/newlib_tests/test_needs_cmds01.c b/lib/newlib_tests/test_needs_cmds01.c
new file mode 100644
index 000000000..92305ee97
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds01.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TPASS, "Testing tst_check_cmd() functionality OK.");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4",
+		"mkfs.ext4 >= 1.0.0",
+		"mkfs.ext4 <= 2.0.0",
+		"mkfs.ext4 != 2.0.0",
+		"mkfs.ext4 > 1.0.0",
+		"mkfs.ext4 < 2.0.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds02.c b/lib/newlib_tests/test_needs_cmds02.c
new file mode 100644
index 000000000..1eeaf6351
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds02.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using non-existing cmd.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Nonexisting command is present!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext45 >= 1.43.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds03.c b/lib/newlib_tests/test_needs_cmds03.c
new file mode 100644
index 000000000..c50077f4e
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds03.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using Illegal operation.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Wrong operator was evaluated!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 ! 1.43.0",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds04.c b/lib/newlib_tests/test_needs_cmds04.c
new file mode 100644
index 000000000..5d05ed46d
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds04.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using incomplete version.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Incomplete version was parsed!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 > 1.43",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds05.c b/lib/newlib_tests/test_needs_cmds05.c
new file mode 100644
index 000000000..f4b509b68
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds05.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format by using version that has garbage.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Garbage version was parsed!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 > 1.43.0-1",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds06.c b/lib/newlib_tests/test_needs_cmds06.c
new file mode 100644
index 000000000..f1234820e
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds06.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test Illegal format with garbage.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Garbage format was parsed!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext4 > 1.43.0 2",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds07.c b/lib/newlib_tests/test_needs_cmds07.c
new file mode 100644
index 000000000..e2d2643f4
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds07.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test non-existed cmd whether still can be detected.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Nonexisting command is present!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.ext45",
+		NULL
+	}
+};
diff --git a/lib/newlib_tests/test_needs_cmds08.c b/lib/newlib_tests/test_needs_cmds08.c
new file mode 100644
index 000000000..acc28d926
--- /dev/null
+++ b/lib/newlib_tests/test_needs_cmds08.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*
+ * Test mkfs.xfs that it doesn't have own parser and table_get function
+ * at the version_parsers structure in lib/tst_cmd.c.
+ * So it should report parser function for this cmd is not implemented.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	tst_res(TFAIL, "Nonexisting parser function for mkfs.xfs is present!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_cmds = (const char *[]) {
+		"mkfs.xfs",
+		"mkfs.xfs >= 4.20.0",
+		NULL
+	}
+};
diff --git a/lib/tst_cmd.c b/lib/tst_cmd.c
index 7446249f9..229848c28 100644
--- a/lib/tst_cmd.c
+++ b/lib/tst_cmd.c
@@ -28,10 +28,21 @@
 #include <signal.h>
 #include "test.h"
 #include "tst_cmd.h"
+#include "tst_private.h"
 
 #define OPEN_MODE	(S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)
 #define OPEN_FLAGS	(O_WRONLY | O_APPEND | O_CREAT)
 
+enum cmd_op {
+	OP_GE, /* >= */
+	OP_GT, /* >  */
+	OP_LE, /* <= */
+	OP_LT, /* <  */
+	OP_EQ, /* == */
+	OP_NE, /* != */
+};
+
+
 int tst_cmd_fds_(void (cleanup_fn)(void),
 		const char *const argv[],
 		int stdout_fd,
@@ -176,3 +187,160 @@ int tst_system(const char *command)
 	signal(SIGCHLD, old_handler);
 	return ret;
 }
+
+static int mkfs_ext4_version_parser(void)
+{
+	FILE *f;
+	int rc, major, minor, patch;
+
+	f = popen("mkfs.ext4 -V 2>&1", "r");
+	if (!f) {
+		tst_resm(TWARN, "Could not run mkfs.ext4 -V 2>&1 cmd");
+		return -1;
+	}
+
+	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
+	pclose(f);
+	if (rc != 3) {
+		tst_resm(TWARN, "Unable to parse mkfs.ext4 version");
+		return -1;
+	}
+
+	return major * 10000 +  minor * 100 + patch;
+}
+
+static int mkfs_ext4_version_table_get(char *version)
+{
+	int major, minor, patch;
+	int len;
+
+	if (sscanf(version, "%u.%u.%u %n", &major, &minor, &patch, &len) != 3) {
+		tst_resm(TWARN, "Illegal version(%s), should use format like 1.43.0", version);
+		return -1;
+	}
+
+	if (len != (int)strlen(version)) {
+		tst_resm(TWARN, "Grabage after version");
+		return -1;
+	}
+
+	return major * 10000 + minor * 100 + patch;
+}
+
+static struct version_parser {
+	const char *cmd;
+	int (*parser)(void);
+	int (*table_get)(char *version);
+} version_parsers[] = {
+	{"mkfs.ext4", mkfs_ext4_version_parser, mkfs_ext4_version_table_get},
+	{},
+};
+
+void tst_check_cmd(const char *cmd)
+{
+	struct version_parser *p;
+	char *cmd_token, *op_token, *version_token, *next, *str;
+	char path[PATH_MAX];
+	char parser_cmd[100];
+	int ver_parser, ver_get;
+	int op_flag = 0;
+
+	strcpy(parser_cmd, cmd);
+
+	cmd_token = strtok_r(parser_cmd, " ", &next);
+	op_token = strtok_r(NULL, " ", &next);
+	version_token = strtok_r(NULL, " ", &next);
+	str = strtok_r(NULL, " ", &next);
+
+	if (tst_get_path(cmd_token, path, sizeof(path)))
+		tst_brkm(TCONF, NULL, "Couldn't find '%s' in $PATH", cmd_token);
+
+	if (!op_token)
+		return;
+
+	if (!strcmp(op_token, ">="))
+		op_flag = OP_GE;
+	else if (!strcmp(op_token, ">"))
+		op_flag = OP_GT;
+	else if (!strcmp(op_token, "<="))
+		op_flag = OP_LE;
+	else if (!strcmp(op_token, "<"))
+		op_flag = OP_LT;
+	else if (!strcmp(op_token, "=="))
+		op_flag = OP_EQ;
+	else if (!strcmp(op_token, "!="))
+		op_flag = OP_NE;
+	else
+		tst_brkm(TCONF, NULL, "Invalid op(%s)", op_token);
+
+	if (!version_token || str) {
+		tst_brkm(TCONF, NULL,
+			"Illegal format(%s), should use format like mkfs.ext4 >= 1.43.0",
+			cmd);
+	}
+
+	for (p = &version_parsers[0]; p->cmd; p++) {
+		if (!strcmp(p->cmd, cmd_token)) {
+			tst_resm(TINFO, "Parsing %s version", p->cmd);
+			break;
+		}
+	}
+
+	if (!p->cmd) {
+		tst_brkm(TBROK, NULL, "No version parser for %s implemented!",
+			cmd_token);
+	}
+
+	ver_parser = p->parser();
+	if (ver_parser < 0)
+		tst_brkm(TBROK, NULL, "Failed to parse %s version", p->cmd);
+
+	ver_get = p->table_get(version_token);
+	if (ver_get < 0)
+		tst_brkm(TBROK, NULL, "Failed to get %s version", p->cmd);
+
+	switch (op_flag) {
+	case OP_GE:
+		if (ver_parser < ver_get) {
+			tst_brkm(TCONF, NULL, "%s required >= %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_GT:
+		if (ver_parser <= ver_get) {
+			tst_brkm(TCONF, NULL, "%s required > %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_LE:
+		if (ver_parser > ver_get) {
+			tst_brkm(TCONF, NULL, "%s required <= %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_LT:
+		if (ver_parser >= ver_get) {
+			tst_brkm(TCONF, NULL, "%s required < %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_EQ:
+		if (ver_parser != ver_get) {
+			tst_brkm(TCONF, NULL, "%s required == %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	case OP_NE:
+		if (ver_parser == ver_get) {
+			tst_brkm(TCONF, NULL, "%s required != %d, but got %d, "
+				"the version is required in order run the test.",
+				cmd, ver_get, ver_parser);
+		}
+		break;
+	}
+}
diff --git a/lib/tst_test.c b/lib/tst_test.c
index a79275722..c49e30f5b 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -29,7 +29,7 @@
 #include "tst_wallclock.h"
 #include "tst_sys_conf.h"
 #include "tst_kconfig.h"
-
+#include "tst_private.h"
 #include "old_resource.h"
 #include "old_device.h"
 #include "old_tmpdir.h"
@@ -987,12 +987,10 @@ static void do_setup(int argc, char *argv[])
 
 	if (tst_test->needs_cmds) {
 		const char *cmd;
-		char path[PATH_MAX];
 		int i;
 
 		for (i = 0; (cmd = tst_test->needs_cmds[i]); ++i)
-			if (tst_get_path(cmd, path, sizeof(path)))
-				tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd);
+			tst_check_cmd(cmd);
 	}
 
 	if (tst_test->needs_drivers) {
-- 
2.23.0


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

[LTP] [PATCH v3 2/2] testcase: make use of needs_cmd version check

[Yang Xu]

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 testcases/kernel/syscalls/quotactl/quotactl04.c | 11 +----------
 testcases/kernel/syscalls/statx/statx05.c       | 13 ++-----------
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/testcases/kernel/syscalls/quotactl/quotactl04.c b/testcases/kernel/syscalls/quotactl/quotactl04.c
index dab61cf4d..62c94d596 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl04.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl04.c
@@ -121,18 +121,9 @@ static void do_mount(const char *source, const char *target,
 
 static void setup(void)
 {
-	FILE *f;
 	const char *const fs_opts[] = {"-I 256", "-O quota,project", NULL};
-	int rc, major, minor, patch;
 
 	test_id = geteuid();
-	f = SAFE_POPEN("mkfs.ext4 -V 2>&1", "r");
-	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
-	if (rc != 3)
-		tst_res(TWARN, "Unable parse version number");
-	else if (major * 10000 + minor * 100 + patch < 14300)
-		tst_brk(TCONF, "Test needs mkfs.ext4 >= 1.43 for quota,project option, test skipped");
-	pclose(f);
 	SAFE_MKFS(tst_device->dev, tst_device->fs_type, fs_opts, NULL);
 	do_mount(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, NULL);
 }
@@ -184,7 +175,7 @@ static struct tst_test test = {
 	.dev_fs_type = "ext4",
 	.mntpoint = MNTPOINT,
 	.needs_cmds = (const char *[]) {
-		"mkfs.ext4",
+		"mkfs.ext4 >= 1.43.0",
 		NULL
 	}
 };
diff --git a/testcases/kernel/syscalls/statx/statx05.c b/testcases/kernel/syscalls/statx/statx05.c
index 86bd3ace1..bdee98c22 100644
--- a/testcases/kernel/syscalls/statx/statx05.c
+++ b/testcases/kernel/syscalls/statx/statx05.c
@@ -87,18 +87,9 @@ static void run(unsigned int i)
 
 static void setup(void)
 {
-	FILE *f;
 	char opt_bsize[32];
 	const char *const fs_opts[] = {"-O encrypt", opt_bsize, NULL};
-	int ret, rc, major, minor, patch;
-
-	f = SAFE_POPEN("mkfs.ext4 -V 2>&1", "r");
-	rc = fscanf(f, "mke2fs %d.%d.%d", &major, &minor, &patch);
-	if (rc != 3)
-		tst_res(TWARN, "Unable parse version number");
-	else if (major * 10000 + minor * 100 + patch < 14300)
-		tst_brk(TCONF, "Test needs mkfs.ext4 >= 1.43 for encrypt option, test skipped");
-	pclose(f);
+	int ret;
 
 	snprintf(opt_bsize, sizeof(opt_bsize), "-b %i", getpagesize());
 
@@ -132,7 +123,7 @@ static struct tst_test test = {
 	.mntpoint = MNTPOINT,
 	.dev_fs_type = "ext4",
 	.needs_cmds = (const char *[]) {
-		"mkfs.ext4",
+		"mkfs.ext4 >= 1.43.0",
 		"e4crypt",
 		NULL
 	}
-- 
2.23.0


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

Re: [LTP] [PATCH v3 2/2] testcase: make use of needs_cmd version check

[Petr Vorel]

Hi Xu,

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

>  testcases/kernel/syscalls/quotactl/quotactl04.c | 11 +----------
>  testcases/kernel/syscalls/statx/statx05.c       | 13 ++-----------
>  2 files changed, 3 insertions(+), 21 deletions(-)

NOTE: you could also remove #include <stdio.h> from both files,
it was needed only for popen() pclose() and fscanf()

Kind regards,
Petr

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

Re: [LTP] [PATCH v3 2/2] testcase: make use of needs_cmd version check

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
> Reviewed-by: Petr Vorel<pvorel@suse.cz>
>
>>   testcases/kernel/syscalls/quotactl/quotactl04.c | 11 +----------
>>   testcases/kernel/syscalls/statx/statx05.c       | 13 ++-----------
>>   2 files changed, 3 insertions(+), 21 deletions(-)
>
> NOTE: you could also remove #include<stdio.h>  from both files,
> it was needed only for popen() pclose() and fscanf()
Good catch, I also remove useless tst_safe_stdio.h since we remove 
version check code. But statx05.c still use snprintf so I keep stdio.h 
in there.

I merged this patch, thanks for you and cyril's reivew.

Best Regards
Yang Xu
>
> Kind regards,
> Petr

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

Re: [LTP] [PATCH v3 1/2] library: add cmd check handler in needs_cmds

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v3 1/2] library: add cmd check handler in needs_cmds

[Petr Vorel]

Hi Xu,

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

Very nice code, nice tests, thanks!

> diff --git a/lib/newlib_tests/test_needs_cmds01.c b/lib/newlib_tests/test_needs_cmds01.c
> new file mode 100644
> index 000000000..92305ee97
> --- /dev/null
> +++ b/lib/newlib_tests/test_needs_cmds01.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +#include <stdio.h>
All <stdio.h> in the tests aren't needed, please remove it before merge.

nit: I'd also call tests tst_needs_cmds*.c (tst_ prefix instead of test_) as
most of the tests in the library takes this scheme, but that's very minor.

Kind regards,
Petr

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

Re: [LTP] [PATCH v3 1/2] library: add cmd check handler in needs_cmds

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
> Reviewed-by: Petr Vorel<pvorel@suse.cz>
>
> Very nice code, nice tests, thanks!
>
>> diff --git a/lib/newlib_tests/test_needs_cmds01.c b/lib/newlib_tests/test_needs_cmds01.c
>> new file mode 100644
>> index 000000000..92305ee97
>> --- /dev/null
>> +++ b/lib/newlib_tests/test_needs_cmds01.c
>> @@ -0,0 +1,25 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2021 Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +#include<stdio.h>
> All<stdio.h>  in the tests aren't needed, please remove it before merge.
I remove stdio.h for these lib test cases and add stdio.h in tst_cmd.c 
since there used popen() function.
>
> nit: I'd also call tests tst_needs_cmds*.c (tst_ prefix instead of test_) as
> most of the tests in the library takes this scheme, but that's very minor.
I have renamed to tst_needs_cmds*.c and merged.

Best Regards
Yang Xu
>
> Kind regards,
> Petr

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