[PATCH net-next v1 0/3] tools/net/ynl: Add support for netlink-raw families

[PATCH net-next v1 0/3] tools/net/ynl: Add support for netlink-raw families

[Donald Hunter]

This patchset adds support for netlink-raw families such as rtnetlink.

The first patch contains the schema definition.
The second patch extends ynl to support netlink-raw
The third patch adds rtnetlink addr and route message types

The second patch depends on "tools: ynl-gen: fix parse multi-attr enum
attribute":

https://patchwork.kernel.org/project/netdevbpf/list/?series=769229

The netlink-raw schema is very similar to genetlink-legacy and I thought
about making the changes there and symlinking to it. On balance I
thought that might be problematic for accurate schema validation.

rtnetlink doesn't seem to fit into unified or directional message
enumeration models. It seems like an 'explicit' model would be useful,
to require the schema author to specify the message ids directly. The
patch supports commands and it supports notifications, but it's
currently hard to support both simultaneously from the same netlink-raw
spec. I plan to work on this in a future patchset.

There is not yet support for notifications because ynl currently doesn't
support defining 'event' properties on a 'do' operation. I plan to work
on this in a future patch.

The link message types are a work in progress that I plan to submit in a
future patchset. Links contain different nested attributes dependent on
the type of link. Decoding these will need some kind of attr-space
selection based on the value of another attribute in the message.

Donald Hunter (3):
  doc/netlink: Add a schema for netlink-raw families
  tools/net/ynl: Add support for netlink-raw families
  doc/netlink: Add specs for addr and route rtnetlink message types

 Documentation/netlink/netlink-raw.yaml    | 414 ++++++++++++++++++++++
 Documentation/netlink/specs/rt_addr.yaml  | 179 ++++++++++
 Documentation/netlink/specs/rt_route.yaml | 192 ++++++++++
 tools/net/ynl/lib/nlspec.py               |  25 ++
 tools/net/ynl/lib/ynl.py                  | 185 +++++++---
 5 files changed, 941 insertions(+), 54 deletions(-)
 create mode 100644 Documentation/netlink/netlink-raw.yaml
 create mode 100644 Documentation/netlink/specs/rt_addr.yaml
 create mode 100644 Documentation/netlink/specs/rt_route.yaml

-- 
2.41.0

[PATCH net-next v1 1/3] doc/netlink: Add a schema for netlink-raw families

[Donald Hunter]

This schema is largely a copy of the genetlink-legacy schema with the
following additions:

 - a top-level protonum property, e.g. 0 (for NETLINK_ROUTE)
 - add netlink-raw to the list of protocols supported by the schema
 - add an id property to mcast-group definitions

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/netlink-raw.yaml | 414 +++++++++++++++++++++++++
 1 file changed, 414 insertions(+)
 create mode 100644 Documentation/netlink/netlink-raw.yaml

diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml
new file mode 100644
index 000000000000..34af9c7f94a8
--- /dev/null
+++ b/Documentation/netlink/netlink-raw.yaml
@@ -0,0 +1,414 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+%YAML 1.2
+---
+$id: http://kernel.org/schemas/netlink/genetlink-legacy.yaml#
+$schema: https://json-schema.org/draft-07/schema
+
+# Common defines
+$defs:
+  uint:
+    type: integer
+    minimum: 0
+  len-or-define:
+    type: [ string, integer ]
+    pattern: ^[0-9A-Za-z_]+( - 1)?$
+    minimum: 0
+
+# Schema for specs
+title: Protocol
+description: Specification of a genetlink protocol
+type: object
+required: [ name, doc, attribute-sets, operations ]
+additionalProperties: False
+properties:
+  name:
+    description: Name of the genetlink family.
+    type: string
+  doc:
+    type: string
+  version:
+    description: Generic Netlink family version. Default is 1.
+    type: integer
+    minimum: 1
+  protocol:
+    description: Schema compatibility level. Default is "genetlink".
+    enum: [ genetlink, genetlink-c, genetlink-legacy, netlink-raw ] # Trim
+  # Start netlink-raw
+  protonum:
+    description: Protocol number to use for netlink-raw
+    type: integer
+  # End netlink-raw
+  uapi-header:
+    description: Path to the uAPI header, default is linux/${family-name}.h
+    type: string
+  # Start genetlink-c
+  c-family-name:
+    description: Name of the define for the family name.
+    type: string
+  c-version-name:
+    description: Name of the define for the verion of the family.
+    type: string
+  max-by-define:
+    description: Makes the number of attributes and commands be specified by a define, not an enum value.
+    type: boolean
+  # End genetlink-c
+  # Start genetlink-legacy
+  kernel-policy:
+    description: |
+      Defines if the input policy in the kernel is global, per-operation, or split per operation type.
+      Default is split.
+    enum: [ split, per-op, global ]
+  # End genetlink-legacy
+
+  definitions:
+    description: List of type and constant definitions (enums, flags, defines).
+    type: array
+    items:
+      type: object
+      required: [ type, name ]
+      additionalProperties: False
+      properties:
+        name:
+          type: string
+        header:
+          description: For C-compatible languages, header which already defines this value.
+          type: string
+        type:
+          enum: [ const, enum, flags, struct ] # Trim
+        doc:
+          type: string
+        # For const
+        value:
+          description: For const - the value.
+          type: [ string, integer ]
+        # For enum and flags
+        value-start:
+          description: For enum or flags the literal initializer for the first value.
+          type: [ string, integer ]
+        entries:
+          description: For enum or flags array of values.
+          type: array
+          items:
+            oneOf:
+              - type: string
+              - type: object
+                required: [ name ]
+                additionalProperties: False
+                properties:
+                  name:
+                    type: string
+                  value:
+                    type: integer
+                  doc:
+                    type: string
+        render-max:
+          description: Render the max members for this enum.
+          type: boolean
+        # Start genetlink-c
+        enum-name:
+          description: Name for enum, if empty no name will be used.
+          type: [ string, "null" ]
+        name-prefix:
+          description: For enum the prefix of the values, optional.
+          type: string
+        # End genetlink-c
+        # Start genetlink-legacy
+        members:
+          description: List of struct members. Only scalars and strings members allowed.
+          type: array
+          items:
+            type: object
+            required: [ name, type ]
+            additionalProperties: False
+            properties:
+              name:
+                type: string
+              type:
+                description: The netlink attribute type
+                enum: [ u8, u16, u32, u64, s8, s16, s32, s64, string, binary ]
+              len:
+                $ref: '#/$defs/len-or-define'
+              byte-order:
+                enum: [ little-endian, big-endian ]
+              doc:
+                description: Documentation for the struct member attribute.
+                type: string
+              enum:
+                description: Name of the enum type used for the attribute.
+                type: string
+              enum-as-flags:
+                description: |
+                  Treat the enum as flags. In most cases enum is either used as flags or as values.
+                  Sometimes, however, both forms are necessary, in which case header contains the enum
+                  form while specific attributes may request to convert the values into a bitfield.
+                type: boolean
+              display-hint: &display-hint
+                description: |
+                  Optional format indicator that is intended only for choosing
+                  the right formatting mechanism when displaying values of this
+                  type.
+                enum: [ hex, mac, fddi, ipv4, ipv6, uuid ]
+        # End genetlink-legacy
+
+  attribute-sets:
+    description: Definition of attribute spaces for this family.
+    type: array
+    items:
+      description: Definition of a single attribute space.
+      type: object
+      required: [ name, attributes ]
+      additionalProperties: False
+      properties:
+        name:
+          description: |
+            Name used when referring to this space in other definitions, not used outside of the spec.
+          type: string
+        name-prefix:
+          description: |
+            Prefix for the C enum name of the attributes. Default family[name]-set[name]-a-
+          type: string
+        enum-name:
+          description: Name for the enum type of the attribute.
+          type: string
+        doc:
+          description: Documentation of the space.
+          type: string
+        subset-of:
+          description: |
+            Name of another space which this is a logical part of. Sub-spaces can be used to define
+            a limited group of attributes which are used in a nest.
+          type: string
+        # Start genetlink-c
+        attr-cnt-name:
+          description: The explicit name for constant holding the count of attributes (last attr + 1).
+          type: string
+        attr-max-name:
+          description: The explicit name for last member of attribute enum.
+          type: string
+        # End genetlink-c
+        attributes:
+          description: List of attributes in the space.
+          type: array
+          items:
+            type: object
+            required: [ name, type ]
+            additionalProperties: False
+            properties:
+              name:
+                type: string
+              type: &attr-type
+                description: The netlink attribute type
+                enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64,
+                        string, nest, array-nest, nest-type-value ]
+              doc:
+                description: Documentation of the attribute.
+                type: string
+              value:
+                description: Value for the enum item representing this attribute in the uAPI.
+                $ref: '#/$defs/uint'
+              type-value:
+                description: Name of the value extracted from the type of a nest-type-value attribute.
+                type: array
+                items:
+                  type: string
+              byte-order:
+                enum: [ little-endian, big-endian ]
+              multi-attr:
+                type: boolean
+              nested-attributes:
+                description: Name of the space (sub-space) used inside the attribute.
+                type: string
+              enum:
+                description: Name of the enum type used for the attribute.
+                type: string
+              enum-as-flags:
+                description: |
+                  Treat the enum as flags. In most cases enum is either used as flags or as values.
+                  Sometimes, however, both forms are necessary, in which case header contains the enum
+                  form while specific attributes may request to convert the values into a bitfield.
+                type: boolean
+              checks:
+                description: Kernel input validation.
+                type: object
+                additionalProperties: False
+                properties:
+                  flags-mask:
+                    description: Name of the flags constant on which to base mask (unsigned scalar types only).
+                    type: string
+                  min:
+                    description: Min value for an integer attribute.
+                    type: integer
+                  min-len:
+                    description: Min length for a binary attribute.
+                    $ref: '#/$defs/len-or-define'
+                  max-len:
+                    description: Max length for a string or a binary attribute.
+                    $ref: '#/$defs/len-or-define'
+              sub-type: *attr-type
+              display-hint: *display-hint
+              # Start genetlink-c
+              name-prefix:
+                type: string
+              # End genetlink-c
+              # Start genetlink-legacy
+              struct:
+                description: Name of the struct type used for the attribute.
+                type: string
+              # End genetlink-legacy
+
+      # Make sure name-prefix does not appear in subsets (subsets inherit naming)
+      dependencies:
+        name-prefix:
+          not:
+            required: [ subset-of ]
+        subset-of:
+          not:
+            required: [ name-prefix ]
+
+  operations:
+    description: Operations supported by the protocol.
+    type: object
+    required: [ list ]
+    additionalProperties: False
+    properties:
+      enum-model:
+        description: |
+          The model of assigning values to the operations.
+          "unified" is the recommended model where all message types belong
+          to a single enum.
+          "directional" has the messages sent to the kernel and from the kernel
+          enumerated separately.
+        enum: [ unified, directional ] # Trim
+      name-prefix:
+        description: |
+          Prefix for the C enum name of the command. The name is formed by concatenating
+          the prefix with the upper case name of the command, with dashes replaced by underscores.
+        type: string
+      enum-name:
+        description: Name for the enum type with commands.
+        type: string
+      async-prefix:
+        description: Same as name-prefix but used to render notifications and events to separate enum.
+        type: string
+      async-enum:
+        description: Name for the enum type with notifications/events.
+        type: string
+      # Start genetlink-legacy
+      fixed-header: &fixed-header
+        description: |
+          Name of the structure defining the optional fixed-length protocol
+          header. This header is placed in a message after the netlink and
+          genetlink headers and before any attributes.
+        type: string
+      # End genetlink-legacy
+      list:
+        description: List of commands
+        type: array
+        items:
+          type: object
+          additionalProperties: False
+          required: [ name, doc ]
+          properties:
+            name:
+              description: Name of the operation, also defining its C enum value in uAPI.
+              type: string
+            doc:
+              description: Documentation for the command.
+              type: string
+            value:
+              description: Value for the enum in the uAPI.
+              $ref: '#/$defs/uint'
+            attribute-set:
+              description: |
+                Attribute space from which attributes directly in the requests and replies
+                to this command are defined.
+              type: string
+            flags: &cmd_flags
+              description: Command flags.
+              type: array
+              items:
+                enum: [ admin-perm ]
+            dont-validate:
+              description: Kernel attribute validation flags.
+              type: array
+              items:
+                enum: [ strict, dump ]
+            # Start genetlink-legacy
+            fixed-header: *fixed-header
+            # End genetlink-legacy
+            do: &subop-type
+              description: Main command handler.
+              type: object
+              additionalProperties: False
+              properties:
+                request: &subop-attr-list
+                  description: Definition of the request message for a given command.
+                  type: object
+                  additionalProperties: False
+                  properties:
+                    attributes:
+                      description: |
+                        Names of attributes from the attribute-set (not full attribute
+                        definitions, just names).
+                      type: array
+                      items:
+                        type: string
+                    # Start genetlink-legacy
+                    value:
+                      description: |
+                        ID of this message if value for request and response differ,
+                        i.e. requests and responses have different message enums.
+                      $ref: '#/$defs/uint'
+                    # End genetlink-legacy
+                reply: *subop-attr-list
+                pre:
+                  description: Hook for a function to run before the main callback (pre_doit or start).
+                  type: string
+                post:
+                  description: Hook for a function to run after the main callback (post_doit or done).
+                  type: string
+            dump: *subop-type
+            notify:
+              description: Name of the command sharing the reply type with this notification.
+              type: string
+            event:
+              type: object
+              additionalProperties: False
+              properties:
+                attributes:
+                  description: Explicit list of the attributes for the notification.
+                  type: array
+                  items:
+                    type: string
+            mcgrp:
+              description: Name of the multicast group generating given notification.
+              type: string
+  mcast-groups:
+    description: List of multicast groups.
+    type: object
+    required: [ list ]
+    additionalProperties: False
+    properties:
+      list:
+        description: List of groups.
+        type: array
+        items:
+          type: object
+          required: [ name ]
+          additionalProperties: False
+          properties:
+            name:
+              description: |
+                The name for the group, used to form the define and the value of the define.
+              type: string
+            # Start genetlink-c
+            c-define-name:
+              description: Override for the name of the define in C uAPI.
+              type: string
+            # End genetlink-c
+            flags: *cmd_flags
+            # Start netlink-raw
+            id:
+              description: Id of the netlink multicast group
+              type: integer
+            # End netlink-raw
-- 
2.41.0

[PATCH net-next v1 2/3] tools/net/ynl: Add support for netlink-raw families

[Donald Hunter]

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

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/nlspec.py |  25 +++++
 tools/net/ynl/lib/ynl.py    | 185 +++++++++++++++++++++++++-----------
 2 files changed, 156 insertions(+), 54 deletions(-)

diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index 0ff0d18666b2..6072c9f35223 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -322,6 +322,20 @@ class SpecOperation(SpecElement):
             self.attr_set = self.family.attr_sets[attr_set_name]
 
 
+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')
+
+
 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
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
+
     def __repr__(self):
         return f"[type:{self.type} len:{self._len}] {self.raw}"
 
@@ -190,6 +206,7 @@ class NlMsg:
 
         self.error = 0
         self.done = 0
+        self.fixed_header_attrs = []
 
         extack_off = None
         if self.nl_type == Netlink.NLMSG_ERROR:
@@ -229,6 +246,24 @@ class NlMsg:
                             desc += f" ({spec['doc']})"
                         self.extack['miss-type'] = desc
 
+    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
+
     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]
+            if op.fixed_header:
+                nl_msg.decode_fixed_header(ynl.consts, op)
 
-        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]
-
-        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 cmd(self):
+        return self.genl_cmd
 
     def __repr__(self):
         msg = repr(self.nl)
@@ -344,9 +377,35 @@ class GenlMsg:
         return msg
 
 
-class GenlFamily:
-    def __init__(self, family_name):
+class NetlinkProtocolFamily:
+    def __init__(self, family_name, proto_num):
         self.family_name = family_name
+        self.proto = proto_num
+
+    def _message(self, nl_type, nl_flags, seq=None):
+        if seq is None:
+            seq = random.randint(1, 1024)
+        nlmsg = struct.pack("HHII", nl_type, nl_flags, seq, 0)
+        return nlmsg
+
+    def message(self, flags, command, version, seq=None):
+        return self._message(command, flags, seq)
+
+    def decode(self, ynl, nl_msg):
+        op = ynl.rsp_by_value[nl_msg.nl_type]
+        if op.fixed_header:
+            nl_msg.decode_fixed_header(ynl.consts, op)
+        nl_msg.raw_attrs = NlAttrs(nl_msg.raw)
+        return nl_msg
+
+    def get_mcast_id(self, mcast_name, mcast_groups):
+        if mcast_name not in mcast_groups:
+            raise Exception(f'Multicast group "{mcast_name}" not present in the spec')
+        return mcast_groups[mcast_name].id
+
+class GenlProtocolFamily(NetlinkProtocolFamily):
+    def __init__(self, family_name):
+        super().__init__(family_name, Netlink.NETLINK_GENERIC)
 
         global genl_family_name_to_id
         if genl_family_name_to_id is None:
@@ -355,6 +414,18 @@ class GenlFamily:
         self.genl_family = genl_family_name_to_id[family_name]
         self.family_id = genl_family_name_to_id[family_name]['id']
 
+    def message(self, flags, command, version, seq=None):
+        nlmsg = self._message(self.family_id, flags, seq)
+        genlmsg = struct.pack("BBH", command, version, 0)
+        return nlmsg + genlmsg
+
+    def decode(self, ynl, nl_msg):
+        return GenlMsg(nl_msg, ynl)
+
+    def get_mcast_id(self, mcast_name, mcast_groups):
+        if mcast_name not in self.genl_family['mcast']:
+            raise Exception(f'Multicast group "{mcast_name}" not present in the family')
+        return self.genl_family['mcast'][mcast_name]
 
 #
 # YNL implementation details.
@@ -367,9 +438,19 @@ class YnlFamily(SpecFamily):
 
         self.include_raw = False
 
-        self.sock = socket.socket(socket.AF_NETLINK, socket.SOCK_RAW, Netlink.NETLINK_GENERIC)
+        try:
+            if self.proto == "netlink-raw":
+                self.family = NetlinkProtocolFamily(self.yaml['name'],
+                                                    self.yaml['protonum'])
+            else:
+                self.family = GenlProtocolFamily(self.yaml['name'])
+        except KeyError:
+            raise Exception(f"Family '{self.yaml['name']}' not supported by the kernel")
+
+        self.sock = socket.socket(socket.AF_NETLINK, socket.SOCK_RAW, self.family.proto)
         self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_CAP_ACK, 1)
         self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_EXT_ACK, 1)
+        self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_GET_STRICT_CHK, 1)
 
         self.async_msg_ids = set()
         self.async_msg_queue = []
@@ -382,18 +463,12 @@ class YnlFamily(SpecFamily):
             bound_f = functools.partial(self._op, op_name)
             setattr(self, op.ident_name, bound_f)
 
-        try:
-            self.family = GenlFamily(self.yaml['name'])
-        except KeyError:
-            raise Exception(f"Family '{self.yaml['name']}' not supported by the kernel")
 
     def ntf_subscribe(self, mcast_name):
-        if mcast_name not in self.family.genl_family['mcast']:
-            raise Exception(f'Multicast group "{mcast_name}" not present in the family')
-
+        mcast_id = self.family.get_mcast_id(mcast_name, self.mcast_groups)
         self.sock.bind((0, 0))
         self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP,
-                             self.family.genl_family['mcast'][mcast_name])
+                             mcast_id)
 
     def _add_attr(self, space, name, value):
         attr = self.attr_sets[space][name]
@@ -419,23 +494,12 @@ class YnlFamily(SpecFamily):
         return struct.pack('HH', len(attr_payload) + 4, nl_type) + attr_payload + pad
 
     def _decode_enum(self, raw, attr_spec):
-        enum = self.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
+        return NlAttr.decode_enum(raw, attr_spec, self.consts)
 
     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)
             for m in members:
                 if m.enum:
                      decoded[m.name] = self._decode_enum(decoded[m.name], m)
@@ -451,12 +515,21 @@ class YnlFamily(SpecFamily):
         attr_space = self.attr_sets[space]
         rsp = dict()
         for attr in attrs:
-            attr_spec = attr_space.attrs_by_val[attr.type]
+            try:
+                attr_spec = attr_space.attrs_by_val[attr.type]
+            except KeyError:
+                print(f"No attribute spec for {attr.type} in attribute space {space}, skipping.")
+                continue
+
             if attr_spec["type"] == 'nest':
                 subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'])
                 decoded = subdict
             elif attr_spec["type"] == 'string':
-                decoded = attr.as_strz()
+                try:
+                    decoded = attr.as_strz()
+                except UnicodeDecodeError:
+                    print(f"Failed to decode string {attr_spec['name']}, skipping")
+                    continue
             elif attr_spec["type"] == 'binary':
                 decoded = self._decode_binary(attr, attr_spec)
             elif attr_spec["type"] == 'flag':
@@ -517,9 +590,12 @@ class YnlFamily(SpecFamily):
         if self.include_raw:
             msg['nlmsg'] = nl_msg
             msg['genlmsg'] = genl_msg
-        op = self.rsp_by_value[genl_msg.genl_cmd]
+        op = self.rsp_by_value[genl_msg.cmd()]
+        decoded = self._decode(genl_msg.raw_attrs, op.attr_set.name)
+        decoded.update(genl_msg.fixed_header_attrs)
+
         msg['name'] = op['name']
-        msg['msg'] = self._decode(genl_msg.raw_attrs, op.attr_set.name)
+        msg['msg'] = decoded
         self.async_msg_queue.append(msg)
 
     def check_ntf(self):
@@ -539,12 +615,12 @@ class YnlFamily(SpecFamily):
                     print("Netlink done while checking for ntf!?")
                     continue
 
-                gm = GenlMsg(nl_msg)
-                if gm.genl_cmd not in self.async_msg_ids:
-                    print("Unexpected msg id done while checking for ntf", gm)
+                decoded = self.family.decode(self, nl_msg)
+                if decoded.cmd() not in self.async_msg_ids:
+                    print("Unexpected msg id done while checking for ntf", decoded)
                     continue
 
-                self.handle_ntf(nl_msg, gm)
+                self.handle_ntf(nl_msg, decoded)
 
     def operation_do_attributes(self, name):
       """
@@ -565,7 +641,7 @@ class YnlFamily(SpecFamily):
             nl_flags |= Netlink.NLM_F_DUMP
 
         req_seq = random.randint(1024, 65535)
-        msg = _genl_msg(self.family.family_id, nl_flags, op.req_value, 1, req_seq)
+        msg = self.family.message(nl_flags, op.req_value, 1, req_seq)
         fixed_header_members = []
         if op.fixed_header:
             fixed_header_members = self.consts[op.fixed_header].members
@@ -597,18 +673,19 @@ class YnlFamily(SpecFamily):
                     done = True
                     break
 
-                gm = GenlMsg(nl_msg, fixed_header_members)
+                decoded = self.family.decode(self, nl_msg)
+
                 # 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:
-                        self.handle_ntf(nl_msg, gm)
+                if nl_msg.nl_seq != req_seq or decoded.cmd() != op.rsp_value:
+                    if decoded.cmd() in self.async_msg_ids:
+                        self.handle_ntf(nl_msg, decoded)
                         continue
                     else:
-                        print('Unexpected message: ' + repr(gm))
+                        print('Unexpected message: ' + repr(decoded))
                         continue
 
-                rsp_msg = self._decode(gm.raw_attrs, op.attr_set.name)
-                rsp_msg.update(gm.fixed_header_attrs)
+                rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name)
+                rsp_msg.update(decoded.fixed_header_attrs)
                 rsp.append(rsp_msg)
 
         if not rsp:
-- 
2.41.0

[PATCH net-next v1 3/3] doc/netlink: Add specs for addr and route rtnetlink message types

[Donald Hunter]

Add netlink-raw specs for the following rtnetlink messages:
 - newaddr, deladdr, getaddr (dump)
 - getroute (dump)

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/specs/rt_addr.yaml  | 179 ++++++++++++++++++++
 Documentation/netlink/specs/rt_route.yaml | 192 ++++++++++++++++++++++
 2 files changed, 371 insertions(+)
 create mode 100644 Documentation/netlink/specs/rt_addr.yaml
 create mode 100644 Documentation/netlink/specs/rt_route.yaml

diff --git a/Documentation/netlink/specs/rt_addr.yaml b/Documentation/netlink/specs/rt_addr.yaml
new file mode 100644
index 000000000000..f97ae7c35e44
--- /dev/null
+++ b/Documentation/netlink/specs/rt_addr.yaml
@@ -0,0 +1,179 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: rt-addr
+protocol: netlink-raw
+protonum: 0
+
+doc:
+  Address configuration over rtnetlink.
+
+definitions:
+  -
+    name: ifaddrmsg
+    type: struct
+    members:
+      -
+        name: ifa-family
+        type: u8
+      -
+        name: ifa-prefixlen
+        type: u8
+      -
+        name: ifa-flags
+        type: u8
+        enum: ifa-flags
+        enum-as-flags: true
+      -
+        name: ifa-scope
+        type: u8
+      -
+        name: ifa-index
+        type: u32
+  -
+    name: ifa-cacheinfo
+    type: struct
+    members:
+      -
+        name: ifa-prefered
+        type: u32
+      -
+        name: ifa-valid
+        type: u32
+      -
+        name: cstamp
+        type: u32
+      -
+        name: tstamp
+        type: u32
+
+  -
+    name: ifa-flags
+    type: flags
+    entries:
+      -
+        name: secondary
+      -
+        name: nodad
+      -
+        name: optimistic
+      -
+        name: dadfailed
+      -
+        name: homeaddress
+      -
+        name: deprecated
+      -
+        name: tentative
+      -
+        name: permanent
+      -
+        name: managetempaddr
+      -
+        name: noprefixroute
+      -
+        name: mcautojoin
+      -
+        name: stable-privacy
+
+attribute-sets:
+  -
+    name: addr-attrs
+    attributes:
+      -
+        name: ifa-address
+        type: binary
+        display-hint: ipv4
+      -
+        name: ifa-local
+        type: binary
+        display-hint: ipv4
+      -
+        name: ifa-label
+        type: string
+      -
+        name: ifa-broadcast
+        type: binary
+        display-hint: ipv4
+      -
+        name: ifa-anycast
+        type: binary
+      -
+        name: ifa-cacheinfo
+        type: binary
+        struct: ifa-cacheinfo
+      -
+        name: ifa-multicast
+        type: binary
+      -
+        name: ifa-flags
+        type: u32
+        enum: ifa-flags
+        enum-as-flags: true
+      -
+        name: ifa-rt-priority
+        type: u32
+      -
+        name: ifa-target-netnsid
+        type: binary
+      -
+        name: ifa-proto
+        type: u8
+
+
+operations:
+  fixed-header: ifaddrmsg
+  enum-model: directional
+  list:
+    -
+      name: newaddr
+      doc: Add new address
+      attribute-set: addr-attrs
+      do:
+        request:
+          value: 20
+          attributes: &ifaddr-all
+            - ifa-family
+            - ifa-flags
+            - ifa-prefixlen
+            - ifa-scope
+            - ifa-index
+            - ifa-address
+            - ifa-label
+            - ifa-local
+            - ifa-cacheinfo
+    -
+      name: deladdr
+      doc: Remove address
+      attribute-set: addr-attrs
+      do:
+        request:
+          value: 21
+          attributes:
+            - ifa-family
+            - ifa-flags
+            - ifa-prefixlen
+            - ifa-scope
+            - ifa-index
+            - ifa-address
+            - ifa-local
+    -
+      name: getaddr
+      doc: Dump address information.
+      attribute-set: addr-attrs
+      dump:
+        request:
+          value: 22
+          attributes:
+            - index
+        reply:
+          value: 20
+          attributes: *ifaddr-all
+
+mcast-groups:
+  list:
+    -
+      name: rtnlgrp-ipv4-ifaddr
+      id: 5
+    -
+      name: rtnlgrp-ipv6-ifaddr
+      id: 9
diff --git a/Documentation/netlink/specs/rt_route.yaml b/Documentation/netlink/specs/rt_route.yaml
new file mode 100644
index 000000000000..882af9b50bd4
--- /dev/null
+++ b/Documentation/netlink/specs/rt_route.yaml
@@ -0,0 +1,192 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: rt-route
+protocol: netlink-raw
+protonum: 0
+
+doc:
+  Route configuration over rtnetlink.
+
+definitions:
+  -
+    name: rtm-type
+    name-prefix: rtn-
+    type: enum
+    entries:
+      - unspec
+      - unicast
+      - local
+      - broadcast
+      - anycast
+      - multicast
+      - blackhole
+      - unreachable
+      - prohibit
+      - throw
+      - nat
+      - xresolve
+  -
+    name: rtmsg
+    type: struct
+    members:
+      -
+        name: rtm-family
+        type: u8
+      -
+        name: rtm-dst-len
+        type: u8
+      -
+        name: rtm-src-len
+        type: u8
+      -
+        name: rtm-tos
+        type: u8
+      -
+        name: rtm-table
+        type: u8
+      -
+        name: rtm-protocol
+        type: u8
+      -
+        name: rtm-scope
+        type: u8
+      -
+        name: rtm-type
+        type: u8
+        enum: rtm-type
+      -
+        name: rtm-flags
+        type: u32
+  -
+    name: rta-cacheinfo
+    type: struct
+    members:
+      -
+        name: rta-clntref
+        type: u32
+      -
+        name: rta-lastuse
+        type: u32
+      -
+        name: rta-expires
+        type: u32
+      -
+        name: rta-error
+        type: u32
+      -
+        name: rta-used
+        type: u32
+
+attribute-sets:
+  -
+    name: route-attrs
+    attributes:
+      -
+        name: rta-dst
+        type: binary
+        display-hint: ipv4
+      -
+        name: rta-src
+        type: binary
+        display-hint: ipv4
+      -
+        name: rta-iif
+        type: u32
+      -
+        name: rta-oif
+        type: u32
+      -
+        name: rta-gateway
+        type: binary
+        display-hint: ipv4
+      -
+        name: rta-priority
+        type: binary
+      -
+        name: rta-prefsrc
+        type: binary
+        display-hint: ipv4
+      -
+        name: rta-metrics
+        type: binary
+      -
+        name: rta-multipath
+        type: binary
+      -
+        name: rta-protoinfo
+        type: binary
+      -
+        name: rta-flow
+        type: u32
+      -
+        name: rta-cacheinfo
+        type: binary
+        struct: rta-cacheinfo
+      -
+        name: rta-session
+        type: binary
+      -
+        name: rta-mp-algo
+        type: binary
+      -
+        name: rta-table
+        type: u32
+      -
+        name: rta-mark
+        type: binary
+      -
+        name: rta-mfc-stats
+        type: binary
+      -
+        name: rta-via
+        type: binary
+      -
+        name: rta-newdst
+        type: binary
+      -
+        name: rta-pref
+        type: binary
+      -
+        name: rta-encap-type
+        type: binary
+      -
+        name: rta-encap
+        type: binary
+      -
+        name: rta-expires
+        type: binary
+      -
+        name: rta-pad
+        type: binary
+      -
+        name: rta-uid
+        type: binary
+      -
+        name: rta-ttl-propagate
+        type: binary
+      -
+        name: rta-ip-proto
+        type: binary
+      -
+        name: rta-sport
+        type: binary
+      -
+        name: rta-dport
+        type: binary
+      -
+        name: rta-nh-id
+        type: binary
+
+operations:
+  enum-model: directional
+  list:
+    -
+      name: getroute
+      doc: Dump route information.
+      attribute-set: route-attrs
+      fixed-header: rtmsg
+      dump:
+        request:
+          value: 26
+        reply:
+          value: 24
-- 
2.41.0

Re: [PATCH net-next v1 0/3] tools/net/ynl: Add support for netlink-raw families

[Jakub Kicinski]

On Tue, 25 Jul 2023 17:22:02 +0100 Donald Hunter wrote:
> This patchset adds support for netlink-raw families such as rtnetlink.
> 
> The first patch contains the schema definition.
> The second patch extends ynl to support netlink-raw
> The third patch adds rtnetlink addr and route message types

Haven't gotten to it today, unfortunately, but very exciting!

Re: [PATCH net-next v1 0/3] tools/net/ynl: Add support for netlink-raw families

[Simon Horman]

On Tue, Jul 25, 2023 at 05:22:02PM +0100, Donald Hunter wrote:
> This patchset adds support for netlink-raw families such as rtnetlink.
> 
> The first patch contains the schema definition.
> The second patch extends ynl to support netlink-raw
> The third patch adds rtnetlink addr and route message types
> 
> The second patch depends on "tools: ynl-gen: fix parse multi-attr enum
> attribute":
> 
> https://patchwork.kernel.org/project/netdevbpf/list/?series=769229
> 
> The netlink-raw schema is very similar to genetlink-legacy and I thought
> about making the changes there and symlinking to it. On balance I
> thought that might be problematic for accurate schema validation.
> 
> rtnetlink doesn't seem to fit into unified or directional message
> enumeration models. It seems like an 'explicit' model would be useful,
> to require the schema author to specify the message ids directly. The
> patch supports commands and it supports notifications, but it's
> currently hard to support both simultaneously from the same netlink-raw
> spec. I plan to work on this in a future patchset.
> 
> There is not yet support for notifications because ynl currently doesn't
> support defining 'event' properties on a 'do' operation. I plan to work
> on this in a future patch.
> 
> The link message types are a work in progress that I plan to submit in a
> future patchset. Links contain different nested attributes dependent on
> the type of link. Decoding these will need some kind of attr-space
> selection based on the value of another attribute in the message.
> 
> Donald Hunter (3):
>   doc/netlink: Add a schema for netlink-raw families
>   tools/net/ynl: Add support for netlink-raw families
>   doc/netlink: Add specs for addr and route rtnetlink message types

Hi Donald,

unfortunately this series doesn't apply to current net-next.
Please consider rebasing and reposting.

-- 
pw-bot: changes-requested

Re: [PATCH net-next v1 0/3] tools/net/ynl: Add support for netlink-raw families

[Donald Hunter]

On Wed, 26 Jul 2023 at 13:38, Simon Horman <simon.horman@corigine.com> wrote:
>
> On Tue, Jul 25, 2023 at 05:22:02PM +0100, Donald Hunter wrote:
> > This patchset adds support for netlink-raw families such as rtnetlink.
> >
> > The first patch contains the schema definition.
> > The second patch extends ynl to support netlink-raw
> > The third patch adds rtnetlink addr and route message types
> >
> > The second patch depends on "tools: ynl-gen: fix parse multi-attr enum
> > attribute":
> >
> > https://patchwork.kernel.org/project/netdevbpf/list/?series=769229
> >
> > The netlink-raw schema is very similar to genetlink-legacy and I thought
> > about making the changes there and symlinking to it. On balance I
> > thought that might be problematic for accurate schema validation.
> >
> > rtnetlink doesn't seem to fit into unified or directional message
> > enumeration models. It seems like an 'explicit' model would be useful,
> > to require the schema author to specify the message ids directly. The
> > patch supports commands and it supports notifications, but it's
> > currently hard to support both simultaneously from the same netlink-raw
> > spec. I plan to work on this in a future patchset.
> >
> > There is not yet support for notifications because ynl currently doesn't
> > support defining 'event' properties on a 'do' operation. I plan to work
> > on this in a future patch.
> >
> > The link message types are a work in progress that I plan to submit in a
> > future patchset. Links contain different nested attributes dependent on
> > the type of link. Decoding these will need some kind of attr-space
> > selection based on the value of another attribute in the message.
> >
> > Donald Hunter (3):
> >   doc/netlink: Add a schema for netlink-raw families
> >   tools/net/ynl: Add support for netlink-raw families
> >   doc/netlink: Add specs for addr and route rtnetlink message types
>
> Hi Donald,
>
> unfortunately this series doesn't apply to current net-next.
> Please consider rebasing and reposting.

Hi Simon,

As I mentioned in the cover letter, it depends on:
"tools: ynl-gen: fix parse multi-attr enum attribute"
https://patchwork.kernel.org/project/netdevbpf/list/?series=769229

Should I wait for that and repost?

Thanks,
Donald.

Re: [PATCH net-next v1 0/3] tools/net/ynl: Add support for netlink-raw families

[Simon Horman]

On Wed, Jul 26, 2023 at 02:06:02PM +0100, Donald Hunter wrote:
> On Wed, 26 Jul 2023 at 13:38, Simon Horman <simon.horman@corigine.com> wrote:
> >
> > On Tue, Jul 25, 2023 at 05:22:02PM +0100, Donald Hunter wrote:
> > > This patchset adds support for netlink-raw families such as rtnetlink.
> > >
> > > The first patch contains the schema definition.
> > > The second patch extends ynl to support netlink-raw
> > > The third patch adds rtnetlink addr and route message types
> > >
> > > The second patch depends on "tools: ynl-gen: fix parse multi-attr enum
> > > attribute":
> > >
> > > https://patchwork.kernel.org/project/netdevbpf/list/?series=769229
> > >
> > > The netlink-raw schema is very similar to genetlink-legacy and I thought
> > > about making the changes there and symlinking to it. On balance I
> > > thought that might be problematic for accurate schema validation.
> > >
> > > rtnetlink doesn't seem to fit into unified or directional message
> > > enumeration models. It seems like an 'explicit' model would be useful,
> > > to require the schema author to specify the message ids directly. The
> > > patch supports commands and it supports notifications, but it's
> > > currently hard to support both simultaneously from the same netlink-raw
> > > spec. I plan to work on this in a future patchset.
> > >
> > > There is not yet support for notifications because ynl currently doesn't
> > > support defining 'event' properties on a 'do' operation. I plan to work
> > > on this in a future patch.
> > >
> > > The link message types are a work in progress that I plan to submit in a
> > > future patchset. Links contain different nested attributes dependent on
> > > the type of link. Decoding these will need some kind of attr-space
> > > selection based on the value of another attribute in the message.
> > >
> > > Donald Hunter (3):
> > >   doc/netlink: Add a schema for netlink-raw families
> > >   tools/net/ynl: Add support for netlink-raw families
> > >   doc/netlink: Add specs for addr and route rtnetlink message types
> >
> > Hi Donald,
> >
> > unfortunately this series doesn't apply to current net-next.
> > Please consider rebasing and reposting.
> 
> Hi Simon,
> 
> As I mentioned in the cover letter, it depends on:
> "tools: ynl-gen: fix parse multi-attr enum attribute"
> https://patchwork.kernel.org/project/netdevbpf/list/?series=769229
> 
> Should I wait for that and repost?

Sorry my bad. I guess this is fine as-is unless Jakub says otherwise.

-- 
pw-bot: under-review

Re: [PATCH net-next v1 0/3] tools/net/ynl: Add support for netlink-raw families

[Jakub Kicinski]

On Wed, 26 Jul 2023 15:16:11 +0200 Simon Horman wrote:
> > As I mentioned in the cover letter, it depends on:
> > "tools: ynl-gen: fix parse multi-attr enum attribute"
> > https://patchwork.kernel.org/project/netdevbpf/list/?series=769229
> > 
> > Should I wait for that and repost?  
> 
> Sorry my bad. I guess this is fine as-is unless Jakub says otherwise.

Right, just to be 100% clear, please don't repost, yet. I'll review it
as is since it's of particular interest to me :)
Simon is right that we don't accept patches which have yet-to-be-applied
dependencies, tho.

Re: [PATCH net-next v1 0/3] tools/net/ynl: Add support for netlink-raw families

[Donald Hunter]

On Wed, 26 Jul 2023 at 16:55, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 26 Jul 2023 15:16:11 +0200 Simon Horman wrote:
> > > As I mentioned in the cover letter, it depends on:
> > > "tools: ynl-gen: fix parse multi-attr enum attribute"
> > > https://patchwork.kernel.org/project/netdevbpf/list/?series=769229
> > >
> > > Should I wait for that and repost?
> >
> > Sorry my bad. I guess this is fine as-is unless Jakub says otherwise.
>
> Right, just to be 100% clear, please don't repost, yet. I'll review it
> as is since it's of particular interest to me :)
> Simon is right that we don't accept patches which have yet-to-be-applied
> dependencies, tho.

Ack, noted. Apologies for rushing it.

Re: [PATCH net-next v1 1/3] doc/netlink: Add a schema for netlink-raw families

[Jakub Kicinski]

On Tue, 25 Jul 2023 17:22:03 +0100 Donald Hunter wrote:
>  - add an id property to mcast-group definitions

nit: my first thought would be to call it "value" rather than "id"
to stick with the naming we have to other objects. Any pros/cons
you see?

Other than that the big ask would be to update the documentation :)

Re: [PATCH net-next v1 2/3] tools/net/ynl: Add support for netlink-raw families

[Jakub Kicinski]

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?

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

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

>      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)

Re: [PATCH net-next v1 1/3] doc/netlink: Add a schema for netlink-raw families

[Donald Hunter]

On Wed, 26 Jul 2023 at 22:09, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 25 Jul 2023 17:22:03 +0100 Donald Hunter wrote:
> >  - add an id property to mcast-group definitions
>
> nit: my first thought would be to call it "value" rather than "id"
> to stick with the naming we have to other objects. Any pros/cons
> you see?

Yep, that makes sense. I wasn't sure about "id" anyway but it didn't occur
to me to align with "value".

> Other than that the big ask would be to update the documentation :)

Do you mind if I update the docs as a follow up patch? I'm on holiday for
the next 10 days - I should have time to respin this patchset but maybe
not to work on docs as well.

Re: [PATCH net-next v1 2/3] tools/net/ynl: Add support for netlink-raw families

[Donald Hunter]

On Wed, 26 Jul 2023 at 22:37, Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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?

Ack.

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

> > 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.

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

> >      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.

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

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

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

Re: [PATCH net-next v1 1/3] doc/netlink: Add a schema for netlink-raw families

[Jakub Kicinski]

On Wed, 26 Jul 2023 22:48:54 +0100 Donald Hunter wrote:
> > Other than that the big ask would be to update the documentation :)  
> 
> Do you mind if I update the docs as a follow up patch? I'm on holiday for
> the next 10 days - I should have time to respin this patchset but maybe
> not to work on docs as well.

Ugh, we had some issues with people posting stuff while / before taking
time off recently (napi netlink, sendpage, tcx). I'd rather wait until
you're back.

Re: [PATCH net-next v1 2/3] tools/net/ynl: Add support for netlink-raw families

[Jakub Kicinski]

On Wed, 26 Jul 2023 23:01:12 +0100 Donald Hunter wrote:
> > 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.

Ynl with multiple specs is doable from the technical standpoint,
I think the API to expose from such a library may be a bigger challenge.
And I'm not sure if it's worth the complexity in practice.

> > 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.

FWIW I ended up merging the fixes to net, but they should be in
net-next by tomorrow afternoon.