Re: [PATCH net-next v3 3/4] selftests: openvswitch: add missing docstrings in ovs-dpctl.py

[Aaron Conole <aconole@redhat.com>]

netdev-bot+sashiko@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 0 · Low: 1
>
> - [Low] The docstring added to print_ovsdp_full()
>   (tools/testing/selftests/net/openvswitch/ovs-dpctl.py line 2940)…
>
> --
>
> selftests: openvswitch: add missing docstrings in ovs-dpctl.py
>
> This adds one-line docstrings to the module, classes and methods in the
> openvswitch selftest helper ovs-dpctl.py to silence pylint C0114, C0115
> and C0116. No code behaviour is changed.
>
>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> index 502e6eb4c4b4..dc88cb1b20c1 100644
>> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>
> [ ... ]
>
>> @@ -2887,6 +2973,7 @@ class PsampleEvent(EventSocket):
>>  
>>  
>>  def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
>> +    """Print full datapath state: stats, vports, and flows."""
>>  	  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")
>
> [Severity: Low]
> Does print_ovsdp_full() actually print flows?  The body only emits the
> datapath name, the values from OVS_DP_ATTR_STATS,
> OVS_DP_ATTR_MEGAFLOW_STATS, OVS_DP_ATTR_MASKS_CACHE_SIZE and
> OVS_DP_ATTR_USER_FEATURES, then one line per vport:
>
>     print(f"  flows:{int(base_stats['flows'])}")
>     ...
>     # port print out
>     for iface in ndb.interfaces:
>         rep = vpl.info(iface.ifname, ifindex)
>         if rep is not None:
>             print(
>                 f"  port {int(rep.get_attr('OVS_VPORT_ATTR_PORT_NO'))}: "
>                 ...
>
> The only flow-related output is the flows:<n> counter that comes from
> OVS_DP_ATTR_STATS, which is already covered by "stats".
>
> Flow enumeration lives in OvsFlow.dump(), and main() calls that only for
> the dump subcommand, not on the show path that reaches
> print_ovsdp_full().
>
> Would something like "Print datapath statistics and attached vports." be
> a more accurate one-liner here?

I agree with this verbiage.

> The commit message says "Each docstring describes the command or
> attribute group the code actually implements", so this one looks like it
> overstates the output.