Matthieu Baerts <[email protected]> writes:
> Hi Jakub, Petr, > > On 13/06/2025 18:57, Jakub Kicinski wrote: >> On Thu, 12 Jun 2025 22:10:48 +0200 Petr Machata wrote: >>> Add tests for MC-routing underlay VXLAN traffic. >>> >>> Signed-off-by: Petr Machata <[email protected]> >>> --- >>> >>> Notes: >>> v2: >>> - Adjust as per shellcheck citations >> >> Noob question - would we also be able to squash the unreachable code >> warnings if we declared ALL_TESTS as an array instead of a string? >> IDK if there's any trick we could use to make shellcheck stop >> complaining. Not blocking the series, obviously. >> >> CC Matthieu, I presume you may have already investigated this :) > > Thank you for the Cc. Yes indeed, I already had this case. > > I don't think declaring ALL_TESTS as an array would help for this case > -- even if it looks clearer than a long string -- because I think > shellcheck will simply check if all the different functions are called > directly. As mentioned in Shellcheck wiki [1]: "ShellCheck may > incorrectly believe that code is unreachable if it's invoked by variable > name or in a trap. In such a case, please Ignore the message". > > That what we did with MPTCP, see the top of the mptcp_join.sh file for > example [2], where we have: > >> # ShellCheck incorrectly believes that most of the code here is unreachable >> # because it's invoked by variable name, see how the "tests" array is used >> #shellcheck disable=SC2317 > > If you add this at the top of your new file, followed by an empty line, > shellcheck will ignore this issue for the whole file. The ALL_TESTS issue is not the end of it either. We use helpers that call stuff indirectly all over the place. defer, in_ns... It would make sense to me to just disable SC2317 in NIPA runs. Or maybe even put it in net/forwarding/.shellcheckrc. Pretty much all those tests are written in a style that will hit these issues. > Note: regarding the other issue you have: > >> In vxlan_bridge_1q_mc_ul.sh line 766: >> setup_wait >> ^--------^ SC2119 (info): Use setup_wait "$@" if function's $1 should mean >> script's $1. > > I guess you can also ignore it, or use "" as argument. If you ignore it > -- which looks cleaner -- I think it is always good to add a comment, e.g. > >> # shellcheck disable=SC2119 # arguments are optional, not needed here. >> setup_wait I'll just pass "$NUM_NETIFS" to get rid of the warning. I really don't like these shellcheck annotations. I'll send a patch that changes the calling convention so that SC doesn't get triggered, because I really don't want every new selftest to have to deal with this.
