Re: [PATCH net-next v1 2/3] tools/net/ynl: Add support for netlink-raw families
From: Jakub Kicinski <kuba@kernel.org>
Date: 2023-07-26 21:37:10
On Tue, 25 Jul 2023 17:22:04 +0100 Donald Hunter wrote:
Refactor the ynl code to encapsulate protocol-family specifics into NetlinkProtocolFamily and GenlProtocolFamily.
+class SpecMcastGroup(SpecElement):
+ """Netlink Multicast Group
+
+ Information about a multicast group.
+
+ Attributes:
+ id numerical id of this multicast group for netlink-raw
+ yaml raw spec as loaded from the spec file
+ """
+ def __init__(self, family, yaml):
+ super().__init__(family, yaml)
+ self.id = self.yaml.get('id')name, too?
quoted hunk ↗ jump to hunk
class SpecFamily(SpecElement): """ Netlink Family Spec class.@@ -343,6 +357,7 @@ class SpecFamily(SpecElement): ntfs dict of all async events consts dict of all constants/enums fixed_header string, optional name of family default fixed header struct + mcast_groups dict of all multicast groups (index by name) """ def __init__(self, spec_path, schema_path=None, exclude_ops=None): with open(spec_path, "r") as stream:@@ -384,6 +399,7 @@ class SpecFamily(SpecElement): self.ops = collections.OrderedDict() self.ntfs = collections.OrderedDict() self.consts = collections.OrderedDict() + self.mcast_groups = collections.OrderedDict() last_exception = None while len(self._resolution_list) > 0:@@ -416,6 +432,9 @@ class SpecFamily(SpecElement): def new_operation(self, elem, req_val, rsp_val): return SpecOperation(self, elem, req_val, rsp_val) + def new_mcast_group(self, elem): + return SpecMcastGroup(self, elem) + def add_unresolved(self, elem): self._resolution_list.append(elem)@@ -512,3 +531,9 @@ class SpecFamily(SpecElement): self.ops[op.name] = op elif op.is_async: self.ntfs[op.name] = op + + mcgs = self.yaml.get('mcast-groups') + if mcgs: + for elem in mcgs['list']: + mcg = self.new_mcast_group(elem) + self.mcast_groups[elem['name']] = mcg
Could you factor out the mcgroup changes to a separate patch?
quoted hunk ↗ jump to hunk
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 3e2ade2194cd..7e877c43e10f 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py@@ -25,6 +25,7 @@ class Netlink: NETLINK_ADD_MEMBERSHIP = 1 NETLINK_CAP_ACK = 10 NETLINK_EXT_ACK = 11 + NETLINK_GET_STRICT_CHK = 12 # Netlink message NLMSG_ERROR = 2@@ -153,6 +154,21 @@ class NlAttr: value[m.name] = decoded return value + @classmethod + def decode_enum(cls, raw, attr_spec, consts): + enum = consts[attr_spec['enum']] + if 'enum-as-flags' in attr_spec and attr_spec['enum-as-flags']: + i = 0 + value = set() + while raw: + if raw & 1: + value.add(enum.entries_by_val[i].name) + raw >>= 1 + i += 1 + else: + value = enum.entries_by_val[raw].name + return value
This doesn't always operates on netlink attributes, technically, so how about we make it a standalone function, not a member of NlAttr? Or should we move it to a parent class, NetlinkProtocolFamily?
+ def decode_fixed_header(self, consts, op): + fixed_header_members = consts[op.fixed_header].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 = NlAttr.decode_enum(value, m, consts) + + self.fixed_header_attrs[m.name] = value + self.raw = self.raw[offset:] + + def cmd(self): + return self.nl_type
And perhaps the pure code moves could be a separate patch for ease of review?
quoted hunk ↗ jump to hunk
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:@@ -318,23 +353,21 @@ 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 + if ynl: + op = ynl.rsp_by_value[self.genl_cmd]
Took me a while to figure out why ynl gets passed here :S I'm not sure what the best structure of inheritance is but I think we should at the very least *not* call genl vs raw-nl "family". NetlinkProtocolFamily -> NetlinkProtocol GenlProtocolFamily -> GenlProtocol and store them in YnlFamily to self.nlproto or self.protocol or some such.
+ if op.fixed_header: + nl_msg.decode_fixed_header(ynl.consts, op)
def _decode_binary(self, attr, attr_spec):
if attr_spec.struct_name:
members = self.consts[attr_spec.struct_name]
- decoded = attr.as_struct(members)
+ decoded = attr.as_struct(members, self.consts)I applied the series on top of Arkadiusz's fixes and this line throws an "as_struct takes 2 arguments, 3 given" exception.
for m in members:
if m.enum:
decoded[m.name] = self._decode_enum(decoded[m.name], m)