* [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py
@ 2026-09-05 10:40 Minxi Hou
2026-09-05 10:40 ` [PATCH 1/4] selftests: openvswitch: convert %-formatting to f-strings Minxi Hou
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Minxi Hou @ 2026-09-05 10:40 UTC (permalink / raw)
To: netdev
Cc: aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms, Minxi Hou
Clean up the ovs-dpctl.py selftest utility to pass pylint. All
changes are mechanical style fixes with no behavior change; the full
openvswitch.sh selftest suite passes (17/17) on this tree.
The series fixes all pylint warnings except 10 remaining C0301
line-too-long warnings (81-89 columns) on f-string constructions
that match the surrounding style, bringing the score from 7.66/10
to 9.93/10:
patch 1: convert %-formatting to f-strings (C0209)
patch 2: fix miscellaneous warnings (unused imports/variables,
bare except, superfluous parens, etc.)
patch 3: add missing module/class/method docstrings
(C0114/C0115/C0116)
patch 4: suppress framework-inherent complexity warnings
Minxi Hou (4):
selftests: openvswitch: convert %-formatting to f-strings
selftests: openvswitch: fix misc pylint warnings in ovs-dpctl.py
selftests: openvswitch: add missing docstrings in ovs-dpctl.py
selftests: openvswitch: suppress pylint complexity warnings
.../selftests/net/openvswitch/ovs-dpctl.py | 425 ++++++++++--------
1 file changed, 245 insertions(+), 180 deletions(-)
base-commit: 9eab111e765729e93087ff86a2ec9b2ae42d0fa5
--
2.55.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] selftests: openvswitch: convert %-formatting to f-strings
2026-09-05 10:40 [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
@ 2026-09-05 10:40 ` Minxi Hou
2026-09-09 12:42 ` netdev-bot+sashiko
2026-09-05 10:40 ` [PATCH 2/4] selftests: openvswitch: fix misc pylint warnings in ovs-dpctl.py Minxi Hou
` (4 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Minxi Hou @ 2026-09-05 10:40 UTC (permalink / raw)
To: netdev
Cc: aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms, Minxi Hou
Convert all 86 instances of %-formatting to f-strings to fix
C0209 pylint warnings. No behavior change.
Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
base-commit: 9eab111e765729e93087ff86a2ec9b2ae42d0fa5
.../selftests/net/openvswitch/ovs-dpctl.py | 267 ++++++++----------
1 file changed, 120 insertions(+), 147 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 1615843c225e..9cd0d8f0ab23 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -65,7 +65,7 @@ OVS_FLOW_CMD_SET = 4
UINT32_MAX = 0xFFFFFFFF
def macstr(mac):
- outstr = ":".join(["%02X" % i for i in mac])
+ outstr = ":".join([f"{i:02X}" for i in mac])
return outstr
@@ -146,7 +146,7 @@ def parse_flags(flag_str, flag_vals):
if flag in flag_vals:
if maskResult & flag_vals[flag]:
raise KeyError(
- "Flag %s set once, cannot be set in multiples" % flag
+ f"Flag {flag} set once, cannot be set in multiples"
)
if setFlag:
@@ -154,7 +154,7 @@ def parse_flags(flag_str, flag_vals):
maskResult |= flag_vals[flag]
else:
- raise KeyError("Missing flag value: %s" % flag)
+ raise KeyError(f"Missing flag value: {flag}")
flag_str = flag_str[flag_len:]
@@ -211,7 +211,7 @@ def convert_ipv6(data):
elif not mask:
mask = 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff'
elif mask.isdigit():
- mask = ipaddress.IPv6Network("::/" + mask).hostmask
+ mask = ipaddress.IPv6Network(f"::/{mask}").hostmask
return ipaddress.IPv6Address(ip).packed, ipaddress.IPv6Address(mask).packed
@@ -342,13 +342,13 @@ def parse_attrs(actstr, attr_desc):
del attr_desc[i]
if not found:
- raise ValueError("Unknown attribute: '%s'" % actstr)
+ raise ValueError(f"Unknown attribute: '{actstr}'")
actstr = actstr[strspn(actstr, ", ") :]
if actstr[0] != ")":
raise ValueError("Action string contains extra garbage or has "
- "unbalanced parenthesis: '%s'" % actstr)
+ f"unbalanced parenthesis: '{actstr}'")
return attrs, actstr[1:]
@@ -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))})"
- return "psample(%s)" % args
+ return f"psample({args})"
def parse(self, actstr):
desc = (
@@ -451,15 +450,15 @@ class ovsactions(nla):
def dpstr(self, more=False):
args = []
- args.append("sample={:.2f}%".format(
- 100 * self.get_attr("OVS_SAMPLE_ATTR_PROBABILITY") /
- UINT32_MAX))
+ prob = 100 * self.get_attr(
+ "OVS_SAMPLE_ATTR_PROBABILITY") / UINT32_MAX
+ args.append(f"sample={prob:.2f}%")
actions = self.get_attr("OVS_SAMPLE_ATTR_ACTIONS")
if actions:
- args.append("actions(%s)" % actions.dpstr(more))
+ args.append(f"actions({actions.dpstr(more)})")
- return "sample(%s)" % ",".join(args)
+ return f"sample({','.join(args)})"
def parse(self, actstr):
def parse_nested_actions(actstr):
@@ -528,26 +527,20 @@ class ovsactions(nla):
"OVS_NAT_ATTR_IP_MAX"
):
if self.get_attr("OVS_NAT_ATTR_IP_MIN"):
- print_str += "=%s," % str(
- self.get_attr("OVS_NAT_ATTR_IP_MIN")
- )
+ print_str += f"={self.get_attr('OVS_NAT_ATTR_IP_MIN')!s},"
if self.get_attr("OVS_NAT_ATTR_IP_MAX"):
- print_str += "-%s," % str(
- self.get_attr("OVS_NAT_ATTR_IP_MAX")
- )
+ print_str += f"-{self.get_attr('OVS_NAT_ATTR_IP_MAX')!s},"
else:
print_str += ","
if self.get_attr("OVS_NAT_ATTR_PROTO_MIN"):
- print_str += "proto_min=%d," % self.get_attr(
- "OVS_NAT_ATTR_PROTO_MIN"
- )
+ val = self.get_attr("OVS_NAT_ATTR_PROTO_MIN")
+ print_str += f"proto_min={val},"
if self.get_attr("OVS_NAT_ATTR_PROTO_MAX"):
- print_str += "proto_max=%d," % self.get_attr(
- "OVS_NAT_ATTR_PROTO_MAX"
- )
+ val = self.get_attr("OVS_NAT_ATTR_PROTO_MAX")
+ print_str += f"proto_max={val},"
if self.get_attr("OVS_NAT_ATTR_PERSISTENT"):
print_str += "persistent,"
@@ -564,22 +557,18 @@ class ovsactions(nla):
if self.get_attr("OVS_CT_ATTR_COMMIT") is not None:
print_str += "commit,"
if self.get_attr("OVS_CT_ATTR_ZONE") is not None:
- print_str += "zone=%d," % self.get_attr("OVS_CT_ATTR_ZONE")
+ print_str += f"zone={int(self.get_attr('OVS_CT_ATTR_ZONE'))},"
if self.get_attr("OVS_CT_ATTR_HELPER") is not None:
- print_str += "helper=%s," % self.get_attr("OVS_CT_ATTR_HELPER")
+ print_str += f"helper={self.get_attr('OVS_CT_ATTR_HELPER')},"
if self.get_attr("OVS_CT_ATTR_NAT") is not None:
print_str += self.get_attr("OVS_CT_ATTR_NAT").dpstr(more)
print_str += ","
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},"
if self.get_attr("OVS_CT_ATTR_TIMEOUT") is not None:
- print_str += "timeout=%s" % self.get_attr(
- "OVS_CT_ATTR_TIMEOUT"
- )
+ print_str += f"timeout={self.get_attr('OVS_CT_ATTR_TIMEOUT')}"
print_str += ")"
return print_str
@@ -596,17 +585,15 @@ class ovsactions(nla):
def dpstr(self, more=False):
print_str = "userspace("
if self.get_attr("OVS_USERSPACE_ATTR_PID") is not None:
- print_str += "pid=%d," % self.get_attr(
- "OVS_USERSPACE_ATTR_PID"
- )
+ print_str += f"pid={int(self.get_attr('OVS_USERSPACE_ATTR_PID'))},"
if self.get_attr("OVS_USERSPACE_ATTR_USERDATA") is not None:
print_str += "userdata="
for f in self.get_attr("OVS_USERSPACE_ATTR_USERDATA"):
- print_str += "%x." % f
+ print_str += f"{f:x}."
if self.get_attr("OVS_USERSPACE_ATTR_EGRESS_TUN_PORT") is not None:
- print_str += "egress_tun_port=%d" % self.get_attr(
- "OVS_USERSPACE_ATTR_EGRESS_TUN_PORT"
- )
+ val = self.get_attr(
+ "OVS_USERSPACE_ATTR_EGRESS_TUN_PORT")
+ print_str += f"egress_tun_port={val}"
print_str += ")"
return print_str
@@ -634,13 +621,13 @@ class ovsactions(nla):
print_str += ","
if field[0] == "OVS_ACTION_ATTR_OUTPUT":
- print_str += "%d" % int(self.get_attr(field[0]))
+ print_str += f"{int(self.get_attr(field[0]))}"
elif field[0] == "OVS_ACTION_ATTR_RECIRC":
- print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
+ print_str += f"recirc(0x{int(self.get_attr(field[0])):x})"
elif field[0] == "OVS_ACTION_ATTR_TRUNC":
- print_str += "trunc(%d)" % int(self.get_attr(field[0]))
+ print_str += f"trunc({int(self.get_attr(field[0]))})"
elif field[0] == "OVS_ACTION_ATTR_DROP":
- print_str += "drop(%d)" % int(self.get_attr(field[0]))
+ print_str += f"drop({int(self.get_attr(field[0]))})"
elif field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
print_str += "ct_clear"
elif field[0] == "OVS_ACTION_ATTR_POP_VLAN":
@@ -658,8 +645,8 @@ class ovsactions(nla):
tci = datum["vlan_tci"]
vid = tci & 0x0FFF
pcp = (tci >> 13) & 0x7
- print_str += "push_vlan(vid=%d,pcp=%d" \
- ",tpid=0x%04x)" % (vid, pcp, tpid)
+ print_str += (f"push_vlan(vid={vid},pcp={pcp}"
+ f",tpid=0x{tpid:04x})")
elif field[0] == "OVS_ACTION_ATTR_POP_ETH":
print_str += "pop_eth"
elif field[0] == "OVS_ACTION_ATTR_POP_NSH":
@@ -767,32 +754,27 @@ class ovsactions(nla):
for kv in actstr[:paren].split(","):
if "=" not in kv:
raise ValueError(
- "push_vlan(): bad field '%s'"
- % kv.strip())
+ f"push_vlan(): bad field '{kv.strip()}'")
k = kv[:kv.index("=")].strip()
v = kv[kv.index("=") + 1:].strip()
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)")
else:
raise ValueError(
- "push_vlan(): unknown key '%s'"
- % k)
+ f"push_vlan(): unknown key '{k}'")
tci = (vid & 0x0FFF) | ((pcp & 0x7) << 13) \
| 0x1000
pvact = self.push_vlan()
@@ -833,7 +815,7 @@ class ovsactions(nla):
actstr = k.parse(actstr, None)
self["attrs"].append(("OVS_ACTION_ATTR_SET", k))
if not actstr.startswith(")"):
- actstr = ")" + actstr
+ actstr = f"){actstr}"
parsed = True
elif parse_starts_block(actstr, "set_masked(", False):
parencount += 1
@@ -843,7 +825,7 @@ class ovsactions(nla):
actstr = k.parse(actstr, m)
self["attrs"].append(("OVS_ACTION_ATTR_SET_MASKED", [k, m]))
if not actstr.startswith(")"):
- actstr = ")" + actstr
+ actstr = f"){actstr}"
parsed = True
elif parse_starts_block(actstr, "ct(", False):
parencount += 1
@@ -974,7 +956,7 @@ class ovsactions(nla):
parencount -= 1
actstr = actstr[strspn(actstr, " "):]
if len(actstr) and actstr[0] != ")":
- raise ValueError("Action str: '%s' unbalanced" % actstr)
+ raise ValueError(f"Action str: '{actstr}' unbalanced")
actstr = actstr[1:]
if len(actstr) and actstr[0] == ")":
@@ -983,7 +965,7 @@ class ovsactions(nla):
actstr = actstr[strspn(actstr, ", ") :]
if not parsed:
- raise ValueError("Action str: '%s' not supported" % actstr)
+ raise ValueError(f"Action str: '{actstr}' not supported")
return (totallen - len(actstr))
@@ -1108,20 +1090,20 @@ class ovskey(nla):
return flowstr, k, m
def dpstr(self, masked=None, more=False):
- outstr = self.proto_str + "("
+ outstr = f"{self.proto_str}("
first = False
for f in self.fields_map:
if first:
outstr += ","
if masked is None:
- outstr += "%s=" % f[0]
+ outstr += f"{f[0]}="
if isinstance(f[2], str):
outstr += f[2] % self[f[1]]
else:
outstr += f[2](self[f[1]])
first = True
elif more or f[3](masked[f[1]]) != 0:
- outstr += "%s=" % f[0]
+ outstr += f"{f[0]}="
if isinstance(f[2], str):
outstr += f[2] % self[f[1]]
else:
@@ -1702,23 +1684,23 @@ class ovskey(nla):
for k in self["attrs"]:
noprint = False
if k[0] == "OVS_TUNNEL_KEY_ATTR_ID":
- print_str += "tun_id=%d" % k[1]
+ print_str += f"tun_id={int(k[1])}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_IPV4_SRC":
- print_str += "src=%s" % k[1]
+ print_str += f"src={k[1]}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_IPV4_DST":
- print_str += "dst=%s" % k[1]
+ print_str += f"dst={k[1]}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_IPV6_SRC":
- print_str += "ipv6_src=%s" % k[1]
+ print_str += f"ipv6_src={k[1]}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_IPV6_DST":
- print_str += "ipv6_dst=%s" % k[1]
+ print_str += f"ipv6_dst={k[1]}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_TOS":
- print_str += "tos=%d" % k[1]
+ print_str += f"tos={int(k[1])}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_TTL":
- print_str += "ttl=%d" % k[1]
+ print_str += f"ttl={int(k[1])}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_TP_SRC":
- print_str += "tp_src=%d" % k[1]
+ print_str += f"tp_src={int(k[1])}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_TP_DST":
- print_str += "tp_dst=%d" % k[1]
+ print_str += f"tp_dst={int(k[1])}"
elif k[0] == "OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT":
noprint = True
flagsattrs.append("df")
@@ -1733,7 +1715,7 @@ class ovskey(nla):
print_str += ","
if len(flagsattrs):
- print_str += "flags(" + "|".join(flagsattrs) + ")"
+ print_str += f"flags({'|'.join(flagsattrs)})"
print_str += ")"
return print_str
@@ -1756,8 +1738,8 @@ class ovskey(nla):
pcp = (tci >> 13) & 0x7
cfi = (tci >> 12) & 0x1
if cfi:
- return "vid=%d,pcp=%d,cfi=%d" % (vid, pcp, cfi)
- return "tci=0x%04x" % tci
+ return f"vid={int(vid)},pcp={int(pcp)},cfi={int(cfi)}"
+ return f"tci=0x{tci:04x}"
@staticmethod
def _parse_vlan_from_flowstr(flowstr):
@@ -1801,7 +1783,7 @@ class ovskey(nla):
eq = flowstr.find('=')
if eq == -1:
raise ValueError(
- "vlan(): expected key=value, got '%s'" % flowstr)
+ f"vlan(): expected key=value, got '{flowstr}'")
key = flowstr[:eq].strip()
flowstr = flowstr[eq + 1:]
@@ -1815,13 +1797,12 @@ class ovskey(nla):
flowstr = flowstr[end:]
if not val:
- raise ValueError("vlan(): empty value for key '%s'" % key)
+ raise ValueError(f"vlan(): empty value for key '{key}'")
try:
v = int(val, 0)
except ValueError as exc:
raise ValueError(
- "vlan(): invalid value '%s' for key '%s'"
- % (val, key)) from exc
+ f"vlan(): invalid value '{val}' for key '{key}'") from exc
if key == 'tci':
if has_tci:
@@ -1829,7 +1810,7 @@ class ovskey(nla):
if has_vid or has_pcp or has_cfi:
raise ValueError(_tci_mix_err)
if v > 0xFFFF or v < 0:
- raise ValueError("vlan(): tci=0x%x out of range" % v)
+ raise ValueError(f"vlan(): tci=0x{v:x} out of range")
tci = v
mask = 0xFFFF
has_tci = True
@@ -1839,7 +1820,7 @@ class ovskey(nla):
if has_vid:
raise ValueError("vlan(): duplicate 'vid'")
if v < 0 or v > 0xFFF:
- raise ValueError("vlan(): vid=%d out of range (0-4095)" % v)
+ raise ValueError(f"vlan(): vid={int(v)} out of range (0-4095)")
tci |= v
mask |= 0x0FFF
has_vid = True
@@ -1849,7 +1830,7 @@ class ovskey(nla):
if has_pcp:
raise ValueError("vlan(): duplicate 'pcp'")
if v < 0 or v > 7:
- raise ValueError("vlan(): pcp=%d out of range (0-7)" % v)
+ raise ValueError(f"vlan(): pcp={int(v)} out of range (0-7)")
tci |= (v & 0x7) << 13
mask |= 0xE000
has_pcp = True
@@ -1864,7 +1845,7 @@ class ovskey(nla):
mask |= ovskey._VLAN_CFI_MASK
has_cfi = True
else:
- raise ValueError("vlan(): unknown key '%s'" % key)
+ raise ValueError(f"vlan(): unknown key '{key}'")
flowstr = flowstr[1:] # skip ')'
# Catch immediate '))' (user error). A ')' after ',' is consumed
@@ -1900,7 +1881,7 @@ class ovskey(nla):
depth -= 1
if depth < 0:
raise ValueError(
- "encap(): unmatched ')' at position %d" % i)
+ f"encap(): unmatched ')' at position {int(i)}")
if depth == 0:
end = i
break
@@ -1923,8 +1904,7 @@ class ovskey(nla):
remaining = inner_key.parse(inner_str, inner_mask)
if remaining and re.search(r'[^\s,)]', remaining):
raise ValueError(
- "encap(): unrecognized trailing "
- "content '%s'" % remaining.strip())
+ f"encap(): unrecognized trailing content '{remaining.strip()}'")
return flowstr, inner_key, inner_mask
@@ -2005,7 +1985,7 @@ class ovskey(nla):
lambda x: parse_flags(x, None),
),
):
- fld = field[1] + "("
+ fld = f"{field[1]}("
if not flowstr.startswith(fld):
continue
@@ -2143,15 +2123,15 @@ class ovskey(nla):
else:
if m is None or field[3](m):
val = fmt(v) if callable(fmt) else fmt % v
- print_str += field[1] + "(" + val + "),"
+ print_str += f"{field[1]}({val}),"
elif more or m != 0:
if field[0] == "OVS_KEY_ATTR_VLAN":
- val = "tci=0x%04x/0x%04x" % (v, m)
+ val = f"tci=0x{v:04x}/0x{m:04x}"
elif callable(fmt):
- val = fmt(v) + "/" + fmt(m)
+ val = f"{fmt(v)}/{fmt(m)}"
else:
- val = (fmt % v) + "/" + (fmt % m)
- print_str += field[1] + "(" + val + "),"
+ val = f"{fmt % v}/{fmt % m}"
+ print_str += f"{field[1]}({val}),"
return print_str
@@ -2233,7 +2213,7 @@ class OvsPacket(GenericNetlinkSocket):
elif msg["cmd"] == OvsPacket.OVS_PACKET_CMD_EXECUTE:
up.execute(msg)
else:
- print("Unknown cmd: %d" % msg["cmd"])
+ print(f"Unknown cmd: {int(msg['cmd'])}")
except NetlinkError as ne:
raise ne
@@ -2401,14 +2381,14 @@ class OvsVport(GenericNetlinkSocket):
return "netdev"
elif vport_type == OvsVport.OVS_VPORT_TYPE_INTERNAL:
return "internal"
- raise ValueError("Unknown vport type:%d" % vport_type)
+ raise ValueError(f"Unknown vport type:{int(vport_type)}")
def str_to_type(vport_type):
if vport_type in ["netdev", "gre", "vxlan", "geneve"]:
return OvsVport.OVS_VPORT_TYPE_NETDEV
elif vport_type == "internal":
return OvsVport.OVS_VPORT_TYPE_INTERNAL
- raise ValueError("Unknown vport type: '%s'" % vport_type)
+ raise ValueError(f"Unknown vport type: '{vport_type}'")
def __init__(self, packet=OvsPacket()):
GenericNetlinkSocket.__init__(self)
@@ -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
+ u3 = ufid[3]
+ ufid_str = (f"ufid:{u0:08x}-{u1h:04x}-{u1l:04x}"
+ f"-{u2h:04x}-{u2l:04x}{u3:08x}")
key_field = self.get_attr("OVS_FLOW_ATTR_KEY")
keymsg = None
@@ -2598,7 +2576,7 @@ class OvsFlow(GenericNetlinkSocket):
print_str = ""
if more:
- print_str += ufid_str + ","
+ print_str += f"{ufid_str},"
if keymsg is not None:
print_str += keymsg.dpstr(maskmsg, more)
@@ -2607,10 +2585,9 @@ class OvsFlow(GenericNetlinkSocket):
if stats is None:
print_str += " packets:0, bytes:0,"
else:
- print_str += " packets:%d, bytes:%d," % (
- stats["packets"],
- stats["bytes"],
- )
+ pkts = stats["packets"]
+ nbytes = stats["bytes"]
+ print_str += f" packets:{pkts}, bytes:{nbytes},"
used = self.get_attr("OVS_FLOW_ATTR_USED")
print_str += " used:"
@@ -2620,7 +2597,7 @@ class OvsFlow(GenericNetlinkSocket):
used_time = int(used)
cur_time_sec = time.clock_gettime(time.CLOCK_MONOTONIC)
used_time = (cur_time_sec * 1000) - used_time
- print_str += "{}s,".format(used_time / 1000)
+ print_str += f"{used_time / 1000}s,"
print_str += " actions:"
if (
@@ -2808,7 +2785,7 @@ class OvsFlow(GenericNetlinkSocket):
pktdata = packetmsg.get_attr("OVS_PACKET_ATTR_PACKET")
pktpres = "yes" if pktdata is not None else "no"
- print("MISS upcall[%d/%s]: %s" % (seq, pktpres, keystr), flush=True)
+ print(f"MISS upcall[{int(seq)}/{pktpres}]: {keystr}", flush=True)
def execute(self, packetmsg):
print("userspace execute command", flush=True)
@@ -2842,16 +2819,16 @@ class psample_sample(genlmsg):
data = ""
for (attr, value) in self["attrs"]:
if attr == "PSAMPLE_ATTR_SAMPLE_GROUP":
- fields.append("group:%d" % value)
+ fields.append(f"group:{int(value)}")
if attr == "PSAMPLE_ATTR_SAMPLE_RATE":
- fields.append("rate:%d" % value)
+ fields.append(f"rate:{int(value)}")
if attr == "PSAMPLE_ATTR_USER_COOKIE":
value = "".join(format(x, "02x") for x in value)
- fields.append("cookie:%s" % value)
+ fields.append(f"cookie:{value}")
if attr == "PSAMPLE_ATTR_DATA" and len(value) > 0:
- data = "data:%s" % "".join(format(x, "02x") for x in value)
+ data = f"data:{''.join((format(x, '02x') for x in value))}"
- return ("%s %s" % (",".join(fields), data)).strip()
+ return (f"{','.join(fields)} {data}").strip()
class psample_msg(Marshal):
@@ -2885,35 +2862,31 @@ def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
user_features = dp_lookup_rep.get_attr("OVS_DP_ATTR_USER_FEATURES")
masks_cache_size = dp_lookup_rep.get_attr("OVS_DP_ATTR_MASKS_CACHE_SIZE")
- print("%s:" % dp_name)
- print(
- " lookups: hit:%d missed:%d lost:%d"
- % (base_stats["hit"], base_stats["missed"], base_stats["lost"])
- )
- print(" flows:%d" % base_stats["flows"])
+ print(f"{dp_name}:")
+ hit = base_stats["hit"]
+ missed = base_stats["missed"]
+ lost = base_stats["lost"]
+ print(f" lookups: hit:{hit} missed:{missed} lost:{lost}")
+ print(f" flows:{int(base_stats['flows'])}")
pkts = base_stats["hit"] + base_stats["missed"]
avg = (megaflow_stats["mask_hit"] / pkts) if pkts != 0 else 0.0
- print(
- " masks: hit:%d total:%d hit/pkt:%f"
- % (megaflow_stats["mask_hit"], megaflow_stats["masks"], avg)
- )
+ mhit = megaflow_stats["mask_hit"]
+ mtotal = megaflow_stats["masks"]
+ print(f" masks: hit:{mhit} total:{mtotal} hit/pkt:{avg:f}")
print(" caches:")
- print(" masks-cache: size:%d" % masks_cache_size)
+ print(f" masks-cache: size:{int(masks_cache_size)}")
if user_features is not None:
- print(" features: 0x%X" % user_features)
+ print(f" features: 0x{user_features:X}")
# port print out
for iface in ndb.interfaces:
rep = vpl.info(iface.ifname, ifindex)
if rep is not None:
print(
- " port %d: %s (%s)"
- % (
- rep.get_attr("OVS_VPORT_ATTR_PORT_NO"),
- rep.get_attr("OVS_VPORT_ATTR_NAME"),
- OvsVport.type_to_str(rep.get_attr("OVS_VPORT_ATTR_TYPE")),
- )
+ f" port {int(rep.get_attr('OVS_VPORT_ATTR_PORT_NO'))}: "
+ f"{rep.get_attr('OVS_VPORT_ATTR_NAME')} "
+ f"({OvsVport.type_to_str(rep.get_attr('OVS_VPORT_ATTR_TYPE'))})"
)
@@ -3045,14 +3018,14 @@ def main(argv):
if not found:
msg = "No DP found"
if args.showdp is not None:
- msg += ":'%s'" % args.showdp
+ msg += f":'{args.showdp}'"
print(msg)
elif hasattr(args, "adddp"):
rep = ovsdp.create(args.adddp, args.upcall, args.versioning, ovspk)
if rep is None:
- print("DP '%s' already exists" % args.adddp)
+ print(f"DP '{args.adddp}' already exists")
else:
- print("DP '%s' added" % args.adddp)
+ print(f"DP '{args.adddp}' added")
if args.upcall:
ovspk.upcall_handler(ovsflow)
elif hasattr(args, "deldp"):
@@ -3060,12 +3033,12 @@ def main(argv):
elif hasattr(args, "addif"):
rep = ovsdp.info(args.dpname, 0)
if rep is None:
- print("DP '%s' not found." % args.dpname)
+ print(f"DP '{args.dpname}' not found.")
return 1
dpindex = rep["dpifindex"]
rep = ovsvp.attach(rep["dpifindex"], args.addif, args.ptype,
args.dport)
- msg = "vport '%s'" % args.addif
+ msg = f"vport '{args.addif}'"
if rep and rep["header"]["error"] is None:
msg += " added."
else:
@@ -3077,10 +3050,10 @@ def main(argv):
elif hasattr(args, "delif"):
rep = ovsdp.info(args.dpname, 0)
if rep is None:
- print("DP '%s' not found." % args.dpname)
+ print(f"DP '{args.dpname}' not found.")
return 1
rep = ovsvp.detach(rep["dpifindex"], args.delif)
- msg = "vport '%s'" % args.delif
+ msg = f"vport '{args.delif}'"
if rep and rep["header"]["error"] is None:
msg += " removed."
else:
@@ -3091,7 +3064,7 @@ def main(argv):
elif hasattr(args, "dumpdp"):
rep = ovsdp.info(args.dumpdp, 0)
if rep is None:
- print("DP '%s' not found." % args.dumpdp)
+ print(f"DP '{args.dumpdp}' not found.")
return 1
rep = ovsflow.dump(rep["dpifindex"])
for flow in rep:
@@ -3099,7 +3072,7 @@ def main(argv):
elif hasattr(args, "flbr"):
rep = ovsdp.info(args.flbr, 0)
if rep is None:
- print("DP '%s' not found." % args.flbr)
+ print(f"DP '{args.flbr}' not found.")
return 1
flow = OvsFlow.ovs_flow_msg()
flow.parse(args.flow, args.acts, rep["dpifindex"])
@@ -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"])
return 0
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] selftests: openvswitch: fix misc pylint warnings in ovs-dpctl.py
2026-09-05 10:40 [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
2026-09-05 10:40 ` [PATCH 1/4] selftests: openvswitch: convert %-formatting to f-strings Minxi Hou
@ 2026-09-05 10:40 ` Minxi Hou
2026-09-09 12:42 ` netdev-bot+sashiko
2026-09-05 10:40 ` [PATCH 3/4] selftests: openvswitch: add missing docstrings " Minxi Hou
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Minxi Hou @ 2026-09-05 10:40 UTC (permalink / raw)
To: netdev
Cc: aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms, Minxi Hou
Fix miscellaneous pylint warnings with no behavior change:
- W0611: remove unused import struct
- W0702: replace bare except with except Exception
- C0325: remove superfluous parentheses after return (3)
- R1705: remove unnecessary elif after return (3)
- W0108: replace unnecessary lambda with int
- R1714: merge comparisons with in operator
- W0719: replace raise Exception with raise ValueError
- C1802: use implicit boolean test instead of len()
- C0121: use is None instead of == None
- R1719: simplify if-expression to bool test
- R1703: simplify if/else to assignment expression
- W0612: remove unused variables (keybits, maskbits, lst)
- replace unused loop variable with underscore
Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
base-commit: 9eab111e765729e93087ff86a2ec9b2ae42d0fa5
.../selftests/net/openvswitch/ovs-dpctl.py | 44 ++++++++-----------
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 9cd0d8f0ab23..6a02810fe4ea 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -11,7 +11,6 @@ import logging
import math
import multiprocessing
import re
-import struct
import sys
import time
import types
@@ -125,10 +124,7 @@ def parse_flags(flag_str, flag_vals):
maskResult = int(digits, 0)
while len(flag_str) > 0 and (flag_str[0] == "+" or flag_str[0] == "-"):
- if flag_str[0] == "+":
- setFlag = True
- elif flag_str[0] == "-":
- setFlag = False
+ setFlag = flag_str[0] == "+"
flag_str = flag_str[1:]
@@ -221,10 +217,9 @@ def convert_int(size):
if not value:
return 0, 0
- elif not mask:
+ if not mask:
return int(value, 0), pow(2, size) - 1
- else:
- return int(value, 0), int(mask, 0)
+ return int(value, 0), int(mask, 0)
return convert_int_sized
@@ -705,11 +700,11 @@ class ovsactions(nla):
parsed = True
else:
actstr = actstr[len("drop"): ]
- return (totallen - len(actstr))
+ return totallen - len(actstr)
elif parse_starts_block(actstr, r"^(\d+)", False, True):
actstr, output = parse_extract_field(
- actstr, None, r"(\d+)", lambda x: int(x), False, "0"
+ actstr, None, r"(\d+)", int, False, "0"
)
self["attrs"].append(["OVS_ACTION_ATTR_OUTPUT", output])
parsed = True
@@ -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:
@@ -804,7 +799,6 @@ class ovsactions(nla):
subacts = ovsactions()
actstr = actstr[len("clone("):]
parsedLen = subacts.parse(actstr)
- lst = []
self["attrs"].append(("OVS_ACTION_ATTR_CLONE", subacts))
actstr = actstr[parsedLen:]
parsed = True
@@ -960,14 +954,14 @@ class ovsactions(nla):
actstr = actstr[1:]
if len(actstr) and actstr[0] == ")":
- return (totallen - len(actstr))
+ return totallen - len(actstr)
actstr = actstr[strspn(actstr, ", ") :]
if not parsed:
raise ValueError(f"Action str: '{actstr}' not supported")
- return (totallen - len(actstr))
+ return totallen - len(actstr)
# pyroute2 resolves nla_map types via getattr(self, name).
@@ -1057,8 +1051,6 @@ class ovskey(nla):
if flowstr.startswith("("):
flowstr = flowstr[1:]
- keybits = b""
- maskbits = b""
for f in self.fields_map:
if flowstr.startswith(f[1]):
# the following assumes that the field looks
@@ -1067,7 +1059,7 @@ class ovskey(nla):
flowstr = flowstr[len(f[1]) + 1 :]
splitchar = 0
for c in flowstr:
- if c == "," or c == ")":
+ if c in (",", ")"):
break
splitchar += 1
data = flowstr[:splitchar]
@@ -1631,7 +1623,7 @@ class ovskey(nla):
for prefix, regex, typ, attr_name, mask_val, default_val, v46_flag in fields:
flowstr, value = parse_extract_field(flowstr, prefix, regex, typ, False)
if not attr_name:
- raise Exception("Bad list value in tunnel fields")
+ raise ValueError("Bad list value in tunnel fields")
if value is None and attr_name in forced_include:
value = default_val
@@ -1714,7 +1706,7 @@ class ovskey(nla):
if not noprint:
print_str += ","
- if len(flagsattrs):
+ if flagsattrs:
print_str += f"flags({'|'.join(flagsattrs)})"
print_str += ")"
return print_str
@@ -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])
msg["attrs"].append(["OVS_DP_ATTR_USER_FEATURES", dpfeatures])
@@ -2379,15 +2371,17 @@ class OvsVport(GenericNetlinkSocket):
def type_to_str(vport_type):
if vport_type == OvsVport.OVS_VPORT_TYPE_NETDEV:
return "netdev"
- elif vport_type == OvsVport.OVS_VPORT_TYPE_INTERNAL:
+ if vport_type == OvsVport.OVS_VPORT_TYPE_INTERNAL:
return "internal"
+
raise ValueError(f"Unknown vport type:{int(vport_type)}")
def str_to_type(vport_type):
if vport_type in ["netdev", "gre", "vxlan", "geneve"]:
return OvsVport.OVS_VPORT_TYPE_NETDEV
- elif vport_type == "internal":
+ if vport_type == "internal":
return OvsVport.OVS_VPORT_TYPE_INTERNAL
+
raise ValueError(f"Unknown vport type: '{vport_type}'")
def __init__(self, packet=OvsPacket()):
@@ -2482,7 +2476,7 @@ class OvsVport(GenericNetlinkSocket):
msg["dpifindex"] = dpindex
msg["attrs"].append(["OVS_VPORT_ATTR_NAME", vport_ifname])
- if p == None:
+ if p is None:
p = self.upcall_packet
else:
self.upcall_packet = p
@@ -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))
elif hasattr(args, "flbr"):
rep = ovsdp.info(args.flbr, 0)
if rep is None:
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] selftests: openvswitch: add missing docstrings in ovs-dpctl.py
2026-09-05 10:40 [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
2026-09-05 10:40 ` [PATCH 1/4] selftests: openvswitch: convert %-formatting to f-strings Minxi Hou
2026-09-05 10:40 ` [PATCH 2/4] selftests: openvswitch: fix misc pylint warnings in ovs-dpctl.py Minxi Hou
@ 2026-09-05 10:40 ` Minxi Hou
2026-09-09 12:42 ` netdev-bot+sashiko
2026-09-05 10:40 ` [PATCH 4/4] selftests: openvswitch: suppress pylint complexity warnings Minxi Hou
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Minxi Hou @ 2026-09-05 10:40 UTC (permalink / raw)
To: netdev
Cc: aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms, Minxi Hou
Add one-line docstrings to all module, class, and method
definitions to fix C0114, C0115, and C0116 pylint warnings
(88 instances).
Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
base-commit: 9eab111e765729e93087ff86a2ec9b2ae42d0fa5
.../selftests/net/openvswitch/ovs-dpctl.py | 110 ++++++++++++++++--
1 file changed, 100 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 6a02810fe4ea..5b29aeb4b50e 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -1,5 +1,6 @@
#!/usr/bin/env python3
# SPDX-License-Identifier: GPL-2.0
+"""OVS datapath control utility for kernel selftests."""
# Controls the openvswitch module. Part of the kselftest suite, but
# can be used for some diagnostic purpose as well.
@@ -64,11 +65,13 @@ OVS_FLOW_CMD_SET = 4
UINT32_MAX = 0xFFFFFFFF
def macstr(mac):
+ """Format MAC address bytes as colon-separated hex string."""
outstr = ":".join([f"{i:02X}" for i in mac])
return outstr
def strcspn(str1, str2):
+ """Return index of first char in str1 that is in str2."""
tot = 0
for char in str1:
if str2.find(char) != -1:
@@ -78,6 +81,7 @@ def strcspn(str1, str2):
def strspn(str1, str2):
+ """Return index of first char in str1 that is not in str2."""
tot = 0
for char in str1:
if str2.find(char) == -1:
@@ -87,6 +91,7 @@ def strspn(str1, str2):
def intparse(statestr, defmask="0xffffffff"):
+ """Parse an integer with optional mask from a state string."""
totalparse = strspn(statestr, "0123456789abcdefABCDEFx/")
# scan until "/"
count = strspn(statestr, "x0123456789abcdefABCDEF")
@@ -107,6 +112,7 @@ def intparse(statestr, defmask="0xffffffff"):
def parse_flags(flag_str, flag_vals):
+ """Parse flag string into bitmask and mask values."""
bitResult = 0
maskResult = 0
@@ -158,6 +164,7 @@ def parse_flags(flag_str, flag_vals):
def parse_ct_state(statestr):
+ """Parse conntrack state flags string."""
ct_flags = {
"new": 1 << 0,
"est": 1 << 1,
@@ -173,6 +180,7 @@ def parse_ct_state(statestr):
def convert_mac(data):
+ """Convert MAC address string with optional mask to bytes."""
def to_bytes(mac):
mac_split = mac.split(":")
ret = bytearray([int(i, 16) for i in mac_split])
@@ -188,6 +196,7 @@ def convert_mac(data):
return to_bytes(mac_str), to_bytes(mask_str)
def convert_ipv4(data):
+ """Convert IPv4 address string with optional mask to integers."""
ip, _, mask = data.partition('/')
if not ip:
@@ -200,6 +209,7 @@ def convert_ipv4(data):
return int(ipaddress.IPv4Address(ip)), int(ipaddress.IPv4Address(mask))
def convert_ipv6(data):
+ """Convert IPv6 address string with optional mask to packed bytes."""
ip, _, mask = data.partition('/')
if not ip:
@@ -212,6 +222,7 @@ def convert_ipv6(data):
return ipaddress.IPv6Address(ip).packed, ipaddress.IPv6Address(mask).packed
def convert_int(size):
+ """Return a converter for integer fields of the given bit size."""
def convert_int_sized(data):
value, _, mask = data.partition('/')
@@ -224,6 +235,7 @@ def convert_int(size):
return convert_int_sized
def parse_starts_block(block_str, scanstr, returnskipped, scanregex=False):
+ """Check if block_str starts with scanstr, optionally skip it."""
if scanregex:
m = re.search(scanstr, block_str)
if m is None:
@@ -250,6 +262,7 @@ def parse_starts_block(block_str, scanstr, returnskipped, scanregex=False):
def parse_extract_field(
block_str, fieldstr, scanfmt, convert, masked=False, defval=None
):
+ """Extract a field value from block_str using regex scanfmt."""
if fieldstr and not block_str.startswith(fieldstr):
return block_str, defval
@@ -349,6 +362,7 @@ def parse_attrs(actstr, attr_desc):
class ovs_dp_msg(genlmsg):
+ """OVS datapath generic netlink message."""
# include the OVS version
# We need a custom header rather than just being able to rely on
# genlmsg because fields ends up not expressing everything correctly
@@ -357,6 +371,7 @@ class ovs_dp_msg(genlmsg):
class ovsactions(nla):
+ """OVS datapath actions netlink attribute."""
nla_flags = NLA_F_NESTED
nla_map = (
@@ -372,7 +387,7 @@ class ovsactions(nla):
("OVS_ACTION_ATTR_PUSH_MPLS", "none"),
("OVS_ACTION_ATTR_POP_MPLS", "flag"),
("OVS_ACTION_ATTR_SET_MASKED", "ovskey"),
- ("OVS_ACTION_ATTR_CT", "ctact"),
+ ("OVS_ACTION_ATTR_CT", "CtAct"),
("OVS_ACTION_ATTR_TRUNC", "uint32"),
("OVS_ACTION_ATTR_PUSH_ETH", "none"),
("OVS_ACTION_ATTR_POP_ETH", "flag"),
@@ -399,6 +414,7 @@ class ovsactions(nla):
)
class psample(nla):
+ """Packet sampling action attributes."""
nla_flags = NLA_F_NESTED
nla_map = (
@@ -408,6 +424,7 @@ class ovsactions(nla):
)
def dpstr(self, more=False):
+ """Format psample attributes as dpctl string."""
args = f"group={int(self.get_attr('OVS_PSAMPLE_ATTR_GROUP'))}"
cookie = self.get_attr("OVS_PSAMPLE_ATTR_COOKIE")
@@ -417,6 +434,7 @@ class ovsactions(nla):
return f"psample({args})"
def parse(self, actstr):
+ """Parse psample attributes from dpctl string."""
desc = (
("group", "OVS_PSAMPLE_ATTR_GROUP", int),
("cookie", "OVS_PSAMPLE_ATTR_COOKIE",
@@ -431,9 +449,11 @@ class ovsactions(nla):
return actstr
class push_vlan(nla):
+ """Push VLAN action fields (tpid + tci)."""
fields = (("vlan_tpid", "!H"), ("vlan_tci", "!H"))
class sample(nla):
+ """sample action attributes."""
nla_flags = NLA_F_NESTED
nla_map = (
@@ -443,6 +463,7 @@ class ovsactions(nla):
)
def dpstr(self, more=False):
+ """Format sample attributes as dpctl string."""
args = []
prob = 100 * self.get_attr(
@@ -456,10 +477,11 @@ class ovsactions(nla):
return f"sample({','.join(args)})"
def parse(self, actstr):
+ """Parse sample attributes from dpctl string."""
def parse_nested_actions(actstr):
subacts = ovsactions()
- parsed_len = subacts.parse(actstr)
- return subacts, actstr[parsed_len :]
+ parsedLen = subacts.parse(actstr)
+ return subacts, actstr[parsedLen :]
def percent_to_rate(percent):
percent = float(percent.strip('%'))
@@ -476,7 +498,8 @@ class ovsactions(nla):
return actstr
- class ctact(nla):
+ class CtAct(nla):
+ """Conntrack action attributes."""
nla_flags = NLA_F_NESTED
nla_map = (
@@ -493,6 +516,7 @@ class ovsactions(nla):
)
class natattr(nla):
+ """NAT sub-action attributes."""
nla_flags = NLA_F_NESTED
nla_map = (
@@ -509,6 +533,7 @@ class ovsactions(nla):
)
def dpstr(self, more=False):
+ """Format NAT attributes as dpctl string."""
print_str = "nat("
if self.get_attr("OVS_NAT_ATTR_SRC"):
@@ -547,6 +572,7 @@ class ovsactions(nla):
return print_str
def dpstr(self, more=False):
+ """Format conntrack attributes as dpctl string."""
print_str = "ct("
if self.get_attr("OVS_CT_ATTR_COMMIT") is not None:
@@ -568,6 +594,7 @@ class ovsactions(nla):
return print_str
class userspace(nla):
+ """userspace action attributes."""
nla_flags = NLA_F_NESTED
nla_map = (
@@ -578,6 +605,7 @@ class ovsactions(nla):
)
def dpstr(self, more=False):
+ """Format userspace attributes as dpctl string."""
print_str = "userspace("
if self.get_attr("OVS_USERSPACE_ATTR_PID") is not None:
print_str += f"pid={int(self.get_attr('OVS_USERSPACE_ATTR_PID'))},"
@@ -593,6 +621,7 @@ class ovsactions(nla):
return print_str
def parse(self, actstr):
+ """Parse userspace attributes from dpctl string."""
attrs_desc = (
("pid", "OVS_USERSPACE_ATTR_PID", int),
("userdata", "OVS_USERSPACE_ATTR_USERDATA",
@@ -607,6 +636,7 @@ class ovsactions(nla):
return actstr
def dpstr(self, more=False):
+ """Format all actions as dpctl string."""
print_str = ""
for field in self["attrs"]:
@@ -669,12 +699,13 @@ class ovsactions(nla):
else:
try:
print_str += datum.dpstr(more)
- except:
- print_str += "{ATTR: %s not decoded}" % field[0]
+ except Exception:
+ print_str += f"{{ATTR: {field[0]} not decoded}}"
return print_str
def parse(self, actstr):
+ """Parse actions from dpctl string."""
totallen = len(actstr)
while len(actstr) != 0:
parsed = False
@@ -784,7 +815,7 @@ class ovsactions(nla):
parencount += 2
subacts = ovsactions()
actstr = actstr[len("dec_ttl(le_1("):]
- parsed_len = subacts.parse(actstr)
+ parsedLen = subacts.parse(actstr)
decttl = ovsactions.dec_ttl()
decttl["attrs"].append(
("OVS_DEC_TTL_ATTR_ACTION", subacts)
@@ -792,7 +823,7 @@ class ovsactions(nla):
self["attrs"].append(
("OVS_ACTION_ATTR_DEC_TTL", decttl)
)
- actstr = actstr[parsed_len:]
+ actstr = actstr[parsedLen:]
parsed = True
elif parse_starts_block(actstr, "clone(", False):
parencount += 1
@@ -824,7 +855,7 @@ class ovsactions(nla):
elif parse_starts_block(actstr, "ct(", False):
parencount += 1
actstr = actstr[len("ct(") :]
- ctact = ovsactions.ctact()
+ ctact = ovsactions.CtAct()
for scan in (
("commit", "OVS_CT_ATTR_COMMIT", None),
@@ -851,7 +882,7 @@ class ovsactions(nla):
# sub-action and this lets it sit anywhere in the ct() action
if actstr.startswith("nat"):
actstr = actstr[3:]
- natact = ovsactions.ctact.natattr()
+ natact = ovsactions.CtAct.natattr()
if actstr.startswith("("):
parencount += 1
@@ -971,6 +1002,7 @@ ovsactions.dec_ttl.actions = ovsactions
class ovskey(nla):
+ """OVS flow key netlink attribute."""
nla_flags = NLA_F_NESTED
nla_map = (
("OVS_KEY_ATTR_UNSPEC", "none"),
@@ -1009,6 +1041,7 @@ class ovskey(nla):
)
class ovs_key_proto(nla):
+ """Protocol key fields (ethertype)."""
fields = (
("src", "!H"),
("dst", "!H"),
@@ -1041,6 +1074,7 @@ class ovskey(nla):
)
def parse(self, flowstr, typeInst):
+ """Parse protocol key from dpctl string."""
if not flowstr.startswith(self.proto_str):
return None, None
@@ -1082,6 +1116,7 @@ class ovskey(nla):
return flowstr, k, m
def dpstr(self, masked=None, more=False):
+ """Format protocol key as dpctl string."""
outstr = f"{self.proto_str}("
first = False
for f in self.fields_map:
@@ -1110,6 +1145,7 @@ class ovskey(nla):
return outstr
class ethaddr(ovs_key_proto):
+ """Ethernet address key fields."""
fields = (
("src", "!6s"),
("dst", "!6s"),
@@ -1151,6 +1187,7 @@ class ovskey(nla):
)
class ovs_key_ipv4(ovs_key_proto):
+ """IPv4 key fields."""
fields = (
("src", "!I"),
("dst", "!I"),
@@ -1204,6 +1241,7 @@ class ovskey(nla):
)
class ovs_key_ipv6(ovs_key_proto):
+ """IPv6 key fields."""
fields = (
("src", "!16s"),
("dst", "!16s"),
@@ -1260,6 +1298,7 @@ class ovskey(nla):
)
class ovs_key_tcp(ovs_key_proto):
+ """TCP key fields (src/dst port)."""
def __init__(
self,
data=None,
@@ -1279,6 +1318,7 @@ class ovskey(nla):
)
class ovs_key_udp(ovs_key_proto):
+ """UDP key fields (src/dst port)."""
def __init__(
self,
data=None,
@@ -1298,6 +1338,7 @@ class ovskey(nla):
)
class ovs_key_sctp(ovs_key_proto):
+ """SCTP key fields (src/dst port)."""
def __init__(
self,
data=None,
@@ -1317,6 +1358,7 @@ class ovskey(nla):
)
class ovs_key_icmp(ovs_key_proto):
+ """ICMP key fields (type/code)."""
fields = (
("type", "B"),
("code", "B"),
@@ -1348,6 +1390,7 @@ class ovskey(nla):
)
class ovs_key_icmpv6(ovs_key_icmp):
+ """ICMPv6 key fields (type/code)."""
def __init__(
self,
data=None,
@@ -1367,6 +1410,7 @@ class ovskey(nla):
)
class ovs_key_arp(ovs_key_proto):
+ """ARP key fields."""
fields = (
("sip", "!I"),
("tip", "!I"),
@@ -1427,6 +1471,7 @@ class ovskey(nla):
)
class ovs_key_nd(ovs_key_proto):
+ """Neighbor discovery key fields."""
fields = (
("target", "!16s"),
("sll", "!6s"),
@@ -1463,6 +1508,7 @@ class ovskey(nla):
)
class ovs_key_ct_tuple_ipv4(ovs_key_proto):
+ """Conntrack original tuple key (IPv4)."""
fields = (
("src", "!I"),
("dst", "!I"),
@@ -1510,6 +1556,7 @@ class ovskey(nla):
)
class ovs_key_ct_tuple_ipv6(nla):
+ """Conntrack original tuple key (IPv6)."""
fields = (
("src", "!16s"),
("dst", "!16s"),
@@ -1555,6 +1602,7 @@ class ovskey(nla):
)
class ovs_key_tunnel(nla):
+ """Tunnel key fields."""
nla_flags = NLA_F_NESTED
nla_map = (
@@ -1578,6 +1626,7 @@ class ovskey(nla):
)
def parse(self, flowstr, mask=None):
+ """Parse tunnel key from dpctl string."""
if not flowstr.startswith("tunnel("):
return None, None
@@ -1670,6 +1719,7 @@ class ovskey(nla):
return flowstr, k, mask
def dpstr(self, mask=None, more=False):
+ """Format tunnel key as dpctl string."""
print_str = "tunnel("
flagsattrs = []
@@ -1712,6 +1762,7 @@ class ovskey(nla):
return print_str
class ovs_key_mpls(nla):
+ """MPLS key fields."""
fields = (("lse", ">I"),)
# 802.1Q CFI (Canonical Format Indicator) bit, always set for Ethernet
@@ -1901,6 +1952,7 @@ class ovskey(nla):
return flowstr, inner_key, inner_mask
def parse(self, flowstr, mask=None):
+ """Parse flow key from dpctl string."""
for field in (
("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse),
("OVS_KEY_ATTR_SKB_MARK", "skb_mark", intparse),
@@ -1997,6 +2049,7 @@ class ovskey(nla):
return flowstr
def dpstr(self, mask=None, more=False):
+ """Format flow key as dpctl string."""
print_str = ""
for field in (
@@ -2166,11 +2219,13 @@ class encap_ovskey(ovskey):
class OvsPacket(GenericNetlinkSocket):
+ """OVS packet command message."""
OVS_PACKET_CMD_MISS = 1 # Flow table miss
OVS_PACKET_CMD_ACTION = 2 # USERSPACE action
OVS_PACKET_CMD_EXECUTE = 3 # Apply actions to packet
class ovs_packet_msg(ovs_dp_msg):
+ """OVS packet message header."""
nla_map = (
("OVS_PACKET_ATTR_UNSPEC", "none"),
("OVS_PACKET_ATTR_PACKET", "array(uint8)"),
@@ -2191,6 +2246,7 @@ class OvsPacket(GenericNetlinkSocket):
self.bind(OVS_PACKET_FAMILY, OvsPacket.ovs_packet_msg)
def upcall_handler(self, up=None):
+ """Execute a packet on the datapath."""
print("listening on upcall packet handler:", self.epid)
while True:
try:
@@ -2211,6 +2267,7 @@ class OvsPacket(GenericNetlinkSocket):
class OvsDatapath(GenericNetlinkSocket):
+ """OVS datapath management."""
OVS_DP_F_VPORT_PIDS = 1 << 1
OVS_DP_F_DISPATCH_UPCALL_PER_CPU = 1 << 3
@@ -2232,6 +2289,7 @@ class OvsDatapath(GenericNetlinkSocket):
)
class dpstats(nla):
+ """Datapath info message."""
fields = (
("hit", "=Q"),
("missed", "=Q"),
@@ -2240,6 +2298,7 @@ class OvsDatapath(GenericNetlinkSocket):
)
class megaflowstats(nla):
+ """Datapath statistics."""
fields = (
("mask_hit", "=Q"),
("masks", "=I"),
@@ -2253,6 +2312,7 @@ class OvsDatapath(GenericNetlinkSocket):
self.bind(OVS_DATAPATH_FAMILY, OvsDatapath.dp_cmd_msg)
def info(self, dpname, ifindex=0):
+ """Create a new datapath."""
msg = OvsDatapath.dp_cmd_msg()
msg["cmd"] = OVS_DP_CMD_GET
msg["version"] = OVS_DATAPATH_VERSION
@@ -2276,6 +2336,7 @@ class OvsDatapath(GenericNetlinkSocket):
def create(
self, dpname, shouldUpcall=False, versionStr=None, p=OvsPacket()
):
+ """Destroy a datapath."""
msg = OvsDatapath.dp_cmd_msg()
msg["cmd"] = OVS_DP_CMD_NEW
if versionStr is None:
@@ -2317,6 +2378,7 @@ class OvsDatapath(GenericNetlinkSocket):
return reply
def destroy(self, dpname):
+ """Look up a datapath by name."""
msg = OvsDatapath.dp_cmd_msg()
msg["cmd"] = OVS_DP_CMD_DEL
msg["version"] = OVS_DATAPATH_VERSION
@@ -2339,10 +2401,12 @@ class OvsDatapath(GenericNetlinkSocket):
class OvsVport(GenericNetlinkSocket):
+ """OVS virtual port management."""
OVS_VPORT_TYPE_NETDEV = 1
OVS_VPORT_TYPE_INTERNAL = 2
class ovs_vport_msg(ovs_dp_msg):
+ """Vport info message."""
nla_map = (
("OVS_VPORT_ATTR_UNSPEC", "none"),
("OVS_VPORT_ATTR_PORT_NO", "uint32"),
@@ -2356,7 +2420,9 @@ class OvsVport(GenericNetlinkSocket):
("OVS_VPORT_ATTR_NETNSID", "uint32"),
)
+
class vportstats(nla):
+ """Tunnel options attributes."""
fields = (
("rx_packets", "=Q"),
("tx_packets", "=Q"),
@@ -2368,7 +2434,9 @@ class OvsVport(GenericNetlinkSocket):
("tx_dropped", "=Q"),
)
+ @staticmethod
def type_to_str(vport_type):
+ """Convert vport type integer to string."""
if vport_type == OvsVport.OVS_VPORT_TYPE_NETDEV:
return "netdev"
if vport_type == OvsVport.OVS_VPORT_TYPE_INTERNAL:
@@ -2376,7 +2444,9 @@ class OvsVport(GenericNetlinkSocket):
raise ValueError(f"Unknown vport type:{int(vport_type)}")
+ @staticmethod
def str_to_type(vport_type):
+ """Convert vport type string to integer."""
if vport_type in ["netdev", "gre", "vxlan", "geneve"]:
return OvsVport.OVS_VPORT_TYPE_NETDEV
if vport_type == "internal":
@@ -2390,6 +2460,7 @@ class OvsVport(GenericNetlinkSocket):
self.upcall_packet = packet
def info(self, vport_name, dpifindex=0, portno=None):
+ """Create a new vport."""
msg = OvsVport.ovs_vport_msg()
msg["cmd"] = OVS_VPORT_CMD_GET
@@ -2415,6 +2486,7 @@ class OvsVport(GenericNetlinkSocket):
return reply
def attach(self, dpindex, vport_ifname, ptype, dport):
+ """Get info about a vport."""
msg = OvsVport.ovs_vport_msg()
msg["cmd"] = OVS_VPORT_CMD_NEW
@@ -2468,6 +2540,7 @@ class OvsVport(GenericNetlinkSocket):
return reply
def reset_upcall(self, dpindex, vport_ifname, p=None):
+ """Attach a vport to a datapath."""
msg = OvsVport.ovs_vport_msg()
msg["cmd"] = OVS_VPORT_CMD_SET
@@ -2493,6 +2566,7 @@ class OvsVport(GenericNetlinkSocket):
return reply
def detach(self, dpindex, vport_ifname):
+ """Reset a vport."""
msg = OvsVport.ovs_vport_msg()
msg["cmd"] = OVS_VPORT_CMD_DEL
@@ -2514,11 +2588,14 @@ class OvsVport(GenericNetlinkSocket):
return reply
def upcall_handler(self, handler=None):
+ """Remove a vport from a datapath."""
self.upcall_packet.upcall_handler(handler)
class OvsFlow(GenericNetlinkSocket):
+ """OVS flow table management."""
class ovs_flow_msg(ovs_dp_msg):
+ """Flow info message."""
nla_map = (
("OVS_FLOW_ATTR_UNSPEC", "none"),
("OVS_FLOW_ATTR_KEY", "ovskey"),
@@ -2534,12 +2611,14 @@ class OvsFlow(GenericNetlinkSocket):
)
class flowstats(nla):
+ """Flow key/mask/actions message."""
fields = (
("packets", "=Q"),
("bytes", "=Q"),
)
def dpstr(self, more=False):
+ """Format flow as dpctl string."""
ufid = self.get_attr("OVS_FLOW_ATTR_UFID")
ufid_str = ""
if ufid is not None:
@@ -2606,6 +2685,7 @@ class OvsFlow(GenericNetlinkSocket):
return print_str
def parse(self, flowstr, actstr, dpidx=0):
+ """Parse flow from dpctl string."""
OVS_UFID_F_OMIT_KEY = 1 << 0
OVS_UFID_F_OMIT_MASK = 1 << 1
OVS_UFID_F_OMIT_ACTIONS = 1 << 2
@@ -2770,6 +2850,7 @@ class OvsFlow(GenericNetlinkSocket):
return rep
def miss(self, packetmsg):
+ """Dump all flows for a datapath."""
seq = packetmsg["header"]["sequence_number"]
keystr = "(none)"
key_field = packetmsg.get_attr("OVS_PACKET_ATTR_KEY")
@@ -2782,13 +2863,16 @@ class OvsFlow(GenericNetlinkSocket):
print(f"MISS upcall[{int(seq)}/{pktpres}]: {keystr}", flush=True)
def execute(self, packetmsg):
+ """Delete a flow from a datapath."""
print("userspace execute command", flush=True)
def action(self, packetmsg):
+ """Add a flow to a datapath."""
print("userspace action command", flush=True)
class psample_sample(genlmsg):
+ """psample generic netlink event handler."""
nla_map = (
("PSAMPLE_ATTR_IIFINDEX", "none"),
("PSAMPLE_ATTR_OIFINDEX", "none"),
@@ -2809,6 +2893,7 @@ class psample_sample(genlmsg):
)
def dpstr(self):
+ """Start receiving psample events."""
fields = []
data = ""
for (attr, value) in self["attrs"]:
@@ -2826,6 +2911,7 @@ class psample_sample(genlmsg):
class psample_msg(Marshal):
+ """psample generic netlink message."""
PSAMPLE_CMD_SAMPLE = 0
PSAMPLE_CMD_GET_GROUP = 1
PSAMPLE_CMD_NEW_GROUP = 2
@@ -2835,11 +2921,13 @@ class psample_msg(Marshal):
class PsampleEvent(EventSocket):
+ """psample event listener."""
genl_family = "psample"
mcast_groups = ["packets"]
marshal_class = psample_msg
def read_samples(self):
+ """Set the psample group to listen on."""
print("listening for psample events", flush=True)
while True:
try:
@@ -2850,6 +2938,7 @@ class PsampleEvent(EventSocket):
def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
+ """Print full OVS datapath information."""
dp_name = dp_lookup_rep.get_attr("OVS_DP_ATTR_NAME")
base_stats = dp_lookup_rep.get_attr("OVS_DP_ATTR_STATS")
megaflow_stats = dp_lookup_rep.get_attr("OVS_DP_ATTR_MEGAFLOW_STATS")
@@ -2885,6 +2974,7 @@ def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
def main(argv):
+ """Entry point for ovs-dpctl utility."""
nlmsg_atoms.encap_ovskey = encap_ovskey
nlmsg_atoms.ovskey = ovskey
nlmsg_atoms.ovsactions = ovsactions
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] selftests: openvswitch: suppress pylint complexity warnings
2026-09-05 10:40 [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
` (2 preceding siblings ...)
2026-09-05 10:40 ` [PATCH 3/4] selftests: openvswitch: add missing docstrings " Minxi Hou
@ 2026-09-05 10:40 ` Minxi Hou
2026-09-09 12:42 ` netdev-bot+sashiko
2026-09-06 8:55 ` [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
2026-09-08 17:04 ` [net-next,0/4] " Minxi Hou
5 siblings, 1 reply; 11+ messages in thread
From: Minxi Hou @ 2026-09-05 10:40 UTC (permalink / raw)
To: netdev
Cc: aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms, Minxi Hou
Add file-level pylint:disable comments for warnings caused by
pyroute2 framework constraints that cannot be fixed without
restructuring the netlink attribute hierarchy.
After this patch, pylint reports 9.93/10 with 10 remaining
C0301 line-too-long warnings (81-89 columns) on f-string
constructions that match the surrounding style.
Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
base-commit: 9eab111e765729e93087ff86a2ec9b2ae42d0fa5
tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 5b29aeb4b50e..5948471ffe63 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -1,6 +1,14 @@
#!/usr/bin/env python3
# SPDX-License-Identifier: GPL-2.0
"""OVS datapath control utility for kernel selftests."""
+# pylint: disable=invalid-name,too-many-lines
+# pylint: disable=too-many-ancestors,too-many-arguments
+# pylint: disable=too-many-positional-arguments,too-many-branches
+# pylint: disable=too-many-locals,too-many-statements
+# pylint: disable=too-many-return-statements,too-many-nested-blocks
+# pylint: disable=unused-argument,broad-exception-caught
+# pylint: disable=no-member,not-callable
+# pylint: disable=non-parent-init-called,super-init-not-called
# Controls the openvswitch module. Part of the kselftest suite, but
# can be used for some diagnostic purpose as well.
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py
2026-09-05 10:40 [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
` (3 preceding siblings ...)
2026-09-05 10:40 ` [PATCH 4/4] selftests: openvswitch: suppress pylint complexity warnings Minxi Hou
@ 2026-09-06 8:55 ` Minxi Hou
2026-09-08 17:04 ` [net-next,0/4] " Minxi Hou
5 siblings, 0 replies; 11+ messages in thread
From: Minxi Hou @ 2026-09-06 8:55 UTC (permalink / raw)
To: netdev; +Cc: aconole, echaudron, i.maximets, dev
Hi all,
This series is a respin of the pylint cleanup I posted in May [1],
which got a changes-requested. I forgot to tag it as v2, sorry for
the confusion.
Changes since v1:
- Dropped the patch that renamed the NLA classes to snake_case. Those
names mirror the kernel-side netlink attribute enums, so keeping
them as-is makes the mapping obvious and avoids a large
cosmetic-only diff.
- Rebased onto current net-next (base-commit 9eab111e7657).
- Pylint score is now 9.93/10 (upstream file scores 7.66/10).
On the direction Aaron raised in the v1 thread: moving ovs-dpctl.py
from pyroute2 to the in-tree YNL specs is still the right move, and
the gaps I listed back then (no ovs_packet.yaml or conntrack specs,
datapath flow operations and attributes missing from the family spec)
still stand. This cleanup is complementary to that migration: it
clears the mechanical noise now and shrinks the diff any future YNL
rewrite has to carry. Happy to pick up the migration discussion
separately, offline works for me.
[1] https://lore.kernel.org/netdev/20260513121240.2590767-1-houminxi@gmail.com/
Thanks,
Minxi
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [net-next,0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py
2026-09-05 10:40 [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
` (4 preceding siblings ...)
2026-09-06 8:55 ` [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
@ 2026-09-08 17:04 ` Minxi Hou
5 siblings, 0 replies; 11+ messages in thread
From: Minxi Hou @ 2026-09-08 17:04 UTC (permalink / raw)
To: netdev
Cc: Minxi Hou, Aaron Conole, Eelco Chaudron, Ilya Maximets, dev,
linux-kselftest
Hi,
A v3 of this series is coming. On a fresh review pass I found that
patch 3/4 had its docstrings shifted by one method (they described
the wrong operations) and that two renames that were meant to be
dropped from the original series had leaked back into patches 2/4
and 3/4. v3 rewrites the docstring patch, drops the renames, and
puts the stray hunks back into the patches they belong to.
The pw-ci contest failure was a rebase artifact; v3 is based on
current net-next.
Thanks,
Minxi
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] selftests: openvswitch: convert %-formatting to f-strings
2026-09-05 10:40 ` [PATCH 1/4] selftests: openvswitch: convert %-formatting to f-strings Minxi Hou
@ 2026-09-09 12:42 ` netdev-bot+sashiko
0 siblings, 0 replies; 11+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 12:42 UTC (permalink / raw)
To: houminxi
Cc: netdev, aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] selftests: openvswitch: fix misc pylint warnings in ovs-dpctl.py
2026-09-05 10:40 ` [PATCH 2/4] selftests: openvswitch: fix misc pylint warnings in ovs-dpctl.py Minxi Hou
@ 2026-09-09 12:42 ` netdev-bot+sashiko
0 siblings, 0 replies; 11+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 12:42 UTC (permalink / raw)
To: houminxi
Cc: netdev, aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] selftests: openvswitch: add missing docstrings in ovs-dpctl.py
2026-09-05 10:40 ` [PATCH 3/4] selftests: openvswitch: add missing docstrings " Minxi Hou
@ 2026-09-09 12:42 ` netdev-bot+sashiko
0 siblings, 0 replies; 11+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 12:42 UTC (permalink / raw)
To: houminxi
Cc: netdev, aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 1 (1) · Low: 1
- [High] Roughly 19 of the newly added docstrings in
tools/testing/selftests/net/openvswitch/ovs-dpctl.py describe a…
- [Medium] The commit message states the patch only adds one-line
docstrings to fix C0114/C0115/C0116, but the diff also contains…
- [Low] Four lines are renamed from snake_case `parsed_len` to camelCase
`parsedLen` (in `ovsactions.sample.parse()` at lines 483-484 and in…
Pre-existing issues:
- [Medium] In `OvsFlow.ovs_flow_msg.dpstr()` the UFID formatting computes
`u2l = ufid[2] & 0`, which is unconditionally 0, so the fifth printed…
--
selftests: openvswitch: add missing docstrings in ovs-dpctl.py
This adds one-line docstrings to the module, classes and methods of
tools/testing/selftests/net/openvswitch/ovs-dpctl.py to silence pylint
C0114, C0115 and C0116. Along with the docstrings the patch also renames
a nested netlink attribute class, renames two locals, narrows one except
clause and adds two staticmethod decorators.
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 6a02810fe4eaf..5b29aeb4b50e2 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
[ ... ]
> @@ -372,7 +387,7 @@ class ovsactions(nla):
> ("OVS_ACTION_ATTR_PUSH_MPLS", "none"),
> ("OVS_ACTION_ATTR_POP_MPLS", "flag"),
> ("OVS_ACTION_ATTR_SET_MASKED", "ovskey"),
> - ("OVS_ACTION_ATTR_CT", "ctact"),
> + ("OVS_ACTION_ATTR_CT", "CtAct"),
[Severity: Medium]
The changelog says the patch only adds one-line docstrings to fix C0114,
C0115 and C0116, but this hunk renames the nested pyroute2 attribute class
ovsactions.ctact to ovsactions.CtAct, including the decoder type string in
nla_map and both construction sites in ovsactions.parse()
(ovsactions.CtAct() in the "ct(" branch and ovsactions.CtAct.natattr() in
the nested "nat" branch). Should the rename be split into its own patch,
or at least described in the changelog?
The rename also makes this file inconsistent: every other nested class
referenced from an nla_map here (ovskey, dpstats, megaflowstats,
vportstats, flowstats, natattr) stays lower case.
Three more undisclosed changes come with the same patch:
- the bare except in ovsactions.dpstr() becomes except Exception, and
the "%" format becomes an f-string, which is the W0702 and C0209 work
that the two earlier patches in the series already claim in their own
changelogs
- @staticmethod is added to OvsVport.type_to_str() and
OvsVport.str_to_type(), changing how those two helpers bind
- an extra blank line is inserted in OvsVport.ovs_vport_msg before
class vportstats
[ ... ]
> @@ -456,10 +477,11 @@ class ovsactions(nla):
> return f"sample({','.join(args)})"
>
> def parse(self, actstr):
> + """Parse sample attributes from dpctl string."""
> def parse_nested_actions(actstr):
> subacts = ovsactions()
> - parsed_len = subacts.parse(actstr)
> - return subacts, actstr[parsed_len :]
> + parsedLen = subacts.parse(actstr)
> + return subacts, actstr[parsedLen :]
[Severity: Low]
This isn't a bug, but the snake_case local parsed_len is renamed to
camelCase parsedLen here in ovsactions.sample.parse() and again in the
"dec_ttl(le_1(" branch of ovsactions.parse(). The rename is not mentioned
in the changelog and is unrelated to the docstring work.
For completeness: this does not leave a pylint C0103 behind, because the
final patch in the series adds a file scope
"# pylint: disable=invalid-name,too-many-lines", and the baseline already
used parsedLen in the "clone(" branch, so the file becomes internally
consistent.
>
> def percent_to_rate(percent):
> percent = float(percent.strip('%'))
> @@ -476,7 +498,8 @@ class ovsactions(nla):
>
> return actstr
>
> - class ctact(nla):
> + class CtAct(nla):
> + """Conntrack action attributes."""
> nla_flags = NLA_F_NESTED
>
> nla_map = (
[ ... ]
> @@ -669,12 +699,13 @@ class ovsactions(nla):
> else:
> try:
> print_str += datum.dpstr(more)
> - except:
> - print_str += "{ATTR: %s not decoded}" % field[0]
> + except Exception:
> + print_str += f"{{ATTR: {field[0]} not decoded}}"
[ ... ]
> @@ -824,7 +855,7 @@ class ovsactions(nla):
> elif parse_starts_block(actstr, "ct(", False):
> parencount += 1
> actstr = actstr[len("ct(") :]
> - ctact = ovsactions.ctact()
> + ctact = ovsactions.CtAct()
[ ... ]
> @@ -1009,6 +1041,7 @@ class ovskey(nla):
> )
>
> class ovs_key_proto(nla):
> + """Protocol key fields (ethertype)."""
> fields = (
> ("src", "!H"),
> ("dst", "!H"),
[Severity: High]
Does ovs_key_proto carry an ethertype? Its fields and fields_map only
hold src and dst as "!H", and ovs_key_tcp, ovs_key_udp and ovs_key_sctp
inherit it for L4 ports. Should the docstring describe the generic
src/dst port base class instead?
>From here on the added descriptions look shifted by one definition, so a
number of them document the neighbouring definition rather than the one
they are attached to. The remaining cases are noted below.
[ ... ]
> @@ -2191,6 +2246,7 @@ class OvsPacket(GenericNetlinkSocket):
> self.bind(OVS_PACKET_FAMILY, OvsPacket.ovs_packet_msg)
>
> def upcall_handler(self, up=None):
> + """Execute a packet on the datapath."""
> print("listening on upcall packet handler:", self.epid)
> while True:
> try:
[Severity: High]
Does OvsPacket.upcall_handler() execute a packet? The body is a blocking
"while True: self.get()" receive and dispatch loop and never sends
OVS_PACKET_CMD_EXECUTE.
[ ... ]
> @@ -2232,6 +2289,7 @@ class OvsDatapath(GenericNetlinkSocket):
> )
>
> class dpstats(nla):
> + """Datapath info message."""
> fields = (
> ("hit", "=Q"),
> ("missed", "=Q"),
> @@ -2240,6 +2298,7 @@ class OvsDatapath(GenericNetlinkSocket):
> )
>
> class megaflowstats(nla):
> + """Datapath statistics."""
> fields = (
> ("mask_hit", "=Q"),
> ("masks", "=I"),
[Severity: High]
Are these two descriptions swapped? dpstats holds hit/missed/lost/flows,
which are statistics rather than an info message, and megaflowstats holds
the megaflow specific mask_hit/masks/cache_hits rather than the generic
datapath statistics.
> @@ -2253,6 +2312,7 @@ class OvsDatapath(GenericNetlinkSocket):
> self.bind(OVS_DATAPATH_FAMILY, OvsDatapath.dp_cmd_msg)
>
> def info(self, dpname, ifindex=0):
> + """Create a new datapath."""
> msg = OvsDatapath.dp_cmd_msg()
> msg["cmd"] = OVS_DP_CMD_GET
> msg["version"] = OVS_DATAPATH_VERSION
> @@ -2276,6 +2336,7 @@ class OvsDatapath(GenericNetlinkSocket):
> def create(
> self, dpname, shouldUpcall=False, versionStr=None, p=OvsPacket()
> ):
> + """Destroy a datapath."""
> msg = OvsDatapath.dp_cmd_msg()
> msg["cmd"] = OVS_DP_CMD_NEW
> if versionStr is None:
> @@ -2317,6 +2378,7 @@ class OvsDatapath(GenericNetlinkSocket):
> return reply
>
> def destroy(self, dpname):
> + """Look up a datapath by name."""
> msg = OvsDatapath.dp_cmd_msg()
> msg["cmd"] = OVS_DP_CMD_DEL
> msg["version"] = OVS_DATAPATH_VERSION
[Severity: High]
These three descriptions do not match the commands the bodies build:
OvsDatapath.info() msg["cmd"] = OVS_DP_CMD_GET -> "Create a new datapath."
OvsDatapath.create() msg["cmd"] = OVS_DP_CMD_NEW -> "Destroy a datapath."
OvsDatapath.destroy() msg["cmd"] = OVS_DP_CMD_DEL -> "Look up a datapath by name."
Can the docstrings be moved back onto the methods they describe? As
written, a reader extending these selftests is told that create() destroys
a datapath and that destroy() is a harmless lookup.
pylint C0115 and C0116 are satisfied by any non-empty string, so no tool
will flag these.
> @@ -2356,7 +2420,9 @@ class OvsVport(GenericNetlinkSocket):
> ("OVS_VPORT_ATTR_NETNSID", "uint32"),
> )
>
> +
> class vportstats(nla):
> + """Tunnel options attributes."""
> fields = (
> ("rx_packets", "=Q"),
> ("tx_packets", "=Q"),
[Severity: High]
vportstats decodes OVS_VPORT_ATTR_STATS and holds rx/tx packets, bytes,
errors and dropped counters. Should this say vport statistics rather than
tunnel options?
[ ... ]
> @@ -2390,6 +2460,7 @@ class OvsVport(GenericNetlinkSocket):
> self.upcall_packet = packet
>
> def info(self, vport_name, dpifindex=0, portno=None):
> + """Create a new vport."""
> msg = OvsVport.ovs_vport_msg()
>
> msg["cmd"] = OVS_VPORT_CMD_GET
> @@ -2415,6 +2486,7 @@ class OvsVport(GenericNetlinkSocket):
> return reply
>
> def attach(self, dpindex, vport_ifname, ptype, dport):
> + """Get info about a vport."""
> msg = OvsVport.ovs_vport_msg()
>
> msg["cmd"] = OVS_VPORT_CMD_NEW
> @@ -2468,6 +2540,7 @@ class OvsVport(GenericNetlinkSocket):
> return reply
>
> def reset_upcall(self, dpindex, vport_ifname, p=None):
> + """Attach a vport to a datapath."""
> msg = OvsVport.ovs_vport_msg()
>
> msg["cmd"] = OVS_VPORT_CMD_SET
> @@ -2493,6 +2566,7 @@ class OvsVport(GenericNetlinkSocket):
> return reply
>
> def detach(self, dpindex, vport_ifname):
> + """Reset a vport."""
> msg = OvsVport.ovs_vport_msg()
>
> msg["cmd"] = OVS_VPORT_CMD_DEL
> @@ -2514,11 +2588,14 @@ class OvsVport(GenericNetlinkSocket):
> return reply
>
> def upcall_handler(self, handler=None):
> + """Remove a vport from a datapath."""
> self.upcall_packet.upcall_handler(handler)
[Severity: High]
The same one definition shift appears across all five vport helpers:
OvsVport.info() OVS_VPORT_CMD_GET -> "Create a new vport."
OvsVport.attach() OVS_VPORT_CMD_NEW -> "Get info about a vport."
OvsVport.reset_upcall() OVS_VPORT_CMD_SET -> "Attach a vport to a datapath."
OvsVport.detach() OVS_VPORT_CMD_DEL -> "Reset a vport."
OvsVport.upcall_handler() delegates to the packet upcall loop ->
"Remove a vport from a datapath."
> @@ -2534,12 +2611,14 @@ class OvsFlow(GenericNetlinkSocket):
> )
>
> class flowstats(nla):
> + """Flow key/mask/actions message."""
> fields = (
> ("packets", "=Q"),
> ("bytes", "=Q"),
> )
[Severity: High]
flowstats decodes OVS_FLOW_ATTR_STATS and only holds packets and bytes.
Is the key/mask/actions description meant for the enclosing
ovs_flow_msg instead?
>
> def dpstr(self, more=False):
> + """Format flow as dpctl string."""
> ufid = self.get_attr("OVS_FLOW_ATTR_UFID")
> ufid_str = ""
> if ufid is not None:
[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the
docstring added to OvsFlow.ovs_flow_msg.dpstr() the UFID formatting just
below masks with 0:
u1l = ufid[1] & 0xFFFF
u2h = ufid[2] >> 16
u2l = ufid[2] & 0
u3 = ufid[3]
ufid_str = (f"ufid:{u0:08x}-{u1h:04x}-{u1l:04x}"
f"-{u2h:04x}-{u2l:04x}{u3:08x}")
Should u2l be "ufid[2] & 0xFFFF" like the sibling u1l line? As written the
fifth group of the printed UFID is always 0000, so a UFID printed by
dump-flows cannot be matched back to the kernel flow.
The same "ufid[2] & 0," is present in the baseline inside the older
.format() call, and the u0/u1h/u1l/u2h/u2l/u3 temporaries were introduced
by the earlier f-string conversion patch in this series, so this commit
only added the docstring here.
[ ... ]
> @@ -2770,6 +2850,7 @@ class OvsFlow(GenericNetlinkSocket):
> return rep
>
> def miss(self, packetmsg):
> + """Dump all flows for a datapath."""
> seq = packetmsg["header"]["sequence_number"]
> keystr = "(none)"
> key_field = packetmsg.get_attr("OVS_PACKET_ATTR_KEY")
> @@ -2782,13 +2863,16 @@ class OvsFlow(GenericNetlinkSocket):
> print(f"MISS upcall[{int(seq)}/{pktpres}]: {keystr}", flush=True)
>
> def execute(self, packetmsg):
> + """Delete a flow from a datapath."""
> print("userspace execute command", flush=True)
>
> def action(self, packetmsg):
> + """Add a flow to a datapath."""
> print("userspace action command", flush=True)
[Severity: High]
OvsFlow.miss(), OvsFlow.execute() and OvsFlow.action() are upcall
callbacks that only print a line; none of them touches the flow table.
Do the dump, delete and add descriptions belong on OvsFlow.dump(),
OvsFlow.del_flows() and OvsFlow.add_flow()?
>
>
> class psample_sample(genlmsg):
> + """psample generic netlink event handler."""
> nla_map = (
> ("PSAMPLE_ATTR_IIFINDEX", "none"),
> ("PSAMPLE_ATTR_OIFINDEX", "none"),
> @@ -2809,6 +2893,7 @@ class psample_sample(genlmsg):
> )
>
> def dpstr(self):
> + """Start receiving psample events."""
> fields = []
> data = ""
> for (attr, value) in self["attrs"]:
[ ... ]
> @@ -2835,11 +2921,13 @@ class psample_msg(Marshal):
>
>
> class PsampleEvent(EventSocket):
> + """psample event listener."""
> genl_family = "psample"
> mcast_groups = ["packets"]
> marshal_class = psample_msg
>
> def read_samples(self):
> + """Set the psample group to listen on."""
> print("listening for psample events", flush=True)
> while True:
> try:
[Severity: High]
These two look swapped as well: psample_sample.dpstr() builds and returns
a formatted string, while PsampleEvent.read_samples() is the receive loop
that starts consuming psample events and does not set any group.
Counting the cases above, roughly nineteen of the added docstrings describe
a different definition than the one they annotate. Checking the rest of
the series, the following patch only adds file scope pylint disables, so
these descriptions are still in place at the end of the range
(c4f118a1c0c71574519379960d46bd12779c6c9c).
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260905104026.3776396-1-houminxi%40gmail.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] selftests: openvswitch: suppress pylint complexity warnings
2026-09-05 10:40 ` [PATCH 4/4] selftests: openvswitch: suppress pylint complexity warnings Minxi Hou
@ 2026-09-09 12:42 ` netdev-bot+sashiko
0 siblings, 0 replies; 11+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 12:42 UTC (permalink / raw)
To: houminxi
Cc: netdev, aconole, davem, dev, echaudro, edumazet, i.maximets, kuba,
linux-kernel, linux-kselftest, pabeni, shuah, horms
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 1
- [Medium] Commit message mischaracterizes the change: the subject says
"complexity warnings" and the body claims all suppressed warnings are…
- [Medium] The docstrings added by this series' commit "selftests:
openvswitch: add missing docstrings in ovs-dpctl.py" document the…
- [Low] The file-wide `broad-exception-caught` (W0718) disable suppresses
a warning that this patch series itself created, not a pyroute2…
--
selftests: openvswitch: suppress pylint complexity warnings
This adds a block of file-level pylint disable comments to
tools/testing/selftests/net/openvswitch/ovs-dpctl.py, covering 16 message
names. The commit message attributes all of them to pyroute2 framework
constraints and reports a resulting score of 9.93/10. A few questions
about the scope of the suppression and about docstrings added earlier in
this series follow.
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 5b29aeb4b50e2..5948471ffe63c 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -1,6 +1,14 @@
> #!/usr/bin/env python3
> # SPDX-License-Identifier: GPL-2.0
> """OVS datapath control utility for kernel selftests."""
> +# pylint: disable=invalid-name,too-many-lines
> +# pylint: disable=too-many-ancestors,too-many-arguments
> +# pylint: disable=too-many-positional-arguments,too-many-branches
> +# pylint: disable=too-many-locals,too-many-statements
> +# pylint: disable=too-many-return-statements,too-many-nested-blocks
> +# pylint: disable=unused-argument,broad-exception-caught
> +# pylint: disable=no-member,not-callable
> +# pylint: disable=non-parent-init-called,super-init-not-called
[Severity: Medium]
Does the commit message match what this block actually does? The subject
says "complexity warnings" and the body says the warnings are "caused by
pyroute2 framework constraints that cannot be fixed without restructuring
the netlink attribute hierarchy", but only too-many-ancestors,
non-parent-init-called, super-init-not-called and partly invalid-name come
from subclassing pyroute2 nla/genlmsg.
The complexity metrics come from this script's own functions, not from
pyroute2. main() is a roughly 200-line if/elif chain, and
ovsactions.parse(), ovskey.parse() and ovskey.dpstr() are similarly large:
def main(argv):
"""Entry point for ovs-dpctl utility."""
nlmsg_atoms.encap_ovskey = encap_ovskey
...
Those are reachable by ordinary refactoring, so is "cannot be fixed"
accurate for too-many-branches, too-many-locals, too-many-statements,
too-many-return-statements, too-many-nested-blocks, too-many-arguments and
too-many-positional-arguments?
Separately, no-member and not-callable are correctness checks (misspelled
attribute, calling a non-callable). Disabling them for the whole 3190-line
file means pylint will no longer flag an attribute typo or an API misuse
anywhere in this file for any future edit. Could those two be scoped to the
specific pyroute2 dynamic-attribute sites instead?
The file already shows the narrow form working:
class dec_ttl(nla): # pylint: disable=invalid-name
"""Nested OVS_DEC_TTL_ATTR_* sub-attributes."""
That inline disable is now dead, since invalid-name is disabled file-wide by
this patch. Should it be dropped, or should the file-wide invalid-name be
dropped in favour of per-site disables?
[Severity: Low]
On the broad-exception-caught entry specifically: was that warning created by
this series rather than by pyroute2? At the baseline commit 9eab111e there is
no "except Exception" in the file at all; it had a bare "except:". An earlier
commit in this series converted it, and the result is the only W0718 site in
the file:
try:
print_str += datum.dpstr(more)
except Exception:
print_str += f"{{ATTR: {field[0]} not decoded}}"
Since that is a single call site in ovsactions.dpstr(), would an inline
disable on that line, or catching a narrower set such as
"except (KeyError, TypeError, ValueError)", be preferable to a file-wide
disable?
Also, could the commit message mention that this warning came from the
series itself instead of attributing it to pyroute2?
[Severity: Medium]
Not about this hunk, but while looking at the file for the pylint scope
question: the docstrings added by the earlier series commit "selftests:
openvswitch: add missing docstrings in ovs-dpctl.py" appear to be shifted by
one definition, so each one describes a neighbouring member.
In OvsDatapath:
def info(self, dpname, ifindex=0):
"""Create a new datapath."""
msg["cmd"] = OVS_DP_CMD_GET
def create(self, dpname, shouldUpcall=False, versionStr=None, p=OvsPacket()):
"""Destroy a datapath."""
msg["cmd"] = OVS_DP_CMD_NEW
def destroy(self, dpname):
"""Look up a datapath by name."""
msg["cmd"] = OVS_DP_CMD_DEL
Should destroy(), which sends OVS_DP_CMD_DEL, be documented as a lookup?
The same shift runs through five OvsVport methods: info() sends
OVS_VPORT_CMD_GET but says "Create a new vport.", attach() sends
OVS_VPORT_CMD_NEW but says "Get info about a vport.", reset_upcall() sends
OVS_VPORT_CMD_SET but says "Attach a vport to a datapath.", detach() sends
OVS_VPORT_CMD_DEL but says "Reset a vport.", and upcall_handler(), which only
forwards to the packet handler, says "Remove a vport from a datapath."
The OvsFlow upcall helpers are affected too:
def miss(self, packetmsg):
"""Dump all flows for a datapath."""
...
print(f"MISS upcall[{int(seq)}/{pktpres}]: {keystr}", flush=True)
def execute(self, packetmsg):
"""Delete a flow from a datapath."""
print("userspace execute command", flush=True)
def action(self, packetmsg):
"""Add a flow to a datapath."""
print("userspace action command", flush=True)
And the psample side:
class psample_sample(genlmsg):
"""psample generic netlink event handler."""
...
def dpstr(self):
"""Start receiving psample events."""
class PsampleEvent(EventSocket):
...
def read_samples(self):
"""Set the psample group to listen on."""
print("listening for psample events", flush=True)
while True:
The nested field structs show it as well: vportstats, whose fields are
rx_packets/tx_packets/..., is documented as "Tunnel options attributes.",
flowstats (packets/bytes) as "Flow key/mask/actions message.", dpstats
(hit/missed/lost/flows) as "Datapath info message." and megaflowstats as
"Datapath statistics."
Given that the shift covers at least 18 members, would it make sense to
re-audit all 88 added docstrings rather than fixing them one at a time?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260905104026.3776396-1-houminxi%40gmail.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-09-09 12:42 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-05 10:40 [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
2026-09-05 10:40 ` [PATCH 1/4] selftests: openvswitch: convert %-formatting to f-strings Minxi Hou
2026-09-09 12:42 ` netdev-bot+sashiko
2026-09-05 10:40 ` [PATCH 2/4] selftests: openvswitch: fix misc pylint warnings in ovs-dpctl.py Minxi Hou
2026-09-09 12:42 ` netdev-bot+sashiko
2026-09-05 10:40 ` [PATCH 3/4] selftests: openvswitch: add missing docstrings " Minxi Hou
2026-09-09 12:42 ` netdev-bot+sashiko
2026-09-05 10:40 ` [PATCH 4/4] selftests: openvswitch: suppress pylint complexity warnings Minxi Hou
2026-09-09 12:42 ` netdev-bot+sashiko
2026-09-06 8:55 ` [PATCH net-next 0/4] selftests: openvswitch: pylint cleanup of ovs-dpctl.py Minxi Hou
2026-09-08 17:04 ` [net-next,0/4] " Minxi Hou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox