| Message ID | 20230815194254.89570-6-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tools/net/ynl: Add support for netlink-raw families | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Tue, 15 Aug 2023 20:42:49 +0100 Donald Hunter wrote: > + def __init__(self, nl_msg, ynl=None): > + self.genl_cmd, self.genl_version, _ = struct.unpack_from("BBH", nl_msg.raw, 0) > + nl_msg.raw = nl_msg.raw[4:] It's a bit of a layering violation that we are futzing with the raw member of NlMsg inside GenlMsg, no? Should we add "fixed hdrs len" argument to NlMsg? Either directly or pass ynl and let get the expected len from ynl? That way NlMsg can split itself into hdr, userhdrs and attrs without GenlMsg "fixing it up"? > - self.hdr = nl_msg.raw[0:4] > - offset = 4 > - > - self.genl_cmd, self.genl_version, _ = struct.unpack("BBH", self.hdr) > - > - self.fixed_header_attrs = dict() > - for m in fixed_header_members: > - format = NlAttr.get_format(m.type, m.byte_order) > - decoded = format.unpack_from(nl_msg.raw, offset) > - offset += format.size > - self.fixed_header_attrs[m.name] = decoded[0] > + if ynl: > + op = ynl.rsp_by_value[self.genl_cmd] > + if op.fixed_header: > + nl_msg.decode_fixed_header(ynl, op.fixed_header) > > - self.raw = nl_msg.raw[offset:] > + self.raw = nl_msg.raw
On Wed, 16 Aug 2023 at 16:20, Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 15 Aug 2023 20:42:49 +0100 Donald Hunter wrote: > > + def __init__(self, nl_msg, ynl=None): > > + self.genl_cmd, self.genl_version, _ = struct.unpack_from("BBH", nl_msg.raw, 0) > > + nl_msg.raw = nl_msg.raw[4:] > > It's a bit of a layering violation that we are futzing with the raw > member of NlMsg inside GenlMsg, no? > > Should we add "fixed hdrs len" argument to NlMsg? Either directly or > pass ynl and let get the expected len from ynl? That way NlMsg can > split itself into hdr, userhdrs and attrs without GenlMsg "fixing it > up"? I agree, it breaks the layering. The issue is that GenlMsg gets created at some point after NlMsg, only when we know the nl_msg is suitable for decoding. The fixed header bit is quite well encapsulated in NlMsg, it's the genl header that needs pulled out and NlMsg shouldn't know anything about it. How about I add a take_bytes(length) method or a generic decode_subheader(format, length) method to NlMsg?
On Thu, 17 Aug 2023 16:14:35 +0100 Donald Hunter wrote: > > It's a bit of a layering violation that we are futzing with the raw > > member of NlMsg inside GenlMsg, no? > > > > Should we add "fixed hdrs len" argument to NlMsg? Either directly or > > pass ynl and let get the expected len from ynl? That way NlMsg can > > split itself into hdr, userhdrs and attrs without GenlMsg "fixing it > > up"? > > I agree, it breaks the layering. The issue is that GenlMsg gets created at > some point after NlMsg, only when we know the nl_msg is suitable for > decoding. The fixed header bit is quite well encapsulated in NlMsg, > it's the genl header that needs pulled out and NlMsg shouldn't know > anything about it. How about I add a take_bytes(length) method or a > generic decode_subheader(format, length) method to NlMsg? Why do we need to fix up the .raw of NlMsg underlying the GenlMsg in the first place? GenlMsg by itself didn't need to do that until now. Another option to consider which would make things more symmetric between raw and genetlink would be to add a wrapper class for old families, too. ClassicMsg? CnlMsg? That way we could retain the separation of NlMsg is just a raw message which could be a NLM_DONE or some other control thing, and higher level class being used to pull fixed headers and separate out attrs. Just a thought, not sure it helps.
On Fri, 18 Aug 2023 at 02:37, Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 17 Aug 2023 16:14:35 +0100 Donald Hunter wrote: > > > It's a bit of a layering violation that we are futzing with the raw > > > member of NlMsg inside GenlMsg, no? > > > > > > Should we add "fixed hdrs len" argument to NlMsg? Either directly or > > > pass ynl and let get the expected len from ynl? That way NlMsg can > > > split itself into hdr, userhdrs and attrs without GenlMsg "fixing it > > > up"? > > > > I agree, it breaks the layering. The issue is that GenlMsg gets created at > > some point after NlMsg, only when we know the nl_msg is suitable for > > decoding. The fixed header bit is quite well encapsulated in NlMsg, > > it's the genl header that needs pulled out and NlMsg shouldn't know > > anything about it. How about I add a take_bytes(length) method or a > > generic decode_subheader(format, length) method to NlMsg? > > Why do we need to fix up the .raw of NlMsg underlying the GenlMsg > in the first place? GenlMsg by itself didn't need to do that until now. Fair point. I will refactor to leave nl_msg.raw untouched. > Another option to consider which would make things more symmetric > between raw and genetlink would be to add a wrapper class for old > families, too. ClassicMsg? CnlMsg? That way we could retain the > separation of NlMsg is just a raw message which could be a NLM_DONE or > some other control thing, and higher level class being used to pull > fixed headers and separate out attrs. Just a thought, not sure it helps. I _think_ I can avoid doing this. There's an asymmetry to the way the NlAttrs get created that I need to fix. When I do that, the rest should be a bit cleaner.
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 3ca28d4bcb18..4fa42a7c5955 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -189,6 +189,7 @@ class NlMsg: self.error = 0 self.done = 0 + self.fixed_header_attrs = [] extack_off = None if self.nl_type == Netlink.NLMSG_ERROR: @@ -228,6 +229,19 @@ class NlMsg: desc += f" ({spec['doc']})" self.extack['miss-type'] = desc + def decode_fixed_header(self, ynl, name): + fixed_header_members = ynl.consts[name].members + self.fixed_header_attrs = dict() + offset = 0 + for m in fixed_header_members: + format = NlAttr.get_format(m.type, m.byte_order) + [ value ] = format.unpack_from(self.raw, offset) + offset += format.size + if m.enum: + value = ynl._decode_enum(value, m) + self.fixed_header_attrs[m.name] = value + self.raw = self.raw[offset:] + def __repr__(self): msg = f"nl_len = {self.nl_len} ({len(self.raw)}) nl_flags = 0x{self.nl_flags:x} nl_type = {self.nl_type}\n" if self.error: @@ -317,23 +331,18 @@ def _genl_load_families(): class GenlMsg: - def __init__(self, nl_msg, fixed_header_members=[]): - self.nl = nl_msg + def __init__(self, nl_msg, ynl=None): + self.genl_cmd, self.genl_version, _ = struct.unpack_from("BBH", nl_msg.raw, 0) + nl_msg.raw = nl_msg.raw[4:] - self.hdr = nl_msg.raw[0:4] - offset = 4 - - self.genl_cmd, self.genl_version, _ = struct.unpack("BBH", self.hdr) - - self.fixed_header_attrs = dict() - for m in fixed_header_members: - format = NlAttr.get_format(m.type, m.byte_order) - decoded = format.unpack_from(nl_msg.raw, offset) - offset += format.size - self.fixed_header_attrs[m.name] = decoded[0] + if ynl: + op = ynl.rsp_by_value[self.genl_cmd] + if op.fixed_header: + nl_msg.decode_fixed_header(ynl, op.fixed_header) - self.raw = nl_msg.raw[offset:] + self.raw = nl_msg.raw self.raw_attrs = NlAttrs(self.raw) + self.fixed_header_attrs = nl_msg.fixed_header_attrs def __repr__(self): msg = repr(self.nl) @@ -596,7 +605,7 @@ class YnlFamily(SpecFamily): done = True break - gm = GenlMsg(nl_msg, fixed_header_members) + gm = GenlMsg(nl_msg, self) # Check if this is a reply to our request if nl_msg.nl_seq != req_seq or gm.genl_cmd != op.rsp_value: if gm.genl_cmd in self.async_msg_ids:
Move decode_fixed_header into NlMsg in preparation for adding netlink-raw support. Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- tools/net/ynl/lib/ynl.py | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)