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

[Petr Vorel <pvorel@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.

Martin, thanks for your input! Done in v5 (just sent).

> > > > +++ 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).

OK, this is not in v5. I don't mind to send v6 on Monday, if we agree this
is wanted. Implementation detail: should I use for -2 and -1 for these two values?
(code < 0 would be error, code > 0 would be OK, 0 itself skipped)

Kind regards,
Petr

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