Re: [PATCH net v3 3/3] selftests: net: hsr: add shared-mutation regression test

[Xin Xie <xiexinet@gmail.com>]

Dispositions for the AI-review findings on this patch (the review is
published on the web only, not mailed to the list):

> The header calls this subtest "F2 (path/LAN ID)" and the changelog
> says "Add regression coverage for both shared-data corruptions",
> but is any path ID actually checked anywhere in the script?
>
> This link is created with proto 1, so only the PRP path is
> exercised, and the checks below only decode the PRP RCT LAN-ID
> nibble.  That covers prp_create_tagged_frame()'s frame->skb_prp
> branch.
>
> The sibling branches in hsr_create_tagged_frame() are not touched
> by either subtest: [...]
>
> Would it be worth adding an HSR-tagged (proto 0, version 1) case
> with captures on the slave peers so those branches are covered too?

The analysis is correct: only the PRP RCT branch is exercised.  Note
the F1 link is HSR_V0, which assigns the same path ID to both slaves,
so a path-ID check would indeed need a version-1 link as suggested;
the NETIF_F_HW_HSR_TAG_INS branches additionally need offload
hardware that a veth topology does not provide.

> Every other capability this script needs is probed and turned into
> a skip: ip/tc/python3 via require(), sch_netem via the tc qdisc
> probe, and HSR/PRP plus HSR RedBox via the ip link add probes.
> AF_PACKET is the exception.
>
> tools/testing/selftests/net/hsr/config lists only: [...]
>
> and CONFIG_PACKET in net/packet/Kconfig is a plain tristate with no
> default y.  On a kernel built from this fragment,
> socket(AF_PACKET, ...) raises OSError(EAFNOSUPPORT), python3 exits
> 1, run_f2() returns 1 and the merge reports a hard FAIL rather than
> a skip.
>
> Should CONFIG_PACKET be added to the hsr config fragment?

Correct.  In practice a kselftest-merge picks up CONFIG_PACKET=y
from other net selftests' fragments, so this only bites on a kernel
built from the hsr fragment alone.

> The capture loop above also exits when the four second deadline
> expires, leaving i_src as None.  Since None != RB is true, does
> that make a missed vIp capture report as "interlink did not carry
> the RedBox MAC"?
>
> The m_src check that follows is the assertion this subtest exists
> for [...] and it is skipped because the i_src check already called
> sys.exit(1).  [...]
>
> Could F1 get an equivalent explicit check for m_src is None / i_src
> is None before the value comparisons, so a timeout is
> distinguishable from a wrong MAC?

Yes, the subtest still fails in that case, but the printed reason is
misleading and the primary assertion never runs; an explicit None
check would separate the two outcomes.

> The changelog says the results are merged "so packet-socket
> pressure or a skip cannot hide a failure".  Does the merge hold for
> statuses that are not one of the kselftest constants?
>
> Both run_f2() and run_f1() end with nsx python3 /dev/stdin, so ret
> is the raw exit status of ip netns exec python3.  A signal-killed
> interpreter gives 128+N (137 for a SIGKILL/OOM, 139 for SIGSEGV),
> and an ip netns exec failure gives 255.
>
> ksft_status_merge() only ranks four values: [...]
>
> With ret=137, ${weights[137]} expands to the empty string, which
> bash evaluates as 0 inside [[ ]], so 0 -ge 0 succeeds and the
> function echoes a, i.e. 0.  rc stays 0, the branch above prints
> "[ OK ]" and exit "$rc" returns 0 for a subtest that was killed
> mid-run.  [...]
>
> Would normalising ret to $ksft_fail for anything outside
> 0/$ksft_xfail/$ksft_skip/$ksft_fail before merging address this?

Yes, normalising ret to the ksft constants before merging addresses
it.  This only affects abnormal exits; normal outcomes always return
the ksft codes, which merge correctly.

All four are test-code remarks; none changes the verdict direction on
the configurations the test runs (it fails on the unfixed kernel and
passes with the series), so no respin is planned for them.

-- 
Xin