Re: [LTP] [PATCH v4 4/9] tst_supported_fs: Support skip list when query single fs

[Martin Doucha <mdoucha@suse.cz>]

On 09. 09. 22 13:33, Petr Vorel wrote:
>> Hi!
>>>>> +++ b/lib/tst_supported_fs_types.c
>>>>> @@ -134,6 +134,24 @@ enum tst_fs_impl tst_fs_is_supported(const char *fs_type)
>>>>>   	return TST_FS_UNSUPPORTED;
>>>>>   }
> 
>>>>> +enum tst_fs_impl tst_fs_is_supported_skiplist(const char *fs_type, const char
>>>>> +					      *const *skiplist)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = tst_fs_is_supported(fs_type);
>>>>> +
>>>>> +	if (!ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (tst_fs_in_skiplist(fs_type, skiplist)) {
>>>>> +		tst_brk(TCONF, "%s is not supported by the test",
>>>>> +			fs_type);
> 
>>>> I would expect to return TST_FS_UNSUPPORTED here. It's a bit hacky
>>>> solution to exit a library call with tst_brk(TCONF, "");

I agree with that. Return a status code from 
tst_fs_is_supported_skiplist() and raise TCONF somewhere higher in the 
call chain.

>>> +++ b/include/tst_fs.h
>>> @@ -171,7 +171,8 @@ int tst_fill_file(const char *path, char pattern, size_t bs, size_t bcount);
>>>   int tst_prealloc_file(const char *path, size_t bs, size_t bcount);
> 
>>>   enum tst_fs_impl {
>>> -       TST_FS_UNSUPPORTED = 0,
>>> +       TST_FS_UNSUPPORTED = -2,
>>> +       TST_FS_SKIPPED_BY_TEST = -1,
> 
>> Maybe just TST_FS_SKIPPED
> 
>>>          TST_FS_KERNEL = 1,
>>>          TST_FS_FUSE = 2,
>>>   };
> 
>> I wonder though if we really care about the difference between
>> unsupported and skipped. Maybe just print TINFO message in case of the
>> skipped filesystem and return zero in both cases? And maybe rename
>> TST_FS_UNSUPPORTED to TST_FSNOT_USED.
> 
> I thought being precise why there is TCONF would help, but users will just read
> previous TINFO messages.
> 
> Having just single "error" enum constant, I'm OK to keep TST_FS_UNSUPPORTED
> (unless anybody really think renaming it to TST_FS_NOT_USED would be better).

I'd like to see why exactly a filesystem was skipped so I prefer two 
enum values. Though I'd name them TST_FS_UNAVAILABLE (missing mkfs or 
kernel support) and TST_FS_UNSUPPORTED (disabled by test skiplist).

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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