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

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

[Donald Hunter]

Add support for user headers and struct attrs to YNL. This patchset adds
features to ynl and add a partial spec for openvswitch that demonstrates
use of the features.

Patch 1 fixes a trivial signedness mismatch with struct genlmsghdr
Patch 2-5 add features to ynl
Patch 6 adds partial openvswitch specs that demonstrate the new features

v1 - v2: all requested by Jakub Kicinski
 - Split ynl changes into separate patches
 - Rename user-header to fixed-header and improve description
 - Move fixed-header to operations section of spec
 - Introduce objects to represent struct config in nlspec
 - Use kebab-case throughout openvswitch specs

Donald Hunter (6):
  tools: ynl: Fix genlmsg header encoding formats
  tools: ynl: Add struct parsing to nlspec
  tools: ynl: Add array-nest attr decoding to ynl
  tools: ynl: Add struct attr decoding to ynl
  tools: ynl: Add fixed-header support to ynl
  netlink: specs: add partial specification for openvswitch

 Documentation/netlink/genetlink-legacy.yaml   |  17 +-
 Documentation/netlink/specs/ovs_datapath.yaml | 153 ++++++++++++++++++
 Documentation/netlink/specs/ovs_vport.yaml    | 139 ++++++++++++++++
 tools/net/ynl/lib/nlspec.py                   |  72 +++++++--
 tools/net/ynl/lib/ynl.py                      |  53 +++++-
 5 files changed, 413 insertions(+), 21 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 v2 1/6] tools: ynl: Fix genlmsg header encoding formats

[Donald Hunter]

The pack strings use 'b' signed char for cmd and version but struct
genlmsghdr defines them as unsigned char. Use 'B' instead.

Fixes: 4e4480e89c47 ("tools: ynl: move the cli and netlink code around")
Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/ynl.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 90764a83c646..32536e1f9064 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -200,7 +200,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
 
 
@@ -264,7 +264,7 @@ class GenlMsg:
         self.hdr = nl_msg.raw[0:4]
         self.raw = nl_msg.raw[4:]
 
-        self.genl_cmd, self.genl_version, _ = struct.unpack("bbH", self.hdr)
+        self.genl_cmd, self.genl_version, _ = struct.unpack("BBH", self.hdr)
 
         self.raw_attrs = NlAttrs(self.raw)
 
@@ -358,7 +358,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):
-- 
2.39.0

[PATCH net-next v2 2/6] tools: ynl: Add struct parsing to nlspec

[Donald Hunter]

Add python classes for struct definitions to nlspec

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/nlspec.py | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index d04450c2a44a..5ac2dfd415c5 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -214,6 +214,43 @@ class SpecAttrSet(SpecElement):
         return self.attrs.items()
 
 
+class SpecStructMember(SpecElement):
+    """Struct member attribute
+
+    Represents a single struct member attribute.
+
+    Attributes:
+        type    string, kernel type of the member attribute
+    """
+    def __init__(self, family, yaml):
+        super().__init__(family, yaml)
+        self.type = yaml['type']
+
+class SpecStruct(SpecElement):
+    """Netlink struct type
+
+    Represents a C struct definition.
+
+    Attributes:
+        members   ordered list of struct members
+    """
+    def __init__(self, family, yaml):
+        super().__init__(family, yaml)
+
+        self.members = []
+        for member in yaml.get('members', []):
+            self.members.append(self.new_member(family, member))
+
+    def new_member(self, family, elem):
+        return SpecStructMember(family, elem)
+
+    def __iter__(self):
+        yield from self.members
+
+    def items(self):
+        return self.members.items()
+
+
 class SpecOperation(SpecElement):
     """Netlink Operation
 
@@ -344,6 +381,9 @@ class SpecFamily(SpecElement):
     def new_attr_set(self, elem):
         return SpecAttrSet(self, elem)
 
+    def new_struct(self, elem):
+        return SpecStruct(self, elem)
+
     def new_operation(self, elem, req_val, rsp_val):
         return SpecOperation(self, elem, req_val, rsp_val)
 
@@ -399,6 +439,8 @@ class SpecFamily(SpecElement):
         for elem in definitions:
             if elem['type'] == 'enum' or elem['type'] == 'flags':
                 self.consts[elem['name']] = self.new_enum(elem)
+            elif elem['type'] == 'struct':
+                self.consts[elem['name']] = self.new_struct(elem)
             else:
                 self.consts[elem['name']] = elem
 
-- 
2.39.0

Re: [PATCH net-next v2 2/6] tools: ynl: Add struct parsing to nlspec

[Jakub Kicinski]

On Sun, 19 Mar 2023 19:37:59 +0000 Donald Hunter wrote:
> +class SpecStructMember(SpecElement):
> +    """Struct member attribute
> +
> +    Represents a single struct member attribute.
> +
> +    Attributes:
> +        type    string, kernel type of the member attribute

We can have structs inside structs in theory, or "binary blobs" so this
is really a subset of what attr can be rather than necessarily a kernel
type?

> +    """
> +    def __init__(self, family, yaml):
> +        super().__init__(family, yaml)
> +        self.type = yaml['type']
> +

nit: double new line

> +class SpecStruct(SpecElement):
> +    """Netlink struct type

Re: [PATCH net-next v2 2/6] tools: ynl: Add struct parsing to nlspec

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Sun, 19 Mar 2023 19:37:59 +0000 Donald Hunter wrote:
>> +class SpecStructMember(SpecElement):
>> +    """Struct member attribute
>> +
>> +    Represents a single struct member attribute.
>> +
>> +    Attributes:
>> +        type    string, kernel type of the member attribute
>
> We can have structs inside structs in theory, or "binary blobs" so this
> is really a subset of what attr can be rather than necessarily a kernel
> type?

Okay, so the schema currently defines the member types as u*, s* and
string. Does it make sense to add 'binary' and 'struct'?

To be clear, do you want me to drop the word 'kernel' from the
docstring, or something more?

>> +    """
>> +    def __init__(self, family, yaml):
>> +        super().__init__(family, yaml)
>> +        self.type = yaml['type']
>> +
>
> nit: double new line

Ack.

Re: [PATCH net-next v2 2/6] tools: ynl: Add struct parsing to nlspec

[Jakub Kicinski]

On Wed, 22 Mar 2023 11:38:01 +0000 Donald Hunter wrote:
> >> +    Attributes:
> >> +        type    string, kernel type of the member attribute  
> >
> > We can have structs inside structs in theory, or "binary blobs" so this
> > is really a subset of what attr can be rather than necessarily a kernel
> > type?  
> 
> Okay, so the schema currently defines the member types as u*, s* and
> string. Does it make sense to add 'binary' and 'struct'?

We don't have to until it's needed.

> To be clear, do you want me to drop the word 'kernel' from the
> docstring, or something more?

Removing kernel should be good enough

[PATCH net-next v2 3/6] tools: ynl: Add array-nest attr decoding to ynl

[Donald Hunter]

Add support for decoding nested arrays of scalars in netlink messages.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/ynl.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 32536e1f9064..077ba9e8dc98 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -93,6 +93,10 @@ 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 __repr__(self):
         return f"[type:{self.type} len:{self._len}] {self.raw}"
 
@@ -381,6 +385,8 @@ class YnlFamily(SpecFamily):
                 decoded = attr.as_bin()
             elif attr_spec["type"] == 'flag':
                 decoded = True
+            elif attr_spec["type"] == 'array-nest':
+                decoded = attr.as_array(attr_spec["sub-type"])
             else:
                 raise Exception(f'Unknown {attr.type} {attr_spec["name"]} {attr_spec["type"]}')
 
-- 
2.39.0

Re: [PATCH net-next v2 3/6] tools: ynl: Add array-nest attr decoding to ynl

[Jakub Kicinski]

On Sun, 19 Mar 2023 19:38:00 +0000 Donald Hunter wrote:
> Add support for decoding nested arrays of scalars in netlink messages.

example?

> Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
> ---
>  tools/net/ynl/lib/ynl.py | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 32536e1f9064..077ba9e8dc98 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -93,6 +93,10 @@ 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) })

So in terms of C this treats the payload of the attr as a packed array?
That's not what array-nest is, array-nest wraps every entry in another
nlattr:
https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#array-nest

It's not a C array dumped into an attribute.

IIRC I was intending to use 'binary' for packed arrays. Still use
sub-type to carry the type, but main type should be 'binary'.

If that sounds reasonable could you document or remind me to document
this as the expected behavior? Sub-type appears completely undocumented
now :S

>      def __repr__(self):
>          return f"[type:{self.type} len:{self._len}] {self.raw}"
>  
> @@ -381,6 +385,8 @@ class YnlFamily(SpecFamily):
>                  decoded = attr.as_bin()
>              elif attr_spec["type"] == 'flag':
>                  decoded = True
> +            elif attr_spec["type"] == 'array-nest':
> +                decoded = attr.as_array(attr_spec["sub-type"])
>              else:
>                  raise Exception(f'Unknown {attr.type} {attr_spec["name"]} {attr_spec["type"]}')
>  

Re: [PATCH net-next v2 3/6] tools: ynl: Add array-nest attr decoding to ynl

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Sun, 19 Mar 2023 19:38:00 +0000 Donald Hunter wrote:
>> Add support for decoding nested arrays of scalars in netlink messages.
>
> example?

OVS_VPORT_ATTR_UPCALL_PID is a C array of u32 values. I can provide that
as an example in the commit message.

>> Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
>> ---
>>  tools/net/ynl/lib/ynl.py | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
>> index 32536e1f9064..077ba9e8dc98 100644
>> --- a/tools/net/ynl/lib/ynl.py
>> +++ b/tools/net/ynl/lib/ynl.py
>> @@ -93,6 +93,10 @@ 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) })
>
> So in terms of C this treats the payload of the attr as a packed array?
> That's not what array-nest is, array-nest wraps every entry in another
> nlattr:
> https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#array-nest
>
> It's not a C array dumped into an attribute.
>
> IIRC I was intending to use 'binary' for packed arrays. Still use
> sub-type to carry the type, but main type should be 'binary'.
>
> If that sounds reasonable could you document or remind me to document
> this as the expected behavior? Sub-type appears completely undocumented
> now :S

That sounds reasonable, yes. I will also rename the method to
'as_c_array'. I think it should just be restricted to scalar subtypes,
i.e. u16, u32, etc. Do you agree?

I will update the  documentation for this.

Re: [PATCH net-next v2 3/6] tools: ynl: Add array-nest attr decoding to ynl

[Jakub Kicinski]

On Wed, 22 Mar 2023 11:27:25 +0000 Donald Hunter wrote:
> > So in terms of C this treats the payload of the attr as a packed array?
> > That's not what array-nest is, array-nest wraps every entry in another
> > nlattr:
> > https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#array-nest
> >
> > It's not a C array dumped into an attribute.
> >
> > IIRC I was intending to use 'binary' for packed arrays. Still use
> > sub-type to carry the type, but main type should be 'binary'.
> >
> > If that sounds reasonable could you document or remind me to document
> > this as the expected behavior? Sub-type appears completely undocumented
> > now :S  
> 
> That sounds reasonable, yes. I will also rename the method to
> 'as_c_array'. I think it should just be restricted to scalar subtypes,
> i.e. u16, u32, etc. Do you agree?

We can limit it to scalars for now. There are some arrays of structs
(from memory TC GRED had VCs defined as array of structs?) but that
should hopefully be rare and can be added later.

[PATCH net-next v2 4/6] tools: ynl: Add struct attr decoding to ynl

[Donald Hunter]

Add support for decoding attributes that contain C structs.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/genetlink-legacy.yaml |  7 ++++++-
 tools/net/ynl/lib/ynl.py                    | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index 5dc6f1c07a97..654f40b26beb 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -172,7 +172,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 +218,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 077ba9e8dc98..24f8af3c2b38 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
@@ -97,6 +102,16 @@ class NlAttr:
         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:
+            format, size = self.type_formats[m.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}"
 
@@ -385,6 +400,9 @@ 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])
             elif attr_spec["type"] == 'array-nest':
                 decoded = attr.as_array(attr_spec["sub-type"])
             else:
-- 
2.39.0

Re: [PATCH net-next v2 4/6] tools: ynl: Add struct attr decoding to ynl

[Jakub Kicinski]

On Sun, 19 Mar 2023 19:38:01 +0000 Donald Hunter wrote:
>                  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 ]

I wonder if we should also only allow struct as a subtype of binary?

Structs can technically grow with newer kernels (i.e. new members can
be added at the end). So I think for languages like C we will still
need to expose to the user the original length of the attribute.
And binary comes with a length so codgen reuse fits nicely.

Either way - docs need to be updated.

Re: [PATCH net-next v2 4/6] tools: ynl: Add struct attr decoding to ynl

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Sun, 19 Mar 2023 19:38:01 +0000 Donald Hunter wrote:
>>                  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 ]
>
> I wonder if we should also only allow struct as a subtype of binary?
>
> Structs can technically grow with newer kernels (i.e. new members can
> be added at the end). So I think for languages like C we will still
> need to expose to the user the original length of the attribute.
> And binary comes with a length so codgen reuse fits nicely.
>
> Either way - docs need to be updated.

Yep, as I was replying to your previous comment, I started to think
about making struct a subtype of binary. That would make a struct attr
something like:

 -
   name: stats
   type: binary
   sub-type: struct
   struct: vport-stats

I originally chose 'struct' as the attr name, following the pattern that
'enum' is used for enum names but I'm not sure it's clear enough. Maybe
'sub-type-name' would be better?

I will update the documentation for this.

Re: [PATCH net-next v2 4/6] tools: ynl: Add struct attr decoding to ynl

[Jakub Kicinski]

On Wed, 22 Mar 2023 11:48:12 +0000 Donald Hunter wrote:
> > On Sun, 19 Mar 2023 19:38:01 +0000 Donald Hunter wrote:  
> >>                  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 ]  
> >
> > I wonder if we should also only allow struct as a subtype of binary?
> >
> > Structs can technically grow with newer kernels (i.e. new members can
> > be added at the end). So I think for languages like C we will still
> > need to expose to the user the original length of the attribute.
> > And binary comes with a length so codgen reuse fits nicely.
> >
> > Either way - docs need to be updated.  
> 
> Yep, as I was replying to your previous comment, I started to think
> about making struct a subtype of binary. That would make a struct attr
> something like:
> 
>  -
>    name: stats
>    type: binary
>    sub-type: struct
>    struct: vport-stats

LGTM!

> I originally chose 'struct' as the attr name, following the pattern that
> 'enum' is used for enum names but I'm not sure it's clear enough. Maybe
> 'sub-type-name' would be better?

Agreed, using the sub-type's value as name of another attr 
is mixing keys and values.

But sub-type-name would then also be used for enums (I mean in 
normal type: u32 enums, not binary arrays)?
enums don't have a sub-type so there we'd have sub-type-name
and no sub-type.
Plus for binary arrays of enums we'd have:

  -
    name: stats
    type: binary
    sub-type: u32
    sub-type-name: vport-stats

Doesn't say enum anywhere :S  We'd need to assume if sub-type is
a scalar the sub-type-name is an enum?

Maybe to avoid saying struct twice we should go the enum way and
actually ditch the sub-type for structs? Presence of struct: abc
implies it's a struct, only use sub-type for scalar types?

  -
    name: stats
    type: binary
    struct: vport-stats

  -
    name: another
    type: binary
    sub-type: u32
    enum: enums-name

Re: [PATCH net-next v2 4/6] tools: ynl: Add struct attr decoding to ynl

[Donald Hunter]

On Wed, 22 Mar 2023 at 18:38, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Maybe to avoid saying struct twice we should go the enum way and
> actually ditch the sub-type for structs? Presence of struct: abc
> implies it's a struct, only use sub-type for scalar types?
>
>   -
>     name: stats
>     type: binary
>     struct: vport-stats

Yep, this looks good. I'll add this to the docs too.

>   -
>     name: another
>     type: binary
>     sub-type: u32
>     enum: enums-name

[PATCH net-next v2 5/6] tools: ynl: Add fixed-header support to ynl

[Donald Hunter]

Add support for netlink families that add an optional fixed header structure
after the genetlink header and before any attributes. The fixed-header can be
specified on a per op basis, or once for all operations.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/genetlink-legacy.yaml | 10 +++++++
 tools/net/ynl/lib/nlspec.py                 | 30 ++++++++++++---------
 tools/net/ynl/lib/ynl.py                    | 23 +++++++++++++---
 3 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index 654f40b26beb..30a8051c1d8f 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -261,6 +261,13 @@ properties:
       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 defininig 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
@@ -293,6 +300,9 @@ properties:
               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
diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index 5ac2dfd415c5..69ee9f940e0e 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -257,18 +257,19 @@ class SpecOperation(SpecElement):
     Information about a single Netlink operation.
 
     Attributes:
-        value       numerical ID when serialized, None if req/rsp values differ
+        value           numerical ID when serialized, None if req/rsp values differ
 
-        req_value   numerical ID when serialized, user -> kernel
-        rsp_value   numerical ID when serialized, user <- kernel
-        is_call     bool, whether the operation is a call
-        is_async    bool, whether the operation is a notification
-        is_resv     bool, whether the operation does not exist (it's just a reserved ID)
-        attr_set    attribute set name
+        req_value       numerical ID when serialized, user -> kernel
+        rsp_value       numerical ID when serialized, user <- kernel
+        is_call         bool, whether the operation is a call
+        is_async        bool, whether the operation is a notification
+        is_resv         bool, whether the operation does not exist (it's just a reserved ID)
+        attr_set        attribute set name
+        fixed_header    string, optional fixed header structure name
 
-        yaml        raw spec as loaded from the spec file
+        yaml            raw spec as loaded from the spec file
     """
-    def __init__(self, family, yaml, req_value, rsp_value):
+    def __init__(self, family, yaml, req_value, rsp_value, default_fixed_header):
         super().__init__(family, yaml)
 
         self.value = req_value if req_value == rsp_value else None
@@ -278,6 +279,7 @@ class SpecOperation(SpecElement):
         self.is_call = 'do' in yaml or 'dump' in yaml
         self.is_async = 'notify' in yaml or 'event' in yaml
         self.is_resv = not self.is_async and not self.is_call
+        self.fixed_header = self.yaml.get('fixed-header', default_fixed_header)
 
         # Added by resolve:
         self.attr_set = None
@@ -384,24 +386,26 @@ class SpecFamily(SpecElement):
     def new_struct(self, elem):
         return SpecStruct(self, elem)
 
-    def new_operation(self, elem, req_val, rsp_val):
-        return SpecOperation(self, elem, req_val, rsp_val)
+    def new_operation(self, elem, req_val, rsp_val, default_fixed_header):
+        return SpecOperation(self, elem, req_val, rsp_val, default_fixed_header)
 
     def add_unresolved(self, elem):
         self._resolution_list.append(elem)
 
     def _dictify_ops_unified(self):
+        default_fixed_header = self.yaml['operations'].get('fixed-header')
         val = 1
         for elem in self.yaml['operations']['list']:
             if 'value' in elem:
                 val = elem['value']
 
-            op = self.new_operation(elem, val, val)
+            op = self.new_operation(elem, val, val, default_fixed_header)
             val += 1
 
             self.msgs[op.name] = op
 
     def _dictify_ops_directional(self):
+        default_fixed_header = self.yaml['operations'].get('fixed-header')
         req_val = rsp_val = 1
         for elem in self.yaml['operations']['list']:
             if 'notify' in elem:
@@ -426,7 +430,7 @@ class SpecFamily(SpecElement):
             else:
                 raise Exception("Can't parse directional ops")
 
-            op = self.new_operation(elem, req_val, rsp_val)
+            op = self.new_operation(elem, req_val, rsp_val, default_fixed_header)
             req_val = req_val_next
             rsp_val = rsp_val_next
 
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 24f8af3c2b38..736a637ecb2b 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -277,14 +277,22 @@ def _genl_load_families():
 
 
 class GenlMsg:
-    def __init__(self, nl_msg):
+    def __init__(self, nl_msg, fixed_header_members = []):
         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.fixed_header_attrs = dict()
+        for m in fixed_header_members:
+            format, size = NlAttr.type_formats[m.type]
+            decoded = struct.unpack_from(format, nl_msg.raw, offset)
+            offset += size
+            self.fixed_header_attrs[m.name] = decoded[0]
+
+        self.raw = nl_msg.raw[offset:]
         self.raw_attrs = NlAttrs(self.raw)
 
     def __repr__(self):
@@ -496,6 +504,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)
+        fixed_header_members = []
+        if op.fixed_header:
+            fixed_header_members = self.consts[op.fixed_header].members
+            for m in fixed_header_members:
+                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)
@@ -522,7 +537,7 @@ class YnlFamily(SpecFamily):
                     done = True
                     break
 
-                gm = GenlMsg(nl_msg)
+                gm = GenlMsg(nl_msg, fixed_header_members)
                 # 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:
@@ -532,7 +547,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.fixed_header_attrs)
 
         if not rsp:
             return None
-- 
2.39.0

Re: [PATCH net-next v2 5/6] tools: ynl: Add fixed-header support to ynl

[Jakub Kicinski]

On Sun, 19 Mar 2023 19:38:02 +0000 Donald Hunter wrote:
> -    def __init__(self, family, yaml, req_value, rsp_value):
> +    def __init__(self, family, yaml, req_value, rsp_value, default_fixed_header):
>          super().__init__(family, yaml)
>  
>          self.value = req_value if req_value == rsp_value else None
> @@ -278,6 +279,7 @@ class SpecOperation(SpecElement):
>          self.is_call = 'do' in yaml or 'dump' in yaml
>          self.is_async = 'notify' in yaml or 'event' in yaml
>          self.is_resv = not self.is_async and not self.is_call
> +        self.fixed_header = self.yaml.get('fixed-header', default_fixed_header)
>  
>          # Added by resolve:
>          self.attr_set = None
> @@ -384,24 +386,26 @@ class SpecFamily(SpecElement):
>      def new_struct(self, elem):
>          return SpecStruct(self, elem)
>  
> -    def new_operation(self, elem, req_val, rsp_val):
> -        return SpecOperation(self, elem, req_val, rsp_val)
> +    def new_operation(self, elem, req_val, rsp_val, default_fixed_header):
> +        return SpecOperation(self, elem, req_val, rsp_val, default_fixed_header)
>  
>      def add_unresolved(self, elem):
>          self._resolution_list.append(elem)
>  
>      def _dictify_ops_unified(self):
> +        default_fixed_header = self.yaml['operations'].get('fixed-header')
>          val = 1
>          for elem in self.yaml['operations']['list']:
>              if 'value' in elem:
>                  val = elem['value']
>  
> -            op = self.new_operation(elem, val, val)
> +            op = self.new_operation(elem, val, val, default_fixed_header)
>              val += 1
>  
>              self.msgs[op.name] = op
>  
>      def _dictify_ops_directional(self):
> +        default_fixed_header = self.yaml['operations'].get('fixed-header')
>          req_val = rsp_val = 1
>          for elem in self.yaml['operations']['list']:
>              if 'notify' in elem:
> @@ -426,7 +430,7 @@ class SpecFamily(SpecElement):
>              else:
>                  raise Exception("Can't parse directional ops")
>  
> -            op = self.new_operation(elem, req_val, rsp_val)
> +            op = self.new_operation(elem, req_val, rsp_val, default_fixed_header)

Can we record the "family-default" fixed header in the family and read
from there rather than passing the arg around?

Also - doc - to be clear - by documentation I mean in the right places
under Documentation/user-api/netlink/

Re: [PATCH net-next v2 5/6] tools: ynl: Add fixed-header support to ynl

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Sun, 19 Mar 2023 19:38:02 +0000 Donald Hunter wrote:
>>
>>      def _dictify_ops_directional(self):
>> +        default_fixed_header = self.yaml['operations'].get('fixed-header')
>>          req_val = rsp_val = 1
>>          for elem in self.yaml['operations']['list']:
>>              if 'notify' in elem:
>> @@ -426,7 +430,7 @@ class SpecFamily(SpecElement):
>>              else:
>>                  raise Exception("Can't parse directional ops")
>>  
>> -            op = self.new_operation(elem, req_val, rsp_val)
>> +            op = self.new_operation(elem, req_val, rsp_val, default_fixed_header)
>
> Can we record the "family-default" fixed header in the family and read
> from there rather than passing the arg around?

Yes, will do.

> Also - doc - to be clear - by documentation I mean in the right places
> under Documentation/user-api/netlink/

Ack.

[PATCH net-next v2 6/6] netlink: specs: add partial specification for openvswitch

[Donald Hunter]

The openvswitch family has a fixed header, uses struct attrs and has array
values. This partial spec 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': 48,
  '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 | 153 ++++++++++++++++++
 Documentation/netlink/specs/ovs_vport.yaml    | 139 ++++++++++++++++
 2 files changed, 292 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..2eefdf9d8cd7
--- /dev/null
+++ b/Documentation/netlink/specs/ovs_datapath.yaml
@@ -0,0 +1,153 @@
+# 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
+
+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:
+  fixed-header: ovs-header
+  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..a0d01adcb435
--- /dev/null
+++ b/Documentation/netlink/specs/ovs_vport.yaml
@@ -0,0 +1,139 @@
+# 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
+
+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
+      fixed-header: ovs-header
+      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 v2 0/6] ynl: add support for user headers and struct attrs

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sun, 19 Mar 2023 19:37:57 +0000 you wrote:
> Add support for user headers and struct attrs to YNL. This patchset adds
> features to ynl and add a partial spec for openvswitch that demonstrates
> use of the features.
> 
> Patch 1 fixes a trivial signedness mismatch with struct genlmsghdr
> Patch 2-5 add features to ynl
> Patch 6 adds partial openvswitch specs that demonstrate the new features
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/6] tools: ynl: Fix genlmsg header encoding formats
    https://git.kernel.org/netdev/net/c/758d29fb3a8b
  - [net-next,v2,2/6] tools: ynl: Add struct parsing to nlspec
    (no matching commit)
  - [net-next,v2,3/6] tools: ynl: Add array-nest attr decoding to ynl
    (no matching commit)
  - [net-next,v2,4/6] tools: ynl: Add struct attr decoding to ynl
    (no matching commit)
  - [net-next,v2,5/6] tools: ynl: Add fixed-header support to ynl
    (no matching commit)
  - [net-next,v2,6/6] netlink: specs: add partial specification for openvswitch
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html