| Message ID | 20240823084220.258965-1-arkadiusz.kubalewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] tools/net/ynl: fix cli.py --subscribe feature | expand |
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: > Traceback (most recent call last): > File "/root/arek/linux-dpll/./tools/net/ynl/cli.py", line 114, in <module> > main() > File "/root/arek/linux-dpll/./tools/net/ynl/cli.py", line 109, in main > ynl.check_ntf() > File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 924, in check_ntf > op = self.rsp_by_value[nl_msg.cmd()] > KeyError: 19 > > The key value of 19 returned from nl_msg.cmd() is a received message > header's nl_type, which is the id value of generic netlink family being > addressed in the OS on subscribing. It is wrong to use it for decoding > the notification. Expected notification message on dpll subsystem is > DPLL_CMD_PIN_CHANGE_NTF=13, seems at that point only available as first > byte of RAW message payload, use it to target correct op and allow further > parsing. > > 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py > index d42c1d605969..192d6c150303 100644 > --- a/tools/net/ynl/lib/ynl.py > +++ b/tools/net/ynl/lib/ynl.py > @@ -921,7 +921,7 @@ class YnlFamily(SpecFamily): > print("Netlink done while checking for ntf!?") > continue > > - op = self.rsp_by_value[nl_msg.cmd()] > + op = self.rsp_by_value[nl_msg.raw[0]] I don't think that is the right fix. It would break notifications for raw netlink messages. The point of NlMsg.cmd() is to abstract away where the op id comes from. GenlMsg.cmd() returns the value unpacked from raw[0]. The problem is that we are trying to look up the op before calling nlproto.decode(...) but it wants to know the op to check if it has a fixed header. I think the fix would be to change NetlinkProtocol.decode() to perform the op lookup, if necessary, after it has called self._decode() to unpack the GenlMsg. How about changing NetlinkProtocol.decode() to be: 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()] ... The main loop can call it like this: nlproto.decode(self, nl_msg, op, self.rsp_by_value) and check_ntf() can call it like this: nlproto.decode(self, nl_msg, None, self.rsp_by_value) > decoded = self.nlproto.decode(self, nl_msg, op) > if decoded.cmd() not in self.async_msg_ids: > print("Unexpected msg id done while checking for ntf", decoded)
>From: Donald Hunter <donald.hunter@gmail.com> >Sent: Friday, August 23, 2024 12:40 PM > >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: >> Traceback (most recent call last): >> File "/root/arek/linux-dpll/./tools/net/ynl/cli.py", line 114, in <module> >> main() >> File "/root/arek/linux-dpll/./tools/net/ynl/cli.py", line 109, in main >> ynl.check_ntf() >> File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 924, in >>check_ntf >> op = self.rsp_by_value[nl_msg.cmd()] >> KeyError: 19 >> >> The key value of 19 returned from nl_msg.cmd() is a received message >> header's nl_type, which is the id value of generic netlink family being >> addressed in the OS on subscribing. It is wrong to use it for decoding >> the notification. Expected notification message on dpll subsystem is >> DPLL_CMD_PIN_CHANGE_NTF=13, seems at that point only available as first >> byte of RAW message payload, use it to target correct op and allow further >> parsing. >> >> 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 | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py >> index d42c1d605969..192d6c150303 100644 >> --- a/tools/net/ynl/lib/ynl.py >> +++ b/tools/net/ynl/lib/ynl.py >> @@ -921,7 +921,7 @@ class YnlFamily(SpecFamily): >> print("Netlink done while checking for ntf!?") >> continue >> >> - op = self.rsp_by_value[nl_msg.cmd()] >> + op = self.rsp_by_value[nl_msg.raw[0]] > >I don't think that is the right fix. It would break notifications for >raw netlink messages. The point of NlMsg.cmd() is to abstract away where >the op id comes from. GenlMsg.cmd() returns the value unpacked from >raw[0]. > Well, me either, thus the RFC. Your suggestion makes much more sense than my workaround. >The problem is that we are trying to look up the op before calling >nlproto.decode(...) but it wants to know the op to check if it has a >fixed header. > >I think the fix would be to change NetlinkProtocol.decode() to perform >the op lookup, if necessary, after it has called self._decode() to >unpack the GenlMsg. > >How about changing NetlinkProtocol.decode() to be: > >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()] > ... > >The main loop can call it like this: > >nlproto.decode(self, nl_msg, op, self.rsp_by_value) > >and check_ntf() can call it like this: > >nlproto.decode(self, nl_msg, None, self.rsp_by_value) > Yes, again, this seems much better, I will prepare new patch and send the non-RFC version soon. Thanks for your help! Arkadiusz >> decoded = self.nlproto.decode(self, nl_msg, op) >> if decoded.cmd() not in self.async_msg_ids: >> print("Unexpected msg id done while checking for ntf", >>decoded)
On Fri, 23 Aug 2024 at 20:31, Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> wrote: > > >The problem is that we are trying to look up the op before calling > >nlproto.decode(...) but it wants to know the op to check if it has a > >fixed header. > > > >I think the fix would be to change NetlinkProtocol.decode() to perform > >the op lookup, if necessary, after it has called self._decode() to > >unpack the GenlMsg. > > > >How about changing NetlinkProtocol.decode() to be: > > > >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()] > > ... > > > >The main loop can call it like this: > > > >nlproto.decode(self, nl_msg, op, self.rsp_by_value) > > > >and check_ntf() can call it like this: > > > >nlproto.decode(self, nl_msg, None, self.rsp_by_value) > > > > Yes, again, this seems much better, I will prepare new patch and send > the non-RFC version soon. > > Thanks for your help! > Arkadiusz I tried the changes locally to check it would work and ended up with the following patch. Can you verify that it fixes the issue you had? diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index d42c1d605969..311542a8aa24 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 @@ -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:
>From: Donald Hunter <donald.hunter@gmail.com> >Sent: Friday, August 23, 2024 11:23 PM > >On Fri, 23 Aug 2024 at 20:31, Kubalewski, Arkadiusz ><arkadiusz.kubalewski@intel.com> wrote: >> >> >The problem is that we are trying to look up the op before calling >> >nlproto.decode(...) but it wants to know the op to check if it has a >> >fixed header. >> > >> >I think the fix would be to change NetlinkProtocol.decode() to perform >> >the op lookup, if necessary, after it has called self._decode() to >> >unpack the GenlMsg. >> > >> >How about changing NetlinkProtocol.decode() to be: >> > >> >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()] >> > ... >> > >> >The main loop can call it like this: >> > >> >nlproto.decode(self, nl_msg, op, self.rsp_by_value) >> > >> >and check_ntf() can call it like this: >> > >> >nlproto.decode(self, nl_msg, None, self.rsp_by_value) >> > >> >> Yes, again, this seems much better, I will prepare new patch and send >> the non-RFC version soon. >> >> Thanks for your help! >> Arkadiusz > >I tried the changes locally to check it would work and ended up with >the following patch. Can you verify that it fixes the issue you had? > >diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py >index d42c1d605969..311542a8aa24 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 >@@ -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: I can confirm the notification works with above changes, but there is third call to decode which would also need an update? It shows up on testing messages which return extack errors. Beware line numbers, these can differ on my ynl.py as had it modified slightly. File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 1040, in do return self._op(method, vals, flags) File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 1034, in _op ret = self._ops(ops) File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 982, in _ops self._decode_extack(req_msg, op, nl_msg.extack) File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 801, in _decode_extack msg = self.nlproto.decode(self, NlMsg(request, 0, op.attr_set), op) TypeError: decode() missing 1 required positional argument: 'ops_by_value' error: 1 Thank you! Arkadiusz
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index d42c1d605969..192d6c150303 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -921,7 +921,7 @@ class YnlFamily(SpecFamily): print("Netlink done while checking for ntf!?") continue - op = self.rsp_by_value[nl_msg.cmd()] + op = self.rsp_by_value[nl_msg.raw[0]] decoded = self.nlproto.decode(self, nl_msg, op) if decoded.cmd() not in self.async_msg_ids: print("Unexpected msg id done while checking for ntf", decoded)
Execution of command: ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml / --subscribe "monitor" --sleep 10 fails with: Traceback (most recent call last): File "/root/arek/linux-dpll/./tools/net/ynl/cli.py", line 114, in <module> main() File "/root/arek/linux-dpll/./tools/net/ynl/cli.py", line 109, in main ynl.check_ntf() File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 924, in check_ntf op = self.rsp_by_value[nl_msg.cmd()] KeyError: 19 The key value of 19 returned from nl_msg.cmd() is a received message header's nl_type, which is the id value of generic netlink family being addressed in the OS on subscribing. It is wrong to use it for decoding the notification. Expected notification message on dpll subsystem is DPLL_CMD_PIN_CHANGE_NTF=13, seems at that point only available as first byte of RAW message payload, use it to target correct op and allow further parsing. 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)