Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 (1) · Low: 2 (1)
- [Low] Commit message overstates what the patch achieves: it claims
"Convert all 86 instances of %-formatting to f-strings to fix C0209…
- [Low] The f-string conversion replaces C0209 warnings with a batch of
new over-long source lines (C0301 / PEP8 79-column, kernel 80-column…
Pre-existing issues:
- [Medium] In `OvsFlow.ovs_flow_msg.dpstr()`
(tools/testing/selftests/net/openvswitch/ovs-dpctl.py:2556) the fifth…
- [Low] The `del-flows` branch of `main()` in
tools/testing/selftests/net/openvswitch/ovs-dpctl.py detects a missing…
--
selftests: openvswitch: convert %-formatting to f-strings
This converts %-formatting (and a few str.format() call sites) in
tools/testing/selftests/net/openvswitch/ovs-dpctl.py over to f-strings in
order to silence pylint C0209. It is described as a pure warning cleanup
with no behavior change.
[Severity: Low]
The commit message says:
"Convert all 86 instances of %-formatting to f-strings to fix
C0209 pylint warnings."
Is "all" accurate here? One literal %-format survives this commit, in
ovsactions.dpstr():
try:
print_str += datum.dpstr(more)
except:
print_str += "{ATTR: %s not decoded}" % field[0]
C0209 fires exactly on a literal string used with the % operator, so
pylint still reports it for this file after this patch. This one does get
converted later in the series (in "selftests: openvswitch: add missing
docstrings in ovs-dpctl.py"), so would it make sense to reword the message
here rather than claim the file is C0209-clean?
The message also only mentions %-formatting, while the diff additionally
rewrites str.format() call sites (the UFID string, the sample probability
and the used_time print). Could the count and the "all %-formatting"
wording be adjusted to match what the diff actually touches?
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 1615843c225e4..9cd0d8f0ab233 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
[ ... ]
> @@ -413,14 +413,13 @@ class ovsactions(nla):
> )
>
> def dpstr(self, more=False):
> - args = "group=%d" % self.get_attr("OVS_PSAMPLE_ATTR_GROUP")
> + args = f"group={int(self.get_attr('OVS_PSAMPLE_ATTR_GROUP'))}"
>
> cookie = self.get_attr("OVS_PSAMPLE_ATTR_COOKIE")
> if cookie:
> - args += ",cookie(%s)" % \
> - "".join(format(x, "02x") for x in cookie)
> + args += f",cookie({''.join((format(x, '02x') for x in
> cookie))})"
[Severity: Low]
This conversion trades C0209 for C0301. The cookie line in
ovsactions.psample.dpstr() was wrapped under 80 columns before via a line
continuation and is now over the limit.
The same happens in several other places touched by this patch, all of
which were within 79 columns before:
- the NAT IP_MIN / IP_MAX prints in
ovsactions.ovs_ct_attr.ovs_nat_attr.dpstr()
- the emask print in ovsactions.ovs_ct_attr.dpstr()
- the pid print in ovsactions.userspace.dpstr()
- the three push_vlan() range-check messages in ovsactions.parse()
- the tci / vid range-check messages in ovskey._parse_vlan_from_flowstr()
- the encap() trailing-content message in ovskey._parse_encap_from_flowstr()
- the port print in print_ovsdp_full()
The file goes from 5 to 16 lines longer than 79 columns. Since the same
patch already uses intermediate locals elsewhere (val, pkts/nbytes,
mhit/mtotal), could those be used here too so no new long lines appear?
For reference, the later patch "selftests: openvswitch: suppress pylint
complexity warnings" concedes "10 remaining C0301 line-too-long warnings
(81-89 columns)" at the series tip, so these stay for the whole series.
>
> - return "psample(%s)" % args
> + return f"psample({args})"
[ ... ]
> @@ -564,22 +557,18 @@ class ovsactions(nla):
> if self.get_attr("OVS_CT_ATTR_FORCE_COMMIT") is not None:
> print_str += "force,"
> if self.get_attr("OVS_CT_ATTR_EVENTMASK") is not None:
> - print_str += "emask=0x%X," % self.get_attr(
> - "OVS_CT_ATTR_EVENTMASK"
> - )
> + print_str +=
> f"emask=0x{self.get_attr('OVS_CT_ATTR_EVENTMASK'):X},"
[ ... ]
> @@ -767,32 +754,27 @@ class ovsactions(nla):
> if k == "vid":
> vid = int(v, 0)
> if vid < 0 or vid > 0xFFF:
> raise ValueError(
> - "push_vlan(): vid=%d out of "
> - "range (0-4095)" % vid)
> + f"push_vlan(): vid={int(vid)} out of range
> (0-4095)")
> elif k == "pcp":
> pcp = int(v, 0)
> if pcp < 0 or pcp > 7:
> raise ValueError(
> - "push_vlan(): pcp=%d out of "
> - "range (0-7)" % pcp)
> + f"push_vlan(): pcp={int(pcp)} out of range
> (0-7)")
> elif k == "tpid":
> tpid = int(v, 0)
> if tpid < 0 or tpid > 0xFFFF:
> raise ValueError(
> - "push_vlan(): tpid=0x%x out "
> - "of range (0-0xffff)" % tpid)
> + f"push_vlan(): tpid=0x{tpid:x} out of range
> (0-0xffff)")
[ ... ]
> @@ -2569,16 +2549,14 @@ class OvsFlow(GenericNetlinkSocket):
> ufid = self.get_attr("OVS_FLOW_ATTR_UFID")
> ufid_str = ""
> if ufid is not None:
> - ufid_str = (
> - "ufid:{:08x}-{:04x}-{:04x}-{:04x}-{:04x}{:08x}".format(
> - ufid[0],
> - ufid[1] >> 16,
> - ufid[1] & 0xFFFF,
> - ufid[2] >> 16,
> - ufid[2] & 0,
> - ufid[3],
> - )
> - )
> + u0 = ufid[0]
> + u1h = ufid[1] >> 16
> + u1l = ufid[1] & 0xFFFF
> + u2h = ufid[2] >> 16
> + u2l = ufid[2] & 0
^^^
[Severity: Medium]
This isn't a bug introduced by this patch, but since the mask is being
lifted into a freshly written local, is the & 0 intentional?
Every sibling group in OvsFlow.ovs_flow_msg.dpstr() uses a real shift or
mask (ufid[1] >> 16, ufid[1] & 0xFFFF, ufid[2] >> 16), while u2l is
unconditionally zero, so the fifth group of the printed UFID always comes
out as 0000 and two flows differing only in the low half of ufid[2] print
an identical ufid: string under dump-flows -v.
The packing side in OvsFlow.ovs_flow_msg.parse() drops the same bits:
uuidRawObj[5] & (0xFF << 32) >> 32,
Since >> binds tighter than &, this is uuidRawObj[5] & 0xFF, so only bits
0-7 of the UUID node field reach word 2.
openvswitch.sh test_flow_set installs a UFID with a nonzero fifth group:
local fwd_flow="ufid:00000001-0002-0003-0004-000500060007"
but only asserts on the dumped action, so nothing in-tree catches this.
Would it be worth using 0xFFFF here (as a separate patch) instead of
carrying & 0 forward? It is still present at the series tip.
> + u3 = ufid[3]
> + ufid_str = (f"ufid:{u0:08x}-{u1h:04x}-{u1l:04x}"
> + f"-{u2h:04x}-{u2l:04x}{u3:08x}")
[ ... ]
> @@ -3115,7 +3088,7 @@ def main(argv):
> elif hasattr(args, "flsbr"):
> rep = ovsdp.info(args.flsbr, 0)
> if rep is None:
> - print("DP '%s' not found." % args.flsbr)
> + print(f"DP '{args.flsbr}' not found.")
> ovsflow.del_flows(rep["dpifindex"])
[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but while
this branch is being touched: is the missing return 1 here deliberate?
OvsDatapath.info() turns an ENODEV netlink reply into None:
except NetlinkError as ne:
if ne.code == errno.ENODEV:
reply = None
so del-flows against a nonexistent datapath prints the message and then
falls into rep["dpifindex"], raising TypeError: 'NoneType' object is not
subscriptable. The adjacent addif / delif / dumpdp / add-flow / mod-flow
branches all return 1 after the same check. It is unchanged at the series
tip.
>
> return 0
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260905104026.3776396-1-houminxi%40gmail.com
