[PATCH net] tools/net/ynl: fix cli.py --subscribe feature

[PATCH net] tools/net/ynl: fix cli.py --subscribe feature

[Arkadiusz Kubalewski]

Execution of command:
./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
	--subscribe "monitor" --sleep 10
fails with:
  File "/repo/./tools/net/ynl/cli.py", line 109, in main
    ynl.check_ntf()
  File "/repo/tools/net/ynl/lib/ynl.py", line 924, in check_ntf
    op = self.rsp_by_value[nl_msg.cmd()]
KeyError: 19

Parsing Generic Netlink notification messages performs lookup for op in
the message. The message was not yet decoded, and is not yet considered
GenlMsg, thus msg.cmd() returns Generic Netlink family id (19) instead of
proper notification command id (i.e.: DPLL_CMD_PIN_CHANGE_NTF=13).

Allow the op to be obtained within NetlinkProtocol.decode(..) itself if the
op was not passed to the decode function, thus allow parsing of Generic
Netlink notifications without causing the failure.

Suggested-by: Donald Hunter <donald.hunter@gmail.com>
Link: https://lore.kernel.org/netdev/m2le0n5xpn.fsf@gmail.com/
Fixes: 0a966d606c68 ("tools/net/ynl: Fix extack decoding for directional ops")
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 tools/net/ynl/lib/ynl.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index d42c1d605969..4e7993c8cd9a 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -386,8 +386,10 @@ class NetlinkProtocol:
     def _decode(self, nl_msg):
         return nl_msg
 
-    def decode(self, ynl, nl_msg, op):
+    def decode(self, ynl, nl_msg, op, ops_by_value):
         msg = self._decode(nl_msg)
+        if op is None:
+            op = ops_by_value[msg.cmd()]
         fixed_header_size = ynl._struct_size(op.fixed_header)
         msg.raw_attrs = NlAttrs(msg.raw, fixed_header_size)
         return msg
@@ -796,7 +798,7 @@ class YnlFamily(SpecFamily):
         if 'bad-attr-offs' not in extack:
             return
 
-        msg = self.nlproto.decode(self, NlMsg(request, 0, op.attr_set), op)
+        msg = self.nlproto.decode(self, NlMsg(request, 0, op.attr_set), op, self.rsp_by_value)
         offset = self.nlproto.msghdr_size() + self._struct_size(op.fixed_header)
         path = self._decode_extack_path(msg.raw_attrs, op.attr_set, offset,
                                         extack['bad-attr-offs'])
@@ -921,8 +923,7 @@ class YnlFamily(SpecFamily):
                     print("Netlink done while checking for ntf!?")
                     continue
 
-                op = self.rsp_by_value[nl_msg.cmd()]
-                decoded = self.nlproto.decode(self, nl_msg, op)
+                decoded = self.nlproto.decode(self, nl_msg, None, self.rsp_by_value)
                 if decoded.cmd() not in self.async_msg_ids:
                     print("Unexpected msg id done while checking for ntf", decoded)
                     continue
@@ -980,7 +981,7 @@ class YnlFamily(SpecFamily):
                     if nl_msg.extack:
                         self._decode_extack(req_msg, op, nl_msg.extack)
                 else:
-                    op = self.rsp_by_value[nl_msg.cmd()]
+                    op = None
                     req_flags = []
 
                 if nl_msg.error:
@@ -1004,7 +1005,7 @@ class YnlFamily(SpecFamily):
                     done = len(reqs_by_seq) == 0
                     break
 
-                decoded = self.nlproto.decode(self, nl_msg, op)
+                decoded = self.nlproto.decode(self, nl_msg, op, self.rsp_by_value)
 
                 # Check if this is a reply to our request
                 if nl_msg.nl_seq not in reqs_by_seq or decoded.cmd() != op.rsp_value:
-- 
2.38.1

Re: [PATCH net] tools/net/ynl: fix cli.py --subscribe feature

[Donald Hunter]

Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> writes:

> Execution of command:
> ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
> 	--subscribe "monitor" --sleep 10
> fails with:
>   File "/repo/./tools/net/ynl/cli.py", line 109, in main
>     ynl.check_ntf()
>   File "/repo/tools/net/ynl/lib/ynl.py", line 924, in check_ntf
>     op = self.rsp_by_value[nl_msg.cmd()]
> KeyError: 19
>
> Parsing Generic Netlink notification messages performs lookup for op in
> the message. The message was not yet decoded, and is not yet considered
> GenlMsg, thus msg.cmd() returns Generic Netlink family id (19) instead of
> proper notification command id (i.e.: DPLL_CMD_PIN_CHANGE_NTF=13).
>
> Allow the op to be obtained within NetlinkProtocol.decode(..) itself if the
> op was not passed to the decode function, thus allow parsing of Generic
> Netlink notifications without causing the failure.
>
> Suggested-by: Donald Hunter <donald.hunter@gmail.com>
> Link: https://lore.kernel.org/netdev/m2le0n5xpn.fsf@gmail.com/
> Fixes: 0a966d606c68 ("tools/net/ynl: Fix extack decoding for directional ops")
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

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

Re: [PATCH net] tools/net/ynl: fix cli.py --subscribe feature

[Jakub Kicinski]

On Mon, 02 Sep 2024 10:51:13 +0100 Donald Hunter wrote:
> Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Any preference on passing self.rsp_by_value, vs .decode() accessing
ynl.rsp_by_value on its own? 

Re: [PATCH net] tools/net/ynl: fix cli.py --subscribe feature

[Donald Hunter]

On Tue, 3 Sept 2024 at 21:01, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 02 Sep 2024 10:51:13 +0100 Donald Hunter wrote:
> > Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
>
> Any preference on passing self.rsp_by_value, vs .decode() accessing
> ynl.rsp_by_value on its own?

.decode() accessing ynl.rsp_by_value would be cleaner, but I am
working on some notification fixes that might benefit from the map
being passed as a parameter. The netlink-raw families use a msg id
scheme that is neither unified nor directional. It's more like a mix
of both where req and rsp use different values but notifications reuse
the req values. I suspect that to fix that we'd need to introduce a
dict for ntf_by_value and then the parameter would be context
specific. OVS reuses req/rsp values for notifications as well, but it
uses a unified scheme, and that's mostly a problem for ynl-gen-c. We
could choose the cleaner approach just now and revisit it as part of
fixing notifications for netlink-raw?

Re: [PATCH net] tools/net/ynl: fix cli.py --subscribe feature

[Jakub Kicinski]

On Tue, 3 Sep 2024 22:08:54 +0100 Donald Hunter wrote:
> > On Mon, 02 Sep 2024 10:51:13 +0100 Donald Hunter wrote:  
> > > Reviewed-by: Donald Hunter <donald.hunter@gmail.com>  
> >
> > Any preference on passing self.rsp_by_value, vs .decode() accessing
> > ynl.rsp_by_value on its own?  
> 
> .decode() accessing ynl.rsp_by_value would be cleaner, but I am
> working on some notification fixes that might benefit from the map
> being passed as a parameter. The netlink-raw families use a msg id
> scheme that is neither unified nor directional. It's more like a mix
> of both where req and rsp use different values but notifications reuse
> the req values. I suspect that to fix that we'd need to introduce a
> dict for ntf_by_value and then the parameter would be context
> specific. OVS reuses req/rsp values for notifications as well, but it
> uses a unified scheme, and that's mostly a problem for ynl-gen-c. 

I was worried you'd say it's ID reuse related. That is tricky business.

> We could choose the cleaner approach just now and revisit it as part of
> fixing notifications for netlink-raw?

That's my intuition; there's a non-zero chance that priorities will
change or we'll head in a different direction, and the extra arg will
stick around confusing readers.

RE: [PATCH net] tools/net/ynl: fix cli.py --subscribe feature

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, September 4, 2024 12:00 AM
>
>On Tue, 3 Sep 2024 22:08:54 +0100 Donald Hunter wrote:
>> > On Mon, 02 Sep 2024 10:51:13 +0100 Donald Hunter wrote:
>> > > Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
>> >
>> > Any preference on passing self.rsp_by_value, vs .decode() accessing
>> > ynl.rsp_by_value on its own?
>>
>> .decode() accessing ynl.rsp_by_value would be cleaner, but I am
>> working on some notification fixes that might benefit from the map
>> being passed as a parameter. The netlink-raw families use a msg id
>> scheme that is neither unified nor directional. It's more like a mix
>> of both where req and rsp use different values but notifications reuse
>> the req values. I suspect that to fix that we'd need to introduce a
>> dict for ntf_by_value and then the parameter would be context
>> specific. OVS reuses req/rsp values for notifications as well, but it
>> uses a unified scheme, and that's mostly a problem for ynl-gen-c.
>
>I was worried you'd say it's ID reuse related. That is tricky business.
>
>> We could choose the cleaner approach just now and revisit it as part of
>> fixing notifications for netlink-raw?
>
>That's my intuition; there's a non-zero chance that priorities will
>change or we'll head in a different direction, and the extra arg will
>stick around confusing readers.

Sure, fixed in v2.

Thank you!
Arkadiusz