Re: [LTP] [PATCH v2 2/5] lib: Add support for max_kver to struct tst_test and tst_fs

[Petr Vorel <pvorel@suse.cz>]

Hi Avinesh,

> > > > - * @min_kver: A minimal kernel version the test can run on. e.g. "3.10".
> > > > + * @min_kver: A minimal kernel version the test can run on. e.g. "4.4".
> > > s/minimal/minimum

> > You spotted more grammar errors so I trust you :). But can you please double
> > check that it'đ really correct? Minimal is an adjective, minimum is an adjective
> > but also a noun and an adverb.

> > When googling I even found use of both in the same document :)
> > https://kubernetes.io/docs/reference/node/kernel-version-requirements/
> > "have minimum kernel version requirements"
> > "minimal kernel version requirement"
> I see we are using minimal in many places already. I think both wordings
> are correct. Just that 'maximal kernel version' sounded a bit odd to me.
> So please ignore my comment here.

Np.

> > > > + *
> > > > + * @max_kver: A maximal kernel version the test can run on. e.g. "7.2".
> > > s/maximal/maximum
> > dtto

> > > Also, I assume test will *not be skipped* on kernel versions
> > > 7.1.1, 7.1.2 etc just with
> > > .max_kver="7.1"

> > Good point, thanks => v3 (sigh).

> > check_max_kver() needs to have this condition to take an account the sublevel.

> > -if (tst_kvercmp(v1, v2, v3) > 0) {
> > +if (tst_kvercmp(v1, v2, v3) >= 1024) {
> > 		msg = "The test requires kernel %s or older";
> And will this be fine with -
> .max_kver="7.1.5"
> on SUT with v7.2.0 ?

Indeed, this will not be skipped. The reason is that for .max_kver="7.1"
(e.g. without the 3rd number) we want to behave like 7.1.1023 (i.e. 3rd number
being the highest possible number so that any 7.1 version is ok, which is
achieved by > 1024 instead of 0). If we want also .max_kver="7.1.5" compare
as is (i.e. 7.1.5), it should be something like (untested):

if (tst_kvercmp(v1, v2, v3) > (!v3 ? 1023 : 0)) {
	msg = "The test requires kernel %s or older";

And that behavior would have to be for sure well documented.
WDYT?

> sorry, haven't tested yet.

> > ...

> > Sublevel can be above 255 since 4.4.256
> > https://lwn.net/ml/linux-kernel/20210208145805.898658055@linuxfoundation.org/
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9b82f13e7ef3

> > But it should be below 1024 (practically the highest sublevel has been 337).

> > Kind regards,
> > Petr

> > > maybe we should clarify.

> > > >     *
> > > >     * @supported_archs: A NULL terminated array of architectures the test runs on
> > > >     *                   e.g. {"x86_64, "x86", NULL}. Calls tst_is_on_arch() to
> > > > @@ -551,6 +557,7 @@ struct tst_fs {
> > > >    	struct tst_option *options;
> > > >    	const char *min_kver;
> > > > +	const char *max_kver;
> > > >    	const char *const *supported_archs;
> > > > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > > > index 778a1fed40..fa38b3a7a3 100644
> > > > --- a/lib/tst_test.c
> > > > +++ b/lib/tst_test.c
> > > > @@ -1087,6 +1087,36 @@ static bool check_min_kver(const char *min_kver, const int brk_nosupp)
> > > >    	return true;
> > > >    }
> > > > +/*
> > > > + * Check for the maximal required kernel version.
> > > > + *
> > > > + * return: true if the kernel version is low enough, false otherwise.

> > > This sounds a bit confusing. How about-

> > > Check the running kernel against the last version the test applies to.

> > Makes sense. Based on check_min_kver(), which was renamed from check_kver() in
> > the previous commit. Could you have look on these as well. Anyway, if you have
> > time, having a look on docs wording as a separate effort would help.
> sure, I will see if I find something worth reporting.

+1


> > > return: true if the kernel is old enough, false otherwise.

> > +1

> > > > + */
> > > > +static bool check_max_kver(const char *max_kver, const int brk_nosupp)
> > > > +{
> > > > +	char *msg;
> > > > +	int v1, v2, v3;
> > > > +
> > > > +	if (tst_parse_kver(max_kver, &v1, &v2, &v3)) {
> > > > +		tst_res(TWARN,
> > > > +			"Invalid kernel version %s, expected %%d.%%d.%%d",
> > > I wouldn't say 'kernel version is invalid'. maybe test isn't
> > > supported/applicable on..

> > tst_parse_kver() returns 1 on error => this is really an error check.
> > Other thing is that the same error handling is on both places, but that's very
> > minor.
> sorry, my bad. I misunderstood this. Please ignore.

np.

Kind regards,
Petr

> > Kind regards,
> > Petr
> > ...

> > > Thanks,
> > > Avinesh


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