ynl: maybe minor bug?

Commit Message

Joe Damato April 24, 2024, 11:19 p.m. UTC

Hi there:

I am probably just doing something wrong, but I tried to run
tools/testing/selftests/drivers/net/stats.py today and hit what is possibly
a bug?

Background info: Python 3.12.3

I'm using net-next at commit 9dd15d5088e9 ("Merge branch
'sparx5-port-mirroring'") with a couple driver modifications added on top
of it that don't seem relevant to the two test failures I'm hitting:

1. "loopback has no stats", and
2. "Try to get stats for lowest unused ifindex but not 0"

Both of these tests expect the ynl library to raise an exception, but I
don't think it does, from tools/net/ynl/lib/ynl.py, the _ops method:

  if nl_msg.error:
      raise NlError(nl_msg)
  if nl_msg.done:
      if nl_msg.extack:
          print("Netlink warning:")
          print(nl_msg)

And the code in net/core/netdev-genl.c seems to set:

 else {
        NL_SET_BAD_ATTR(info->extack,
                        info->attrs[NETDEV_A_QSTATS_IFINDEX]);
        err = netdev ? -EOPNOTSUPP : -ENODEV;

which is what cli.py says:

$ ./cli.py --spec ../../../Documentation/netlink/specs/netdev.yaml \
           --dump qstats-get --json '{"ifindex": "1"}'
Netlink warning:
nl_len = 28 (12) nl_flags = 0x202 nl_type = 3
	extack: {'bad-attr': '.ifindex'}
[]

that seems to be the warning print out from the above
tools/net/ynl/lib/ynl.py snippet, not an NlError, which is what you'd get
if you tried ifindex 0 (which is listed as out of range in the YAML spec):

$ ./cli.py --spec ../../../Documentation/netlink/specs/netdev.yaml \
           --dump qstats-get --json '{"ifindex": "0"}'

Netlink error: Numerical result out of range
nl_len = 108 (92) nl_flags = 0x300 nl_type = 2
	error: -34
        extack: {'msg': 'integer out of range', 'policy': {'min-value': 1,
        'max-value': 4294967295, 'type': 'u32'}, 'bad-attr': '.ifindex'}

I'm not sure whether:

1. tools/net/ynl/lib/ynl.py should be raising NlError when there is an
   extack in this case (I think this is probably the way to go?), or

2. the tests should be changed so that they don't expect an exception to be
   raised but (ideally?) hide the warning report from tools/net/ynl/lib/ynl.py
   when the warning is expected.

I don't know python at all so this is definitely wrong, but here's a small
change I made to fix the test (a similar change was made for the test which
follows).

The following patch is not intended to be seriously considered for
application, just to highlight the issue I am hitting:


Thanks,
Joe

Comments

Jakub Kicinski April 24, 2024, 11:43 p.m. UTC | #1

On Wed, 24 Apr 2024 16:19:49 -0700 Joe Damato wrote:
> I'm not sure whether:
> 
> 1. tools/net/ynl/lib/ynl.py should be raising NlError when there is an
>    extack in this case (I think this is probably the way to go?), or
> 
> 2. the tests should be changed so that they don't expect an exception to be
>    raised but (ideally?) hide the warning report from tools/net/ynl/lib/ynl.py
>    when the warning is expected.
> 
> I don't know python at all so this is definitely wrong, but here's a small
> change I made to fix the test (a similar change was made for the test which
> follows).

Sorry about this, we pushed the related fix to the net tree, to get it
to Linus sooner:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=a44f2eb106a46f2275a79de54ce0ea63e4f3d8c8

You can cherry-pick that locally, perhaps. net will get merged into
net-next tomorrow, I should have waited before extending the test :(

Patch

diff --git a/tools/testing/selftests/drivers/net/stats.py b/tools/testing/selftests/drivers/net/stats.py
index 7a7b16b180e2..d9f5d1f3ed34 100755
--- a/tools/testing/selftests/drivers/net/stats.py
+++ b/tools/testing/selftests/drivers/net/stats.py
@@ -115,9 +115,8 @@  def qstat_by_ifindex(cfg) -> None:
     ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.ifindex')

     # loopback has no stats
-    with ksft_raises(NlError) as cm:
-        netfam.qstats_get({"ifindex": 1}, dump=True)
-    ksft_eq(cm.exception.nl_msg.error, -95)
+    stats = netfam.qstats_get({"ifindex": 1}, dump=True)
+    ksft_eq(cm.exception.nl_msg.error, -34)
     ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.ifindex')