| Message ID | 20240830201321.292593-1-arkadiusz.kubalewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [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: > 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>
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?
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?
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.
>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
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:
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(-)