[PATCH net 0/3] tools: ynl: fix subset use and change default value for attrs/ops

[PATCH net 0/3] tools: ynl: fix subset use and change default value for attrs/ops

[Jakub Kicinski]

Fix a problem in subsetting, which will become apparent when
the devlink family comes after the merge window. Even tho none
of the existing families need this, we don't want someone to
get "inspired" by the current, incorrect code when using specs
in other languages.

Change the default value for the first attr/op. This is a slight
behavior change so needs to go in now. The diffstat of the last
patch should serve as the clearest justification there..

Jakub Kicinski (3):
  tools: ynl: fully inherit attrs in subsets
  tools: ynl: use 1 as the default for first entry in attrs/ops
  netlink: specs: update for codegen enumerating from 1

 Documentation/netlink/specs/ethtool.yaml      | 15 -----------
 Documentation/netlink/specs/fou.yaml          |  2 ++
 Documentation/netlink/specs/netdev.yaml       |  2 --
 Documentation/userspace-api/netlink/specs.rst | 10 +++++--
 tools/net/ynl/lib/nlspec.py                   | 27 ++++++++++++-------
 tools/net/ynl/ynl-gen-c.py                    |  7 +++--
 6 files changed, 32 insertions(+), 31 deletions(-)

-- 
2.39.2

[PATCH net 1/3] tools: ynl: fully inherit attrs in subsets

[Jakub Kicinski]

To avoid having to repeat the entire definition of an attribute
(including the value) use the Attr object from the original set.
In fact this is already the documented expectation.

Fixes: be5bea1cc0bf ("net: add basic C code generators for Netlink")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: corbet@lwn.net
CC: sdf@google.com
CC: linux-doc@vger.kernel.org
---
 Documentation/userspace-api/netlink/specs.rst |  3 ++-
 tools/net/ynl/lib/nlspec.py                   | 23 ++++++++++++-------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/Documentation/userspace-api/netlink/specs.rst b/Documentation/userspace-api/netlink/specs.rst
index 6ffe8137cd90..1424ab1b9b33 100644
--- a/Documentation/userspace-api/netlink/specs.rst
+++ b/Documentation/userspace-api/netlink/specs.rst
@@ -199,7 +199,8 @@ The ``value`` property can be skipped, in which case the attribute ID
 will be the value of the previous attribute plus one (recursively)
 and ``0`` for the first attribute in the attribute set.
 
-Note that the ``value`` of an attribute is defined only in its main set.
+Note that the ``value`` of an attribute is defined only in its main set
+(not in subsets).
 
 enum
 ~~~~
diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index 71da568e2c28..dff31dad36c5 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -95,15 +95,22 @@ jsonschema = None
         self.attrs = collections.OrderedDict()
         self.attrs_by_val = collections.OrderedDict()
 
-        val = 0
-        for elem in self.yaml['attributes']:
-            if 'value' in elem:
-                val = elem['value']
+        if self.subset_of is None:
+            val = 0
+            for elem in self.yaml['attributes']:
+                if 'value' in elem:
+                    val = elem['value']
 
-            attr = self.new_attr(elem, val)
-            self.attrs[attr.name] = attr
-            self.attrs_by_val[attr.value] = attr
-            val += 1
+                attr = self.new_attr(elem, val)
+                self.attrs[attr.name] = attr
+                self.attrs_by_val[attr.value] = attr
+                val += 1
+        else:
+            real_set = family.attr_sets[self.subset_of]
+            for elem in self.yaml['attributes']:
+                attr = real_set[elem['name']]
+                self.attrs[attr.name] = attr
+                self.attrs_by_val[attr.value] = attr
 
     def new_attr(self, elem, value):
         return SpecAttr(self.family, self, elem, value)
-- 
2.39.2

[PATCH net 2/3] tools: ynl: use 1 as the default for first entry in attrs/ops

[Jakub Kicinski]

Pretty much all families use value: 1 or reserve as unspec
the first entry in attribute set and the first operation.
Make this the default. Update documentation (the doc for
values of operations just refers back to doc for attrs
so updating only attrs).

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/userspace-api/netlink/specs.rst | 7 ++++++-
 tools/net/ynl/lib/nlspec.py                   | 6 +++---
 tools/net/ynl/ynl-gen-c.py                    | 7 +++++--
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/userspace-api/netlink/specs.rst b/Documentation/userspace-api/netlink/specs.rst
index 1424ab1b9b33..32e53328d113 100644
--- a/Documentation/userspace-api/netlink/specs.rst
+++ b/Documentation/userspace-api/netlink/specs.rst
@@ -197,7 +197,12 @@ value
 Numerical attribute ID, used in serialized Netlink messages.
 The ``value`` property can be skipped, in which case the attribute ID
 will be the value of the previous attribute plus one (recursively)
-and ``0`` for the first attribute in the attribute set.
+and ``1`` for the first attribute in the attribute set.
+
+Attributes (and operations) use ``1`` as the default value for the first
+entry (unlike enums in definitions which start from ``0``) because
+entry ``0`` is almost always reserved as undefined. Spec can explicitly
+set value to ``0`` if needed.
 
 Note that the ``value`` of an attribute is defined only in its main set
 (not in subsets).
diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index dff31dad36c5..9d394e50de23 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -96,7 +96,7 @@ jsonschema = None
         self.attrs_by_val = collections.OrderedDict()
 
         if self.subset_of is None:
-            val = 0
+            val = 1
             for elem in self.yaml['attributes']:
                 if 'value' in elem:
                     val = elem['value']
@@ -252,7 +252,7 @@ jsonschema = None
         self._resolution_list.append(elem)
 
     def _dictify_ops_unified(self):
-        val = 0
+        val = 1
         for elem in self.yaml['operations']['list']:
             if 'value' in elem:
                 val = elem['value']
@@ -263,7 +263,7 @@ jsonschema = None
             self.msgs[op.name] = op
 
     def _dictify_ops_directional(self):
-        req_val = rsp_val = 0
+        req_val = rsp_val = 1
         for elem in self.yaml['operations']['list']:
             if 'notify' in elem:
                 if 'value' in elem:
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 274e9c566f61..62f8f2c3c56c 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -2044,14 +2044,17 @@ _C_KW = {
     max_value = f"({cnt_name} - 1)"
 
     uapi_enum_start(family, cw, family['operations'], 'enum-name')
+    val = 0
     for op in family.msgs.values():
         if separate_ntf and ('notify' in op or 'event' in op):
             continue
 
         suffix = ','
-        if 'value' in op:
-            suffix = f" = {op['value']},"
+        if op.value != val:
+            suffix = f" = {op.value},"
+            val = op.value
         cw.p(op.enum_name + suffix)
+        val += 1
     cw.nl()
     cw.p(cnt_name + ('' if max_by_define else ','))
     if not max_by_define:
-- 
2.39.2

[PATCH net 3/3] netlink: specs: update for codegen enumerating from 1

[Jakub Kicinski]

Now that the codegen rules had been changed we can update
the specs to reflect the new default.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/ethtool.yaml | 15 ---------------
 Documentation/netlink/specs/fou.yaml     |  2 ++
 Documentation/netlink/specs/netdev.yaml  |  2 --
 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 08b776908d15..35c462bce56f 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -11,7 +11,6 @@ doc: Partial family for Ethtool Netlink.
       -
         name: dev-index
         type: u32
-        value: 1
       -
         name: dev-name
         type: string
@@ -25,7 +24,6 @@ doc: Partial family for Ethtool Netlink.
       -
         name: index
         type: u32
-        value: 1
       -
         name: name
         type: string
@@ -39,14 +37,12 @@ doc: Partial family for Ethtool Netlink.
         name: bit
         type: nest
         nested-attributes: bitset-bit
-        value: 1
   -
     name: bitset
     attributes:
       -
         name: nomask
         type: flag
-        value: 1
       -
         name: size
         type: u32
@@ -61,7 +57,6 @@ doc: Partial family for Ethtool Netlink.
       -
         name: index
         type: u32
-        value: 1
       -
         name: value
         type: string
@@ -71,7 +66,6 @@ doc: Partial family for Ethtool Netlink.
       -
         name: string
         type: nest
-        value: 1
         multi-attr: true
         nested-attributes: string
   -
@@ -80,7 +74,6 @@ doc: Partial family for Ethtool Netlink.
       -
         name: id
         type: u32
-        value: 1
       -
         name: count
         type: u32
@@ -96,14 +89,12 @@ doc: Partial family for Ethtool Netlink.
         name: stringset
         type: nest
         multi-attr: true
-        value: 1
         nested-attributes: stringset
   -
     name: strset
     attributes:
       -
         name: header
-        value: 1
         type: nest
         nested-attributes: header
       -
@@ -119,7 +110,6 @@ doc: Partial family for Ethtool Netlink.
     attributes:
       -
         name: header
-        value: 1
         type: nest
         nested-attributes: header
       -
@@ -132,7 +122,6 @@ doc: Partial family for Ethtool Netlink.
     attributes:
       -
         name: header
-        value: 1
         type: nest
         nested-attributes: header
       -
@@ -180,7 +169,6 @@ doc: Partial family for Ethtool Netlink.
     attributes:
       -
         name: pad
-        value: 1
         type: pad
       -
         name: reassembly-errors
@@ -205,7 +193,6 @@ doc: Partial family for Ethtool Netlink.
     attributes:
       -
         name: header
-        value: 1
         type: nest
         nested-attributes: header
       -
@@ -251,13 +238,11 @@ doc: Partial family for Ethtool Netlink.
 
       do: &strset-get-op
         request:
-          value: 1
           attributes:
             - header
             - stringsets
             - counts-only
         reply:
-          value: 1
           attributes:
             - header
             - stringsets
diff --git a/Documentation/netlink/specs/fou.yaml b/Documentation/netlink/specs/fou.yaml
index 266c386eedf3..cca4cf98f03a 100644
--- a/Documentation/netlink/specs/fou.yaml
+++ b/Documentation/netlink/specs/fou.yaml
@@ -26,6 +26,7 @@ kernel-policy: global
       -
         name: unspec
         type: unused
+        value: 0
       -
         name: port
         type: u16
@@ -71,6 +72,7 @@ kernel-policy: global
     -
       name: unspec
       doc: unused
+      value: 0
 
     -
       name: add
diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index cffef09729f1..ba9ee13cf729 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -48,7 +48,6 @@ name: netdev
         name: ifindex
         doc: netdev ifindex
         type: u32
-        value: 1
         checks:
           min: 1
       -
@@ -66,7 +65,6 @@ name: netdev
     -
       name: dev-get
       doc: Get / dump information about a netdev.
-      value: 1
       attribute-set: dev
       do:
         request:
-- 
2.39.2