Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 (2) · Low: 0
- [Medium] Commit message / diff mismatch: the changelog of "selftests:
openvswitch: fix misc pylint warnings in ovs-dpctl.py" lists "W0702:…
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the loop
this patch rewrites hides it: OvsDatapath.create()…
- [Medium] This problem wasn't introduced by this patch, but: the UFID
codec in ovs-dpctl.py neither encodes nor prints the requested UUID…
--
selftests: openvswitch: fix misc pylint warnings in ovs-dpctl.py
This patch applies a set of mechanical pylint cleanups to
tools/testing/selftests/net/openvswitch/ovs-dpctl.py: dropping an unused
import, flattening elif-after-return, dropping superfluous parentheses and
lambdas, and renaming or removing unused variables. No behaviour change is
intended.
A few questions below, mostly about the changelog and about two spots the
cleanups touch or pass over.
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 9cd0d8f0ab233..6a02810fe4eaf 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
[ ... ]
> - W0611: remove unused import struct
> - W0702: replace bare except with except Exception
[Severity: Medium]
Is this W0702 line accurate for this patch? I don't see a hunk here that
touches a bare except. At this commit the handler in ovsactions.dpstr() is
still byte-identical to the baseline:
tools/testing/selftests/net/openvswitch/ovs-dpctl.py:ovsactions.dpstr() {
...
else:
try:
print_str += datum.dpstr(more)
except:
print_str += "{ATTR: %s not decoded}" % field[0]
...
}
So pylint still reports W0702 for this file after the patch is applied, and
the handler still swallows KeyboardInterrupt and SystemExit while formatting
actions on the dump path main() -> ovsflow.dump() -> ovs_flow_msg.dpstr() ->
ovsactions.dpstr().
The except: -> except Exception: conversion actually appears in the next
patch of the series, "selftests: openvswitch: add missing docstrings in
ovs-dpctl.py", whose own changelog only mentions docstrings. Should that
hunk move back here, or should this bullet be dropped?
Separately, the two int() removals in the push_vlan error strings below are
real hunks of this patch but are not listed in the changelog. Worth adding a
bullet for them?
[ ... ]
> @@ -761,12 +756,12 @@ class ovsactions(nla):
> vid = int(v, 0)
> if vid < 0 or vid > 0xFFF:
> raise ValueError(
> - f"push_vlan(): vid={int(vid)} out of range
> (0-4095)")
> + f"push_vlan(): vid={vid} out of range
> (0-4095)")
> elif k == "pcp":
> pcp = int(v, 0)
> if pcp < 0 or pcp > 7:
> raise ValueError(
> - f"push_vlan(): pcp={int(pcp)} out of range
> (0-7)")
> + f"push_vlan(): pcp={pcp} out of range (0-7)")
> elif k == "tpid":
> tpid = int(v, 0)
> if tpid < 0 or tpid > 0xFFFF:
[ ... ]
> @@ -2304,7 +2296,7 @@ class OvsDatapath(GenericNetlinkSocket):
>
> nproc = multiprocessing.cpu_count()
> procarray = []
> - for i in range(1, nproc):
> + for _ in range(1, nproc):
> procarray += [int(p.epid)]
> msg["attrs"].append(["OVS_DP_ATTR_UPCALL_PID", procarray])
[Severity: Medium]
This isn't a bug introduced by this patch, but the rename does remove the one
lint signal pointing at this loop. Was the W0612 warning here telling us the
range bound is wrong rather than that the index is unused?
OvsDatapath.create() sets OVS_DP_F_DISPATCH_UPCALL_PER_CPU and then builds
nproc-1 copies of the same epid, which is an empty list on a single-CPU host.
The index was never used to pick a per-CPU socket, so range(nproc) looks like
what was meant.
The array is also appended under OVS_DP_ATTR_UPCALL_PID, which the kernel
declares as a single u32:
net/openvswitch/datapath.c:
[OVS_DP_ATTR_UPCALL_PID] = { .type = NLA_U32 },
and consumes as the local vport portid:
parms.upcall_portids = a[OVS_DP_ATTR_UPCALL_PID];
Per-CPU dispatch pids are installed only from a[OVS_DP_ATTR_PER_CPU_PIDS].
ovs-dpctl.py declares OVS_DP_ATTR_PER_CPU_PIDS in dp_cmd_msg.nla_map but
never appends it to a message, so does the per-CPU upcall dispatch path ever
actually get programmed by openvswitch.sh?
OVS_DP_CMD_NEW uses GENL_DONT_VALIDATE_STRICT, so the oversized attribute is
accepted; on a one-CPU host the list is empty and the NLA_U32 length check
should reject the message, while create() only swallows EEXIST.
[ ... ]
> @@ -3068,7 +3062,7 @@ def main(argv):
> return 1
> rep = ovsflow.dump(rep["dpifindex"])
> for flow in rep:
> - print(flow.dpstr(True if args.verbose > 0 else False))
> + print(flow.dpstr(args.verbose > 0))
[Severity: Medium]
This is a pre-existing issue and untouched by this patch, but since this hunk
is the dump printing path: does the UFID codec in this file round-trip the
UUID it was given?
In OvsFlow.ovs_flow_msg.parse() the third word is built as:
uuidRawObj[3] << 24
| uuidRawObj[4] << 16
| uuidRawObj[5] & (0xFF << 32) >> 32,
Python binds >> tighter than &, so the last term is node & 0xFF rather than
node >> 32. For the UUID used by openvswitch.sh test_flow_set,
ufid:00000001-0002-0003-0004-000500060007, the word comes out 0x00040007
instead of 0x00040005, so the UFID installed in the kernel differs from the
one requested on the command line.
The matching field is printed in OvsFlow.ovs_flow_msg.dpstr() from:
u2l = ufid[2] & 0
which is always zero, so the fifth UUID group always renders as 0000. Should
that mask be 0xFFFF?
test_flow_set only greps the dump output for actions:drop, so neither the
encode nor the print side is checked by the test.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260905104026.3776396-1-houminxi%40gmail.com
