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

xuyang2018.jy
2021-12-09T01:31:28Z

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