[LTP] [PATCH 1/2] lib: add cmd parse handler in needs_cmd
xuyang2018.jy@fujitsu.com
xuyang2018.jy@fujitsu.com
Tue Dec 7 02:20:26 CET 2021
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
>
More information about the ltp
mailing list