Re: [LTP] tst_kvercmp: Factor out error handling

[Petr Vorel <pvorel@suse.cz>]

Hi all,

> Hi Petr,

> On Aug 5, 2026, Petr Vorel wrote:
> > tst_kvercmp: Factor out error handling

> --- [PATCH 1/9] ---

> > static int _tst_parse_kver(const char *str_kver, int *v1, int *v2, int *v3)

> Could this be renamed to parse_kver? File-scope identifiers beginning with an
> underscore are reserved by C, and LTP's coding rules prohibit such names.

I thought this is wrong, because we have functions with underscore, but it's
trailing underscore, not the leading one.

=> I'll rename to tst_parse_kver_().

Also, agent does not know about LTP rule to have functions in lib with leading
tst_.

> --- [PATCH 2/9] ---

> > This is a preparation for struct tst_test max_kver member in the next
> > commit.

> Could the motivation be stated without referring to the next patch? Each
> commit message should stand on its own rather than depend on series position.

Well, we try to split code into commits to be easily revieweable. With that
sometimes is hard to avoid referring to commits. Preparation for other changes
separated into it's own commit helps is an explanation itself.

=> Consider as irrelevant.

> --- [PATCH 4/9] ---

> > 	if (TST_RET != -1) {
> > 		tst_res(TFAIL, "creat() succeeded unexpectedly");
> > 		return;
> > 	}

> Could this path kill and reap pid before returning? Removing the version gate
> makes it reachable on affected 6.11 and 6.12 kernels, but the child remains
> paused indefinitely. The framework then blocks in tst_reap_children() until
> the watchdog terminates the test instead of returning the intended TFAIL.

=> Indeed, this is bug for creat07.c, at least on SLE16 kernel which is based on 6.12,
where test hangs until timeouts on:

creat07.c:43: TFAIL: creat() succeeded unexpectedly

I guess I'll simply use goto to do the needed cleanup:

+++ testcases/kernel/syscalls/creat/creat07.c
@@ -41,7 +41,7 @@ static void verify_creat(void)

        if (TST_RET != -1) {
                tst_res(TFAIL, "creat() succeeded unexpectedly");
-               return;
+               goto kill;
        }

        if (TST_ERR == ETXTBSY)
@@ -49,6 +49,7 @@ static void verify_creat(void)
        else
                tst_res(TFAIL | TTERRNO, "creat() failed unexpectedly");

+kill:
        SAFE_KILL(pid, SIGKILL);
        SAFE_WAITPID(pid, NULL, 0);
 }
---

execve04.c simply fails on that kernel which is IMHO correct (not hiding bugs):
execve_child.c:27: TFAIL: execve_child shouldn't be executed

> > 	TEST(execve(TEST_APP, argv, environ));

> How is the writer child released when execve() succeeds on an affected
> kernel? The test process is replaced by execve_child, which reports TFAIL and
> exits, while the writer remains blocked in TST_CHECKPOINT_WAKE_AND_WAIT(0) and
> becomes an orphan.

I hope the above patch fixes that. Shouldn't be this fixed separately
(regardless on a removed .min_kver), it can be problematic on any kernel
version.

> --- [PATCH 5/9] ---

> > 	.max_kver = "7.1",

> Could the test keep running and only adjust the expected result of
> FAN_REPORT_PIDFD | FAN_REPORT_TID on kernels supporting thread pidfds? This
> test-wide limit also removes the
> FAN_REPORT_PIDFD | FAN_REPORT_FID | FAN_REPORT_DFID_NAME case on 7.2 and all
> newer kernels, and that exact combination is not tested elsewhere in LTP.

No, we agreed, this is ok (I'm explaining this second time and I suppose I will
have to do it on all patchset versions I'll send).

> --- [PATCH 6/9] ---

> > 	.min_kver = "4.4",
> > 	.max_kver = "5.0",

> How can this exercise do_test() on current CI kernels? The current kernel must
> lie between the bounds; min_kver being less than max_kver is not sufficient.
> With this maximum, including on stable 7.2, the self-test only reports TCONF
> and never tests the new success path.

I deliberately wanted to get TCONF on the current kernel, i.e. the fact than
using both will result in TCONF not in TBROK. Also, it does not matter if the
test TCONF due old version or run and then TPASS, it's about testing library
setup() run with .min_kver and .max_kver (test itself does nothing). Also any
kernel version will sooner later be old and thus TCONF, why not to do it now?
Not it's also a test that any old version will just TCONF not TBROK. I try to
document it better in the test.

> --- [PATCH 7/9] ---

> > int tst_kver_cmp(int a1, int a2, int a3, int b1, int b2, int b3);

> Could the new public function's kernel-doc include a Return: section
> describing the comparison result?

Commit will be removed in the next version.

> > This will be heavily used in metaparse.c (speedup of metadata
> > generation) in the next commit.

> Could this motivation be stated without referring to the next patch? Each
> commit message should stand on its own.

> --- [PATCH 8/9] ---

> > Will be used for metadata.c in the next commit.

> Could the commit message instead explain why host targets need access to
> MAKE_DEPS without depending on a later patch?

Commit will be removed in the next version.

> --- [PATCH 9/9] ---

> > metaparse: HOST_LDLIBS		+= -lltp

> How can this work for a cross-compilation? metaparse is built with HOSTCC, but
> -lltp resolves to lib/libltp.a built with the target CC, so the host linker
> cannot consume the archive. Could the version helper be compiled with HOSTCC,
> or moved into an architecture-independent host library?

Commit will be removed in the next version.

> Verdict - Needs revision

> ---
> Note:

> The agent can sometimes produce false positives although often its
> findings are genuine. If you find issues with the review, please
> comment this email or ignore the suggestions.

> Regards,
> LTP AI Reviewer

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