Re: [PATCH 1/4] selftests: openvswitch: convert %-formatting to f-strings

[netdev-bot+sashiko@kernel.org]

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