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

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

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