This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net-next 1/4] selftests: net: test IPV6_FL_A_RENEW ---
commit e6f2968aee0cf7d9f88007bffba4ffec657d1cd4
Author: Marcelo Mendes Spessoto Junior <[email protected]>
selftests: net: test IPV6_FL_A_RENEW
This adds a flowlabel_renew() helper wrapping IPV6_FLOWLABEL_MGR with
IPV6_FL_A_RENEW, and a new sub-test in run_tests() that creates an
exclusive label, renews it with a longer linger, releases it, sleeps past
the default linger, and expects re-creation to fail.
> diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> index af95b48acea90..cfa7e6270994a 100644
> --- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> +++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
[ ... ]
> @@ -160,6 +171,18 @@ static void run_tests(int fd)
> error(1, errno, "wait");
> if (!WIFEXITED(wstatus) || WEXITSTATUS(wstatus) != 0)
> error(1, errno, "wait: unexpected child result");
> +
> + if (cfg_long_running) {
[Severity: Medium]
Does the new test ever run in an automated pass? The whole block,
including the only call site of the new flowlabel_renew() helper, sits
inside if (cfg_long_running), and cfg_long_running is only set by the '-l'
option in parse_opts():
case 'l':
cfg_long_running = true;
break;
The only in-tree invocation of this binary is in
tools/testing/selftests/net/ipv6_flowlabel.sh, which passes no arguments:
echo "TEST management"
./in_netns.sh ./ipv6_flowlabel_mgr
So under make run_tests the fl6_renew() path is not exercised, while the
changelog says "RENEW was the only flow label action without selftests
coverage". Should the sub-test be unconditional, or should
ipv6_flowlabel.sh pass '-l'?
A sleep-free variant seems possible: renew of a label held by the socket
returns 0, renew of an unknown label returns -ESRCH, and renew with
linger > FL_MAX_LINGER without CAP_NET_ADMIN returns -EPERM.
> + explain("create a new label with FL_MIN_LINGER linger time");
> + expect_pass(flowlabel_get(fd, 5, IPV6_FL_S_EXCL,
> IPV6_FL_F_CREATE));
> + explain("renew the label to increase its linger time and put
> it");
> + expect_pass(flowlabel_renew(fd, 5, 2 * (FL_MIN_LINGER * 2 +
> 1)));
> + expect_pass(flowlabel_put(fd, 5));
> + sleep(FL_MIN_LINGER * 2 + 1);
> + explain("The label cannot be created because the new linger
> time is not over yet");
> + expect_fail(flowlabel_get(fd, 5, IPV6_FL_S_ANY,
> IPV6_FL_F_CREATE));
[Severity: Low]
This isn't a bug, but would it be worth bounding the renewed linger from
both sides? The sequence only checks the negative case, and the label was
created with IPV6_FL_S_EXCL, so in net/ipv6/ip6_flowlabel.c the create at
t=13s fails as long as the entry is still hashed:
net/ipv6/ip6_flowlabel.c:ipv6_flowlabel_get() {
...
if (fl1->share == IPV6_FL_S_EXCL ||
fl1->share != fl->share ||
...
goto release;
}
Since expect_fail() cannot inspect errno, an "entry still present"
failure is indistinguishable from a share/permission failure, and no upper
bound on the renewed lifetime is tested. A kernel change that made a
renewed label linger forever, or that clamped linger/expires too high in
fl6_renew(), would still pass. Adding a second sleep past the renewed 26s
followed by expect_pass(flowlabel_get(fd, 5, ...)) would pin both ends.
The RENEW error paths are also untested: -ESRCH for an unknown label,
-EPERM for linger above FL_MAX_LINGER without CAP_NET_ADMIN, and the
IPV6_FL_S_NONE plus CAP_NET_ADMIN lookup branch in
ipv6_flowlabel_renew().
> + }
> +
> }
[Severity: Low]
There is a stray blank line between the closing brace of the new
if (cfg_long_running) block and the closing brace of run_tests().
scripts/checkpatch.pl reports:
CHECK: Blank lines aren't necessary before a close brace '}'
>
> static void parse_opts(int argc, char **argv)
[Severity: Low]
The From: and Signed-off-by: identities use different addresses:
Author: Marcelo Mendes Spessoto Junior <[email protected]>
Signed-off-by: Marcelo Mendes Spessoto Junior <[email protected]>
checkpatch reports WARNING: FROM_SIGN_OFF_MISMATCH for this. Could the
two be made to match on resend?
--
pw-bot: cr
