On Wed, 26 Jul 2023 at 22:37, Jakub Kicinski [off-list ref] wrote:

On Tue, 25 Jul 2023 17:22:04 +0100 Donald Hunter wrote:

quoted

Refactor the ynl code to encapsulate protocol-family specifics into
NetlinkProtocolFamily and GenlProtocolFamily.

quoted

+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?

Ack.

quoted

+        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?

Will do.

quoted

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?

Fair point. I'll maybe go for standalone just now but will think on
it some more first.

quoted

+    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?

Ack.

quoted

     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

Yeah, agreed. "Family" is way too overloaded already :-)

and store them in YnlFamily to self.nlproto or self.protocol
or some such.

Ack. Just a note that I have been wondering about refactoring this
from "YnlFamily is a Spec" to "Ynl has n Specs" so that we could do
multi spec notification handling. If we did this, then passing a
SpecContext around would look more natural maybe.

quoted

+            if op.fixed_header:
+                nl_msg.decode_fixed_header(ynl.consts, op)

quoted

     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.

Ah, my bad. Looks like I missed a fix for that from the patchset.

quoted

             for m in members:
                 if m.enum:
                      decoded[m.name] = self._decode_enum(decoded[m.name], m)