This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: net: adopt harness for flow label mgr
This converts tools/testing/selftests/net/ipv6_flowlabel_mgr.c from the
local expect_pass/expect_fail/explain helpers to kselftest_harness.h,
splitting run_tests() into individual TEST() cases. It also removes
parse_opts(), so the two previously -l gated checks now always run.
> diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> index af87eec799c80..482921f7ee119 100644
> --- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> +++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
[ ... ]
> @@ -159,230 +131,335 @@ static void tcp_connect(int listener, uint32_t
> flowlabel, int *client, int *acce
[ ... ]
> pid = fork();
> - if (pid == -1)
> - error(1, errno, "fork");
> + ASSERT_NE(-1, pid) TH_LOG("fork failed");
> if (!pid) {
> - expect_pass(flowlabel_get(fd, 3, IPV6_FL_S_USER, 0));
> - if (setuid(USHRT_MAX))
> + err = flowlabel_get(fd, 3, IPV6_FL_S_USER, 0);
> + ASSERT_TRUE(!err) TH_LOG("child failed to get the user-private
> label");
> +
> + if (setuid(USHRT_MAX)) {
> fprintf(stderr, "[INFO] skip setuid child test\n");
> - else
> - expect_fail(flowlabel_get(fd, 3, IPV6_FL_S_USER, 0));
> + exit(0);
> + }
[Severity: Medium]
Should this path report a skip rather than a pass? In
TEST(user_private_label_share), when setuid(USHRT_MAX) fails the child
prints to stderr and calls exit(0).
The parent then runs:
ASSERT_EQ(pid, wait(&wstatus)) TH_LOG("wait failed");
ASSERT_TRUE(WIFEXITED(wstatus)) TH_LOG("child did not exit normally");
ASSERT_EQ(0, WEXITSTATUS(wstatus)) TH_LOG("child reported unexpected
result");
All three succeed, so the harness emits "ok user_private_label_share" even
though the check the test is named for, that an IPV6_FL_S_USER label cannot
be obtained after switching uid, never executed.
setuid(USHRT_MAX) fails when the binary runs without CAP_SETUID, or in a
user namespace where uid 65535 is unmapped, which is reachable since
ipv6_flowlabel.sh drives this through ./in_netns.sh.
Would propagating KSFT_SKIP work better here, for example the child doing
exit(KSFT_SKIP) and the parent turning that into SKIP(return, ...)?
The silent-skip logic itself predates this patch, but before the conversion
there were no per-test TAP results; now the test individually reports "ok"
instead of "# SKIP".
The raw fprintf(stderr, "[INFO] ...") is also the only diagnostic left in
the converted file that does not go through TH_LOG, which prefixes output
with "# ".
[ ... ]
> +TEST(reflect_flag)
> +{
[ ... ]
> + if (flowlabel_consistency_enabled())
> + SKIP(return,
> + "net.ipv6.flowlabel_consistency must be 0 (run via
> ipv6_flowlabel.sh)");
This is the idiom the setuid path above could use: SKIP() sets
_metadata->exit_code = KSFT_SKIP and prints a "# SKIP" diagnostic, so an
unmet precondition is not reported as a pass. Could both unmet-precondition
sites in this file use it consistently?
[ ... ]
