Re: [PATCH v2 14/19] perf test attr: State, spelled out in a line, why a test is skipped

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf test: TypeError and IndexError if kernel_since or kernel_until has one component
--

commit 21b41941c9b2a0fd4a3bf8744960357d34dbae9e
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf test attr: State, spelled out in a line, why a test is skipped

This patch updates the test skipping logic in the perf test attribute scripts
to return descriptive strings instead of boolean values. This enables the
test suite to explicitly log why a test is skipped instead of just reporting
a generic skip.

> diff --git a/tools/perf/tests/shell/lib/attr.py b/tools/perf/tests/shell/lib/attr.py
> index 3bf07d4df9598..ee78c912d9469 100644
> --- a/tools/perf/tests/shell/lib/attr.py
> +++ b/tools/perf/tests/shell/lib/attr.py
> @@ -425,13 +428,17 @@ class Test(object):
>  
>      def skip_test_kernel_since(self):
>          if not self.kernel_since:
> -            return False
> -        return not self.kernel_since <= parse_version(platform.release())
> +            return None
> +        if self.kernel_since <= parse_version(platform.release()):
> +            return None
> +        return "kernel older than %d.%d" % tuple(self.kernel_since)

[Severity: Medium]
Does this code raise a TypeError if a test specifies a single integer for
kernel_since (for example, kernel_since = 6)?

Prior to this patch, Python's native list comparison safely handled 1-element
vs 2-element list comparisons without crashing. It looks like formatting a
variable-length tuple with a fixed 2-element format string %d.%d will cause
a TypeError for 1-element lists.

>  
>      def skip_test_kernel_until(self):
>          if not self.kernel_until:
> -            return False
> -        return not parse_version(platform.release()) < self.kernel_until
> +            return None
> +        if parse_version(platform.release()) < self.kernel_until:
> +            return None
> +        return "kernel newer than %d.%d" % tuple(self.kernel_until)

[Severity: Medium]
Will this formatting also trigger a TypeError if kernel_until contains only
one component?

[ ... ]

> @@ -529,20 +537,36 @@ class Test(object):
>      def run_cmd(self, tempdir):
>          _junk1, _junk2, _junk3, _junk4, myarch = (os.uname())
>  
[ ... ]
> +        kernel = self.skip_test_kernel_since()
> +        if kernel:
> +            log.debug("  detail: the test file's kernel_since = %d.%d, this kernel is %s",
> +                     self.kernel_since[0], self.kernel_since[1], platform.release())

[Severity: Medium]
Could directly accessing index 1 via self.kernel_since[1] cause an IndexError
when the list only contains a major version?

> +            raise Notest(self, kernel)
> +
> +        kernel = self.skip_test_kernel_until()
> +        if kernel:
> +            log.debug("  detail: the test file's kernel_until = %d.%d, this kernel is %s",
> +                     self.kernel_until[0], self.kernel_until[1], platform.release())

[Severity: Medium]
Similarly, does accessing self.kernel_until[1] raise an IndexError if the test
specifies only a single-component version?

> +            raise Notest(self, kernel)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013201415.36428-1-acme@kernel.org?part=14