[PATCH net-next v1 0/2] ynl: add support for user headers and struct attrs

[PATCH net-next v1 0/2] ynl: add support for user headers and struct attrs

[Donald Hunter]

Add support for user headers and struct attrs to YNL.

Patch 1 adds the capabilities to YNL.
Patch 2 adds partial openvswitch specs that demonstrate the new features.

Donald Hunter (2):
  tools: ynl: add user-header and struct attr support
  netlink: specs: add partial specification for openvswitch

 Documentation/netlink/genetlink-legacy.yaml   |  10 +-
 Documentation/netlink/specs/ovs_datapath.yaml | 154 ++++++++++++++++++
 Documentation/netlink/specs/ovs_vport.yaml    | 141 ++++++++++++++++
 tools/net/ynl/lib/ynl.py                      |  58 ++++++-
 4 files changed, 355 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/netlink/specs/ovs_datapath.yaml
 create mode 100644 Documentation/netlink/specs/ovs_vport.yaml

-- 
2.39.0

[PATCH net-next v1 1/2] tools: ynl: add user-header and struct attr support

[Donald Hunter]

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/genetlink-legacy.yaml | 10 +++-
 tools/net/ynl/lib/ynl.py                    | 58 ++++++++++++++++++---
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index c6b8c77f7d12..7f019c0a9762 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -53,6 +53,9 @@ properties:
       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 ]
+  user-header:
+    description: Name of the struct definition for the user header for the family.
+    type: string
   # End genetlink-legacy
 
   definitions:
@@ -172,7 +175,7 @@ properties:
                 type: string
               type: &attr-type
                 enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64,
-                        string, nest, array-nest, nest-type-value ]
+                        string, nest, array-nest, nest-type-value, struct ]
               doc:
                 description: Documentation of the attribute.
                 type: string
@@ -218,6 +221,11 @@ properties:
                     description: Max length for a string or a binary attribute.
                     $ref: '#/$defs/len-or-define'
               sub-type: *attr-type
+              # 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:
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 90764a83c646..584b1e0a6b2f 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -68,6 +68,11 @@ class Netlink:
 
 
 class NlAttr:
+    type_formats = { 'u8' : ('B', 1),
+                     'u16': ('H', 2),
+                     'u32': ('I', 4),
+                     'u64': ('Q', 8) }
+
     def __init__(self, raw, offset):
         self._len, self._type = struct.unpack("HH", raw[offset:offset + 4])
         self.type = self._type & ~Netlink.NLA_TYPE_MASK
@@ -93,6 +98,21 @@ class NlAttr:
     def as_bin(self):
         return self.raw
 
+    def as_array(self, type):
+        format, _ = self.type_formats[type]
+        return list({ x[0] for x in struct.iter_unpack(format, self.raw) })
+
+    def as_struct(self, members):
+        value = dict()
+        offset = 0
+        for m in members:
+            type = m['type']
+            format, size = self.type_formats[type]
+            decoded = struct.unpack_from(format, self.raw, offset)
+            offset += size
+            value[m['name']] = decoded[0]
+        return value
+
     def __repr__(self):
         return f"[type:{self.type} len:{self._len}] {self.raw}"
 
@@ -200,7 +220,7 @@ def _genl_msg(nl_type, nl_flags, genl_cmd, genl_version, seq=None):
     if seq is None:
         seq = random.randint(1, 1024)
     nlmsg = struct.pack("HHII", nl_type, nl_flags, seq, 0)
-    genlmsg = struct.pack("bbH", genl_cmd, genl_version, 0)
+    genlmsg = struct.pack("BBH", genl_cmd, genl_version, 0)
     return nlmsg + genlmsg
 
 
@@ -258,14 +278,22 @@ def _genl_load_families():
 
 
 class GenlMsg:
-    def __init__(self, nl_msg):
+    def __init__(self, nl_msg, extra_headers = []):
         self.nl = nl_msg
 
         self.hdr = nl_msg.raw[0:4]
-        self.raw = nl_msg.raw[4:]
+        offset = 4
 
-        self.genl_cmd, self.genl_version, _ = struct.unpack("bbH", self.hdr)
+        self.genl_cmd, self.genl_version, _ = struct.unpack("BBH", self.hdr)
 
+        self.user_attrs = dict()
+        for m in extra_headers:
+            format, size = NlAttr.type_formats[m['type']]
+            decoded = struct.unpack_from(format, nl_msg.raw, offset)
+            offset += size
+            self.user_attrs[m['name']] = decoded[0]
+
+        self.raw = nl_msg.raw[offset:]
         self.raw_attrs = NlAttrs(self.raw)
 
     def __repr__(self):
@@ -315,6 +343,7 @@ class YnlFamily(SpecFamily):
             setattr(self, op.ident_name, bound_f)
 
         self.family = GenlFamily(self.yaml['name'])
+        self._user_header = self.yaml.get('user-header', None)
 
     def ntf_subscribe(self, mcast_name):
         if mcast_name not in self.family.genl_family['mcast']:
@@ -358,7 +387,7 @@ class YnlFamily(SpecFamily):
                 raw >>= 1
                 i += 1
         else:
-            value = enum['entries'][raw - i]
+            value = enum.entries_by_val[raw - i]['name']
         rsp[attr_spec['name']] = value
 
     def _decode(self, attrs, space):
@@ -381,6 +410,14 @@ class YnlFamily(SpecFamily):
                 decoded = attr.as_bin()
             elif attr_spec["type"] == 'flag':
                 decoded = True
+            elif attr_spec["type"] == 'struct':
+                s = attr_spec['struct']
+                decoded = attr.as_struct(self.consts[s]['members'])
+            elif attr_spec["type"] == 'array-nest':
+                decoded = attr.as_array(attr_spec["sub-type"])
+            elif attr_spec["type"] == 'unused':
+                print(f"Warning: skipping unused attribute {attr_spec['name']}")
+                continue
             else:
                 raise Exception(f'Unknown {attr.type} {attr_spec["name"]} {attr_spec["type"]}')
 
@@ -472,6 +509,13 @@ class YnlFamily(SpecFamily):
 
         req_seq = random.randint(1024, 65535)
         msg = _genl_msg(self.family.family_id, nl_flags, op.req_value, 1, req_seq)
+        user_headers = []
+        if self._user_header:
+            user_headers = self.consts[self._user_header]['members']
+            for m in user_headers:
+                value = vals.pop(m['name'])
+                format, _ = NlAttr.type_formats[m['type']]
+                msg += struct.pack(format, value)
         for name, value in vals.items():
             msg += self._add_attr(op.attr_set.name, name, value)
         msg = _genl_msg_finalize(msg)
@@ -498,7 +542,7 @@ class YnlFamily(SpecFamily):
                     done = True
                     break
 
-                gm = GenlMsg(nl_msg)
+                gm = GenlMsg(nl_msg, user_headers)
                 # 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:
@@ -508,7 +552,7 @@ class YnlFamily(SpecFamily):
                         print('Unexpected message: ' + repr(gm))
                         continue
 
-                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name))
+                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name) | gm.user_attrs)
 
         if not rsp:
             return None
-- 
2.39.0

[PATCH net-next v1 2/2] netlink: specs: add partial specification for openvswitch

[Donald Hunter]

The openvswitch family has a user header, uses struct attrs and has
array values and demonstrates these features in the YNL CLI. These specs
are sufficient to create, delete and dump datapaths and to dump vports:

$ ./tools/net/ynl/cli.py \
    --schema Documentation/netlink/genetlink-legacy.yaml \
    --spec Documentation/netlink/specs/ovs_datapath.yaml \
    --do dp-new --json '{ "dp_ifindex": 0, "name": "demo", "upcall_pid": 0}'
None

$ ./tools/net/ynl/cli.py \
    --schema Documentation/netlink/genetlink-legacy.yaml \
    --spec Documentation/netlink/specs/ovs_datapath.yaml \
    --dump dp-get --json '{ "dp_ifindex": 0 }'
[{'dp_ifindex': 3,
  'masks_cache_size': 256,
  'megaflow_stats': {'cache_hits': 0,
                     'mask_hit': 0,
                     'masks': 0,
                     'pad1': 0,
                     'padding': 0},
  'name': 'test',
  'stats': {'flows': 0, 'hit': 0, 'lost': 0, 'missed': 0},
  'user_features': {'dispatch_upcall_per_cpu',
                    'tc_recirc_sharing',
                    'unaligned'}},
 {'dp_ifindex': 39,
  'masks_cache_size': 256,
  'megaflow_stats': {'cache_hits': 0,
                     'mask_hit': 0,
                     'masks': 0,
                     'pad1': 0,
                     'padding': 0},
  'name': 'demo',
  'stats': {'flows': 0, 'hit': 0, 'lost': 0, 'missed': 0},
  'user_features': set()}]

$ ./tools/net/ynl/cli.py \
    --schema Documentation/netlink/genetlink-legacy.yaml \
    --spec Documentation/netlink/specs/ovs_datapath.yaml \
    --do dp-del --json '{ "dp_ifindex": 0, "name": "demo"}'
None

$ ./tools/net/ynl/cli.py \
    --schema Documentation/netlink/genetlink-legacy.yaml \
    --spec Documentation/netlink/specs/ovs_vport.yaml \
    --dump vport-get --json '{ "dp_ifindex": 3 }'
[{'dp_ifindex': 3,
  'ifindex': 3,
  'name': 'test',
  'port_no': 0,
  'stats': {'rx_bytes': 0,
            'rx_dropped': 0,
            'rx_errors': 0,
            'rx_packets': 0,
            'tx_bytes': 0,
            'tx_dropped': 0,
            'tx_errors': 0,
            'tx_packets': 0},
  'type': 'internal',
  'upcall_pid': [0],
  'upcall_stats': {'fail': 0, 'success': 0}}]

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/specs/ovs_datapath.yaml | 154 ++++++++++++++++++
 Documentation/netlink/specs/ovs_vport.yaml    | 141 ++++++++++++++++
 2 files changed, 295 insertions(+)
 create mode 100644 Documentation/netlink/specs/ovs_datapath.yaml
 create mode 100644 Documentation/netlink/specs/ovs_vport.yaml

diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
new file mode 100644
index 000000000000..c420f78f7c25
--- /dev/null
+++ b/Documentation/netlink/specs/ovs_datapath.yaml
@@ -0,0 +1,154 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: ovs_datapath
+version: 2
+protocol: genetlink-legacy
+
+doc:
+  OVS datapath configuration over generic netlink.
+
+definitions:
+  -
+    name: ovs_header
+    type: struct
+    members:
+      -
+        name: dp_ifindex
+        type: u32
+  -
+    name: user_features
+    type: flags
+    entries:
+      -
+        name: unaligned
+        doc: Allow last Netlink attribute to be unaligned
+      -
+        name: vport_pids
+        doc: Allow datapath to associate multiple Netlink PIDs to each vport
+      -
+        name: tc_recirc_sharing
+        doc: Allow tc offload recirc sharing
+      -
+        name: dispatch_upcall_per_cpu
+        doc: Allow per-cpu dispatch of upcalls
+  -
+    name: datapath_stats
+    type: struct
+    members:
+      -
+        name: hit
+        type: u64
+      -
+        name: missed
+        type: u64
+      -
+        name: lost
+        type: u64
+      -
+        name: flows
+        type: u64
+  -
+    name: megaflow_stats
+    type: struct
+    members:
+      -
+        name: mask_hit
+        type: u64
+      -
+        name: masks
+        type: u32
+      -
+        name: padding
+        type: u32
+      -
+        name: cache_hits
+        type: u64
+      -
+        name: pad1
+        type: u64
+
+user-header: ovs_header
+
+attribute-sets:
+  -
+    name: datapath
+    attributes:
+      -
+        name: name
+        type: string
+      -
+        name: upcall_pid
+        doc: upcall pid
+        type: u32
+      -
+        name: stats
+        type: struct
+        struct: datapath_stats
+      -
+        name: megaflow_stats
+        type: struct
+        struct: megaflow_stats
+      -
+        name: user_features
+        type: u32
+        enum: user_features
+        enum-as-flags: true
+      -
+        name: pad
+        type: unused
+      -
+        name: masks_cache_size
+        type: u32
+      -
+        name: per_cpu_pids
+        type: array-nest
+        sub-type: u32
+
+operations:
+  list:
+    -
+      name: dp-get
+      doc: Get / dump OVS data path configuration and state
+      value: 3
+      attribute-set: datapath
+      do: &dp-get-op
+        request:
+          attributes:
+            - name
+        reply:
+          attributes:
+            - name
+            - upcall_pid
+            - stats
+            - megaflow_stats
+            - user_features
+            - masks_cache_size
+            - per_cpu_pids
+      dump: *dp-get-op
+    -
+      name: dp-new
+      doc: Create new OVS data path
+      value: 1
+      attribute-set: datapath
+      do:
+        request:
+          attributes:
+            - dp_ifindex
+            - name
+            - upcall_pid
+            - user_features
+    -
+      name: dp-del
+      doc: Delete existing OVS data path
+      value: 2
+      attribute-set: datapath
+      do:
+        request:
+          attributes:
+            - dp_ifindex
+            - name
+
+mcast-groups:
+  list:
+    -
+      name: ovs_datapath
diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
new file mode 100644
index 000000000000..3913aded5e28
--- /dev/null
+++ b/Documentation/netlink/specs/ovs_vport.yaml
@@ -0,0 +1,141 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: ovs_vport
+version: 2
+protocol: genetlink-legacy
+
+doc:
+  OVS vport configuration over generic netlink.
+
+definitions:
+  -
+    name: ovs_header
+    type: struct
+    members:
+      -
+        name: dp_ifindex
+        type: u32
+  -
+    name: vport_type
+    type: enum
+    entries: [ unspec, netdev, internal, gre, vxlan, geneve ]
+  -
+    name: vport_stats
+    type: struct
+    members:
+      -
+        name: rx_packets
+        type: u64
+      -
+        name: tx_packets
+        type: u64
+      -
+        name: rx_bytes
+        type: u64
+      -
+        name: tx_bytes
+        type: u64
+      -
+        name: rx_errors
+        type: u64
+      -
+        name: tx_errors
+        type: u64
+      -
+        name: rx_dropped
+        type: u64
+      -
+        name: tx_dropped
+        type: u64
+
+
+user-header: ovs_header
+
+attribute-sets:
+  -
+    name: vport-options
+    attributes:
+      -
+        name: dst_port
+        type: u32
+      -
+        name: extension
+        type: u32
+  -
+    name: upcall-stats
+    attributes:
+      -
+        name: success
+        type: u64
+        value: 0
+      -
+        name: fail
+        type: u64
+  -
+    name: vport
+    attributes:
+      -
+        name: port_no
+        type: u32
+      -
+        name: type
+        type: u32
+        enum: vport_type
+      -
+        name: name
+        type: string
+      -
+        name: options
+        type: nest
+        nested-attributes: vport-options
+      -
+        name: upcall_pid
+        type: array-nest
+        sub-type: u32
+      -
+        name: stats
+        type: struct
+        struct: vport_stats
+      -
+        name: pad
+        type: unused
+      -
+        name: ifindex
+        type: u32
+      -
+        name: netnsid
+        type: u32
+      -
+        name: upcall_stats
+        type: nest
+        nested-attributes: upcall-stats
+
+operations:
+  list:
+    -
+      name: vport-get
+      doc: Get / dump OVS vport configuration and state
+      value: 3
+      attribute-set: vport
+      do: &vport-get-op
+        request:
+          attributes:
+            - dp_ifindex
+            - name
+        reply: &dev-all
+          attributes:
+            - dp_ifindex
+            - port_no
+            - type
+            - name
+            - upcall_pid
+            - stats
+            - ifindex
+            - netnsid
+            - upcall_stats
+      dump: *vport-get-op
+
+mcast-groups:
+  list:
+    -
+      name: ovs_vport
-- 
2.39.0

Re: [PATCH net-next v1 1/2] tools: ynl: add user-header and struct attr support

[Jakub Kicinski]

On Thu, 16 Mar 2023 12:01:41 +0000 Donald Hunter wrote:
> Subject: [PATCH net-next v1 1/2] tools: ynl: add user-header and struct attr support

The use of "and" usually indicates it should be 2 separate patches ;)

> Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
> ---
>  Documentation/netlink/genetlink-legacy.yaml | 10 +++-
>  tools/net/ynl/lib/ynl.py                    | 58 ++++++++++++++++++---
>  2 files changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
> index c6b8c77f7d12..7f019c0a9762 100644
> --- a/Documentation/netlink/genetlink-legacy.yaml
> +++ b/Documentation/netlink/genetlink-legacy.yaml
> @@ -53,6 +53,9 @@ properties:
>        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 ]
> +  user-header:
> +    description: Name of the struct definition for the user header for the family.
> +    type: string

Took me a minute to remember this is header as in protocol header 
not header as in C header file :) Would it possibly be better to call it
fixed-header ? Can't really decide myself.

But the description definitely need to be more verbose:

description: |
  Name of the structure defining the fixed-length protocol header.
  This header is placed in a message after the netlink and genetlink
  headers and before any attributes.

>    # End genetlink-legacy
>  
>    definitions:
> @@ -172,7 +175,7 @@ properties:
>                  type: string
>                type: &attr-type
>                  enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64,
> -                        string, nest, array-nest, nest-type-value ]
> +                        string, nest, array-nest, nest-type-value, struct ]
>                doc:
>                  description: Documentation of the attribute.
>                  type: string
> @@ -218,6 +221,11 @@ properties:
>                      description: Max length for a string or a binary attribute.
>                      $ref: '#/$defs/len-or-define'
>                sub-type: *attr-type
> +              # 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:
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 90764a83c646..584b1e0a6b2f 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -68,6 +68,11 @@ class Netlink:
>  
>  
>  class NlAttr:
> +    type_formats = { 'u8' : ('B', 1),
> +                     'u16': ('H', 2),
> +                     'u32': ('I', 4),
> +                     'u64': ('Q', 8) }
> +
>      def __init__(self, raw, offset):
>          self._len, self._type = struct.unpack("HH", raw[offset:offset + 4])
>          self.type = self._type & ~Netlink.NLA_TYPE_MASK
> @@ -93,6 +98,21 @@ class NlAttr:
>      def as_bin(self):
>          return self.raw
>  
> +    def as_array(self, type):
> +        format, _ = self.type_formats[type]
> +        return list({ x[0] for x in struct.iter_unpack(format, self.raw) })

The Python is strong within you :)

> +    def as_struct(self, members):
> +        value = dict()
> +        offset = 0
> +        for m in members:
> +            type = m['type']

Accessing the spec components directly is a bit of an anti-pattern,
can we parse the struct description into Python objects in
tools/net/ynl/lib/nlspec.py ?

> +            format, size = self.type_formats[type]
> +            decoded = struct.unpack_from(format, self.raw, offset)
> +            offset += size
> +            value[m['name']] = decoded[0]
> +        return value
> +
>      def __repr__(self):
>          return f"[type:{self.type} len:{self._len}] {self.raw}"
>  
> @@ -200,7 +220,7 @@ def _genl_msg(nl_type, nl_flags, genl_cmd, genl_version, seq=None):
>      if seq is None:
>          seq = random.randint(1, 1024)
>      nlmsg = struct.pack("HHII", nl_type, nl_flags, seq, 0)
> -    genlmsg = struct.pack("bbH", genl_cmd, genl_version, 0)
> +    genlmsg = struct.pack("BBH", genl_cmd, genl_version, 0)

Should also be a separate patch

>      return nlmsg + genlmsg
>  
>  
> @@ -258,14 +278,22 @@ def _genl_load_families():
>  
>  
>  class GenlMsg:
> -    def __init__(self, nl_msg):
> +    def __init__(self, nl_msg, extra_headers = []):
>          self.nl = nl_msg
>  
>          self.hdr = nl_msg.raw[0:4]
> -        self.raw = nl_msg.raw[4:]
> +        offset = 4
>  
> -        self.genl_cmd, self.genl_version, _ = struct.unpack("bbH", self.hdr)
> +        self.genl_cmd, self.genl_version, _ = struct.unpack("BBH", self.hdr)
>  
> +        self.user_attrs = dict()
> +        for m in extra_headers:
> +            format, size = NlAttr.type_formats[m['type']]
> +            decoded = struct.unpack_from(format, nl_msg.raw, offset)
> +            offset += size
> +            self.user_attrs[m['name']] = decoded[0]


user_attrs?

> +        self.raw = nl_msg.raw[offset:]
>          self.raw_attrs = NlAttrs(self.raw)
>  
>      def __repr__(self):
> @@ -315,6 +343,7 @@ class YnlFamily(SpecFamily):
>              setattr(self, op.ident_name, bound_f)
>  
>          self.family = GenlFamily(self.yaml['name'])
> +        self._user_header = self.yaml.get('user-header', None)
>  
>      def ntf_subscribe(self, mcast_name):
>          if mcast_name not in self.family.genl_family['mcast']:
> @@ -358,7 +387,7 @@ class YnlFamily(SpecFamily):
>                  raw >>= 1
>                  i += 1
>          else:
> -            value = enum['entries'][raw - i]
> +            value = enum.entries_by_val[raw - i]['name']

Also a separate fix :S

>          rsp[attr_spec['name']] = value
>  
>      def _decode(self, attrs, space):
> @@ -381,6 +410,14 @@ class YnlFamily(SpecFamily):
>                  decoded = attr.as_bin()
>              elif attr_spec["type"] == 'flag':
>                  decoded = True
> +            elif attr_spec["type"] == 'struct':
> +                s = attr_spec['struct']
> +                decoded = attr.as_struct(self.consts[s]['members'])
> +            elif attr_spec["type"] == 'array-nest':
> +                decoded = attr.as_array(attr_spec["sub-type"])
> +            elif attr_spec["type"] == 'unused':
> +                print(f"Warning: skipping unused attribute {attr_spec['name']}")
> +                continue
>              else:
>                  raise Exception(f'Unknown {attr.type} {attr_spec["name"]} {attr_spec["type"]}')
>  
> @@ -472,6 +509,13 @@ class YnlFamily(SpecFamily):
>  
>          req_seq = random.randint(1024, 65535)
>          msg = _genl_msg(self.family.family_id, nl_flags, op.req_value, 1, req_seq)
> +        user_headers = []
> +        if self._user_header:
> +            user_headers = self.consts[self._user_header]['members']
> +            for m in user_headers:
> +                value = vals.pop(m['name'])
> +                format, _ = NlAttr.type_formats[m['type']]
> +                msg += struct.pack(format, value)
>          for name, value in vals.items():
>              msg += self._add_attr(op.attr_set.name, name, value)
>          msg = _genl_msg_finalize(msg)
> @@ -498,7 +542,7 @@ class YnlFamily(SpecFamily):
>                      done = True
>                      break
>  
> -                gm = GenlMsg(nl_msg)
> +                gm = GenlMsg(nl_msg, user_headers)
>                  # 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:
> @@ -508,7 +552,7 @@ class YnlFamily(SpecFamily):
>                          print('Unexpected message: ' + repr(gm))
>                          continue
>  
> -                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name))
> +                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name) | gm.user_attrs)
>  
>          if not rsp:
>              return None

Re: [PATCH net-next v1 2/2] netlink: specs: add partial specification for openvswitch

[Jakub Kicinski]

On Thu, 16 Mar 2023 12:01:42 +0000 Donald Hunter wrote:
> +user-header: ovs_header

Let's place this attr inside 'operations'?

also s/_/-/ everywhere, we try to use - as a separator in the spec, 
the C codegen replaces it with underscores

Re: [PATCH net-next v1 1/2] tools: ynl: add user-header and struct attr support

[Donald Hunter]

On Sat, 18 Mar 2023 at 04:50, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 16 Mar 2023 12:01:41 +0000 Donald Hunter wrote:
> > Subject: [PATCH net-next v1 1/2] tools: ynl: add user-header and struct attr support
>
> The use of "and" usually indicates it should be 2 separate patches ;)

Ack. I'll try and split it into two.

> > +  user-header:
> > +    description: Name of the struct definition for the user header for the family.
> > +    type: string
>
> Took me a minute to remember this is header as in protocol header
> not header as in C header file :) Would it possibly be better to call it
> fixed-header ? Can't really decide myself.

I went with user header because the generic netlink howto calls it the
"optional user specific message header" but happy to go with fixed-header.

> But the description definitely need to be more verbose:
>
> description: |
>   Name of the structure defining the fixed-length protocol header.
>   This header is placed in a message after the netlink and genetlink
>   headers and before any attributes.

Agreed, this is a much clearer description.

> > +    def as_array(self, type):
> > +        format, _ = self.type_formats[type]
> > +        return list({ x[0] for x in struct.iter_unpack(format, self.raw) })
>
> The Python is strong within you :)
>
> > +    def as_struct(self, members):
> > +        value = dict()
> > +        offset = 0
> > +        for m in members:
> > +            type = m['type']
>
> Accessing the spec components directly is a bit of an anti-pattern,
> can we parse the struct description into Python objects in
> tools/net/ynl/lib/nlspec.py ?

Ack, will do.

> > +            format, size = self.type_formats[type]
> > +            decoded = struct.unpack_from(format, self.raw, offset)
> > +            offset += size
> > +            value[m['name']] = decoded[0]
> > +        return value
> > +
> >      def __repr__(self):
> >          return f"[type:{self.type} len:{self._len}] {self.raw}"
> >
> > @@ -200,7 +220,7 @@ def _genl_msg(nl_type, nl_flags, genl_cmd, genl_version, seq=None):
> >      if seq is None:
> >          seq = random.randint(1, 1024)
> >      nlmsg = struct.pack("HHII", nl_type, nl_flags, seq, 0)
> > -    genlmsg = struct.pack("bbH", genl_cmd, genl_version, 0)
> > +    genlmsg = struct.pack("BBH", genl_cmd, genl_version, 0)
>
> Should also be a separate patch

Yep, I will separate this into its own patch.

> >      return nlmsg + genlmsg
> >
> >
> > @@ -258,14 +278,22 @@ def _genl_load_families():
> >
> >
> >  class GenlMsg:
> > -    def __init__(self, nl_msg):
> > +    def __init__(self, nl_msg, extra_headers = []):
> >          self.nl = nl_msg
> >
> >          self.hdr = nl_msg.raw[0:4]
> > -        self.raw = nl_msg.raw[4:]
> > +        offset = 4
> >
> > -        self.genl_cmd, self.genl_version, _ = struct.unpack("bbH", self.hdr)
> > +        self.genl_cmd, self.genl_version, _ = struct.unpack("BBH", self.hdr)
> >
> > +        self.user_attrs = dict()
> > +        for m in extra_headers:
> > +            format, size = NlAttr.type_formats[m['type']]
> > +            decoded = struct.unpack_from(format, nl_msg.raw, offset)
> > +            offset += size
> > +            self.user_attrs[m['name']] = decoded[0]
>
> user_attrs?

Um, attrs of the user-header. I'll try to name this better.

> > +        self.raw = nl_msg.raw[offset:]
> >          self.raw_attrs = NlAttrs(self.raw)
> >
> >      def __repr__(self):
> > @@ -315,6 +343,7 @@ class YnlFamily(SpecFamily):
> >              setattr(self, op.ident_name, bound_f)
> >
> >          self.family = GenlFamily(self.yaml['name'])
> > +        self._user_header = self.yaml.get('user-header', None)
> >
> >      def ntf_subscribe(self, mcast_name):
> >          if mcast_name not in self.family.genl_family['mcast']:
> > @@ -358,7 +387,7 @@ class YnlFamily(SpecFamily):
> >                  raw >>= 1
> >                  i += 1
> >          else:
> > -            value = enum['entries'][raw - i]
> > +            value = enum.entries_by_val[raw - i]['name']
>
> Also a separate fix :S

Ack, will do.

Re: [PATCH net-next v1 2/2] netlink: specs: add partial specification for openvswitch

[Donald Hunter]

On Sat, 18 Mar 2023 at 04:52, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 16 Mar 2023 12:01:42 +0000 Donald Hunter wrote:
> > +user-header: ovs_header
>
> Let's place this attr inside 'operations'?

Ah, good point - can it vary per operation and should it be a property
of each command?

> also s/_/-/ everywhere, we try to use - as a separator in the spec,
> the C codegen replaces it with underscores

Ack, will do.

Re: [PATCH net-next v1 2/2] netlink: specs: add partial specification for openvswitch

[Jakub Kicinski]

On Sat, 18 Mar 2023 16:54:35 +0000 Donald Hunter wrote:
> > On Thu, 16 Mar 2023 12:01:42 +0000 Donald Hunter wrote:  
> > > +user-header: ovs_header  
> >
> > Let's place this attr inside 'operations'?  
> 
> Ah, good point - can it vary per operation and should it be a property
> of each command?

We should allow both. Have tools/net/ynl/lib/nlspec.py expose it as 
a property of an operation, but let the spec writers only specify 
it once if each command uses the same format.