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

On 7/29/26 3:36 PM, Petr Vorel wrote:
> Hi Avinesh,
> 
>> Hi Petr,
> 
>> few comments below
> Thanks!
> 
>> On 7/29/26 11:57 AM, Petr Vorel wrote:
>>> Signed-off-by: Petr Vorel <[email protected]>
>>> ---
>>> The same as v1.
> 
>>>    include/tst_test.h |  9 ++++++++-
>>>    lib/tst_test.c     | 39 +++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 47 insertions(+), 1 deletion(-)
> 
>>> diff --git a/include/tst_test.h b/include/tst_test.h
>>> index c69362485e..0910660827 100644
>>> --- a/include/tst_test.h
>>> +++ b/include/tst_test.h
>>> @@ -280,6 +280,9 @@ struct tst_ulimit_val {
>>>     *
>>>     * @min_kver: A minimum kernel version supporting the filesystem which has been
>>>     *            created with mkfs.
>>> + *
>>> + * @max_kver: A maximum kernel version supporting the filesystem which has been
>>> + *            created with mkfs.
>>>     */
>>>    struct tst_fs {
>>>    	const char *type;
>>> @@ -292,6 +295,7 @@ struct tst_fs {
>>>    	const void *mnt_data;
>>>    	const char *min_kver;
>>> +	const char *max_kver;
>>>    };
>>>    /**
>>> @@ -301,7 +305,9 @@ struct tst_fs {
>>>     *        and each time passed an increasing counter value.
>>>     * @options: An NULL optstr terminated array of struct tst_option.
>>>     *
>>> - * @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.
> 
>>> + *
>>> + * @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 ?

sorry, haven't tested yet.

> ...
> 
> Sublevel can be above 255 since 4.4.256
> https://lwn.net/ml/linux-kernel/[email protected]/
> 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.

> 
>> 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.
> 
> Kind regards,
> Petr
> ...
> 
>> Thanks,
>> Avinesh


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