[PATCH net] tools: ynl: fix mixing ops and notifications on one socket

[PATCH net] tools: ynl: fix mixing ops and notifications on one socket

[Jakub Kicinski]

The multi message support loosened the connection between the request
and response handling, as we can now submit multiple requests before
we start processing responses. Passing the attr set to NlMsgs decoding
no longer makes sense (if it ever did), attr set may differ message
by messsage. Isolate the part of decoding responsible for attr-set
specific interpretation and call it once we identified the correct op.

Without this fix performing SET operation on an ethtool socket, while
being subscribed to notifications causes:

 # File "tools/net/ynl/pyynl/lib/ynl.py", line 1096, in _op
 # Exception|     return self._ops(ops)[0]
 # Exception|            ~~~~~~~~~^^^^^
 # File "tools/net/ynl/pyynl/lib/ynl.py", line 1040, in _ops
 # Exception|     nms = NlMsgs(reply, attr_space=op.attr_set)
 # Exception|                                    ^^^^^^^^^^^

The value of op we use on line 1040 is stale, it comes form the previous
loop. If a notification comes before a response we will update op to None
and the next iteration thru the loop will break with the trace above.

Fixes: 6fda63c45fe8 ("tools/net/ynl: fix cli.py --subscribe feature")
Fixes: ba8be00f68f5 ("tools/net/ynl: Add multi message support to ynl")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: donald.hunter@gmail.com
CC: arkadiusz.kubalewski@intel.com
---
 tools/net/ynl/pyynl/lib/ynl.py | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/tools/net/ynl/pyynl/lib/ynl.py b/tools/net/ynl/pyynl/lib/ynl.py
index ae4d1ef7b83a..7529bce174ff 100644
--- a/tools/net/ynl/pyynl/lib/ynl.py
+++ b/tools/net/ynl/pyynl/lib/ynl.py
@@ -231,14 +231,7 @@ from .nlspec import SpecFamily
                     self.extack['unknown'].append(extack)
 
             if attr_space:
-                # We don't have the ability to parse nests yet, so only do global
-                if 'miss-type' in self.extack and 'miss-nest' not in self.extack:
-                    miss_type = self.extack['miss-type']
-                    if miss_type in attr_space.attrs_by_val:
-                        spec = attr_space.attrs_by_val[miss_type]
-                        self.extack['miss-type'] = spec['name']
-                        if 'doc' in spec:
-                            self.extack['miss-type-doc'] = spec['doc']
+                self.annotate_extack(attr_space)
 
     def _decode_policy(self, raw):
         policy = {}
@@ -264,6 +257,18 @@ from .nlspec import SpecFamily
                 policy['mask'] = attr.as_scalar('u64')
         return policy
 
+    def annotate_extack(self, attr_space):
+        """ Make extack more human friendly with attribute information """
+
+        # We don't have the ability to parse nests yet, so only do global
+        if 'miss-type' in self.extack and 'miss-nest' not in self.extack:
+            miss_type = self.extack['miss-type']
+            if miss_type in attr_space.attrs_by_val:
+                spec = attr_space.attrs_by_val[miss_type]
+                self.extack['miss-type'] = spec['name']
+                if 'doc' in spec:
+                    self.extack['miss-type-doc'] = spec['doc']
+
     def cmd(self):
         return self.nl_type
 
@@ -277,12 +282,12 @@ from .nlspec import SpecFamily
 
 
 class NlMsgs:
-    def __init__(self, data, attr_space=None):
+    def __init__(self, data):
         self.msgs = []
 
         offset = 0
         while offset < len(data):
-            msg = NlMsg(data, offset, attr_space=attr_space)
+            msg = NlMsg(data, offset)
             offset += msg.nl_len
             self.msgs.append(msg)
 
@@ -1036,12 +1041,13 @@ genl_family_name_to_id = None
         op_rsp = []
         while not done:
             reply = self.sock.recv(self._recv_size)
-            nms = NlMsgs(reply, attr_space=op.attr_set)
+            nms = NlMsgs(reply)
             self._recv_dbg_print(reply, nms)
             for nl_msg in nms:
                 if nl_msg.nl_seq in reqs_by_seq:
                     (op, vals, req_msg, req_flags) = reqs_by_seq[nl_msg.nl_seq]
                     if nl_msg.extack:
+                        nl_msg.annotate_extack(op.attr_set)
                         self._decode_extack(req_msg, op, nl_msg.extack, vals)
                 else:
                     op = None
-- 
2.49.0

Re: [PATCH net] tools: ynl: fix mixing ops and notifications on one socket

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> The multi message support loosened the connection between the request
> and response handling, as we can now submit multiple requests before
> we start processing responses. Passing the attr set to NlMsgs decoding
> no longer makes sense (if it ever did), attr set may differ message
> by messsage. Isolate the part of decoding responsible for attr-set
> specific interpretation and call it once we identified the correct op.
>
> Without this fix performing SET operation on an ethtool socket, while
> being subscribed to notifications causes:
>
>  # File "tools/net/ynl/pyynl/lib/ynl.py", line 1096, in _op
>  # Exception|     return self._ops(ops)[0]
>  # Exception|            ~~~~~~~~~^^^^^
>  # File "tools/net/ynl/pyynl/lib/ynl.py", line 1040, in _ops
>  # Exception|     nms = NlMsgs(reply, attr_space=op.attr_set)
>  # Exception|                                    ^^^^^^^^^^^
>
> The value of op we use on line 1040 is stale, it comes form the previous
> loop. If a notification comes before a response we will update op to None
> and the next iteration thru the loop will break with the trace above.
>
> Fixes: 6fda63c45fe8 ("tools/net/ynl: fix cli.py --subscribe feature")
> Fixes: ba8be00f68f5 ("tools/net/ynl: Add multi message support to ynl")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Good catch, fix LGTM. It looks like a followup refactor could combine
annotate_extack and _decode_extack and get rid of the attr_space
parameter to NlMsg(). WDYT?

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

> ---
> CC: donald.hunter@gmail.com
> CC: arkadiusz.kubalewski@intel.com
> ---
>  tools/net/ynl/pyynl/lib/ynl.py | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/tools/net/ynl/pyynl/lib/ynl.py b/tools/net/ynl/pyynl/lib/ynl.py
> index ae4d1ef7b83a..7529bce174ff 100644
> --- a/tools/net/ynl/pyynl/lib/ynl.py
> +++ b/tools/net/ynl/pyynl/lib/ynl.py
> @@ -231,14 +231,7 @@ from .nlspec import SpecFamily
>                      self.extack['unknown'].append(extack)
>  
>              if attr_space:
> -                # We don't have the ability to parse nests yet, so only do global
> -                if 'miss-type' in self.extack and 'miss-nest' not in self.extack:
> -                    miss_type = self.extack['miss-type']
> -                    if miss_type in attr_space.attrs_by_val:
> -                        spec = attr_space.attrs_by_val[miss_type]
> -                        self.extack['miss-type'] = spec['name']
> -                        if 'doc' in spec:
> -                            self.extack['miss-type-doc'] = spec['doc']
> +                self.annotate_extack(attr_space)
>  
>      def _decode_policy(self, raw):
>          policy = {}
> @@ -264,6 +257,18 @@ from .nlspec import SpecFamily
>                  policy['mask'] = attr.as_scalar('u64')
>          return policy
>  
> +    def annotate_extack(self, attr_space):
> +        """ Make extack more human friendly with attribute information """
> +
> +        # We don't have the ability to parse nests yet, so only do global
> +        if 'miss-type' in self.extack and 'miss-nest' not in self.extack:
> +            miss_type = self.extack['miss-type']
> +            if miss_type in attr_space.attrs_by_val:
> +                spec = attr_space.attrs_by_val[miss_type]
> +                self.extack['miss-type'] = spec['name']
> +                if 'doc' in spec:
> +                    self.extack['miss-type-doc'] = spec['doc']
> +
>      def cmd(self):
>          return self.nl_type
>  
> @@ -277,12 +282,12 @@ from .nlspec import SpecFamily
>  
>  
>  class NlMsgs:
> -    def __init__(self, data, attr_space=None):
> +    def __init__(self, data):
>          self.msgs = []
>  
>          offset = 0
>          while offset < len(data):
> -            msg = NlMsg(data, offset, attr_space=attr_space)
> +            msg = NlMsg(data, offset)
>              offset += msg.nl_len
>              self.msgs.append(msg)
>  
> @@ -1036,12 +1041,13 @@ genl_family_name_to_id = None
>          op_rsp = []
>          while not done:
>              reply = self.sock.recv(self._recv_size)
> -            nms = NlMsgs(reply, attr_space=op.attr_set)
> +            nms = NlMsgs(reply)
>              self._recv_dbg_print(reply, nms)
>              for nl_msg in nms:
>                  if nl_msg.nl_seq in reqs_by_seq:
>                      (op, vals, req_msg, req_flags) = reqs_by_seq[nl_msg.nl_seq]
>                      if nl_msg.extack:
> +                        nl_msg.annotate_extack(op.attr_set)
>                          self._decode_extack(req_msg, op, nl_msg.extack, vals)
>                  else:
>                      op = None

Re: [PATCH net] tools: ynl: fix mixing ops and notifications on one socket

[Jakub Kicinski]

On Thu, 19 Jun 2025 09:25:55 +0100 Donald Hunter wrote:
> Good catch, fix LGTM. It looks like a followup refactor could combine
> annotate_extack and _decode_extack and get rid of the attr_space
> parameter to NlMsg(). WDYT?

Indeed, sounds like a step in the right direction!
extack is a tricky thing to fit into an object model :(

Re: [PATCH net] tools: ynl: fix mixing ops and notifications on one socket

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 18 Jun 2025 10:17:46 -0700 you wrote:
> The multi message support loosened the connection between the request
> and response handling, as we can now submit multiple requests before
> we start processing responses. Passing the attr set to NlMsgs decoding
> no longer makes sense (if it ever did), attr set may differ message
> by messsage. Isolate the part of decoding responsible for attr-set
> specific interpretation and call it once we identified the correct op.
> 
> [...]

Here is the summary with links:
  - [net] tools: ynl: fix mixing ops and notifications on one socket
    https://git.kernel.org/netdev/net/c/9738280aae59

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html