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

Petr Vorel
2021-12-08T17:06:08Z

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