[patch net-next v2 0/3] tools: ynl-gen: fix subset attributes handling

[patch net-next v2 0/3] tools: ynl-gen: fix subset attributes handling

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Attributes defined in an attribute subset should just specify a list
of attributes of the original attribute set. The original attribute
definitions should be used. This patchset ensures that.

Jiri Pirko (3):
  tools: ynl-gen: lift type requirement for attribute subsets
  netlink: specs: remove redundant type keys from attributes in subsets
  tools: ynl-gen: raise exception when subset attribute contains more
    than "name" key

 Documentation/netlink/genetlink-c.yaml      |  2 +-
 Documentation/netlink/genetlink-legacy.yaml |  2 +-
 Documentation/netlink/genetlink.yaml        |  2 +-
 Documentation/netlink/netlink-raw.yaml      |  2 +-
 Documentation/netlink/specs/devlink.yaml    | 10 ----------
 Documentation/netlink/specs/dpll.yaml       |  8 --------
 Documentation/netlink/specs/ethtool.yaml    |  3 ---
 tools/net/ynl/lib/nlspec.py                 |  2 ++
 tools/net/ynl/ynl-gen-c.py                  |  2 ++
 9 files changed, 8 insertions(+), 25 deletions(-)

-- 
2.41.0

[patch net-next v2 1/3] tools: ynl-gen: lift type requirement for attribute subsets

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In case an attribute is used in a subset, the type has to be currently
specified. As the attribute is already defined in the original set, this
is a redundant information in yaml file, moreover, may lead to
inconsistencies.

Example:
attribute-sets:
    ...
    name: pin
    enum-name: dpll_a_pin
    attributes:
      ...
      -
        name: parent-id
        type: u32
      ...
  -
    name: pin-parent-device
    subset-of: pin
    attributes:
      -
        name: parent-id
        type: u32             <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

Remove the requirement from schema files to specify the "type" and add
check and bail out if "type" is not set.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 Documentation/netlink/genetlink-c.yaml      | 2 +-
 Documentation/netlink/genetlink-legacy.yaml | 2 +-
 Documentation/netlink/genetlink.yaml        | 2 +-
 Documentation/netlink/netlink-raw.yaml      | 2 +-
 tools/net/ynl/ynl-gen-c.py                  | 2 ++
 5 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml
index 9806c44f604c..80d8aa2708c5 100644
--- a/Documentation/netlink/genetlink-c.yaml
+++ b/Documentation/netlink/genetlink-c.yaml
@@ -142,7 +142,7 @@ properties:
           type: array
           items:
             type: object
-            required: [ name, type ]
+            required: [ name ]
             additionalProperties: False
             properties:
               name:
diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index 12a0a045605d..2a21aae525a4 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -180,7 +180,7 @@ properties:
           type: array
           items:
             type: object
-            required: [ name, type ]
+            required: [ name ]
             additionalProperties: False
             properties:
               name:
diff --git a/Documentation/netlink/genetlink.yaml b/Documentation/netlink/genetlink.yaml
index 3d338c48bf21..9e8354f80466 100644
--- a/Documentation/netlink/genetlink.yaml
+++ b/Documentation/netlink/genetlink.yaml
@@ -115,7 +115,7 @@ properties:
           type: array
           items:
             type: object
-            required: [ name, type ]
+            required: [ name ]
             additionalProperties: False
             properties:
               name:
diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml
index 896797876414..9aeb64b27ada 100644
--- a/Documentation/netlink/netlink-raw.yaml
+++ b/Documentation/netlink/netlink-raw.yaml
@@ -187,7 +187,7 @@ properties:
           type: array
           items:
             type: object
-            required: [ name, type ]
+            required: [ name ]
             additionalProperties: False
             properties:
               name:
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 897af958cee8..b378412a9d7a 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -723,6 +723,8 @@ class AttrSet(SpecAttrSet):
             self.c_name = ''
 
     def new_attr(self, elem, value):
+        if 'type' not in elem:
+            raise Exception(f"Type has to be set for attribute {elem['name']}")
         if elem['type'] in scalars:
             t = TypeScalar(self.family, self, elem, value)
         elif elem['type'] == 'unused':
-- 
2.41.0

Re: [patch net-next v2 1/3] tools: ynl-gen: lift type requirement for attribute subsets

[Jakub Kicinski]

On Fri, 29 Sep 2023 15:47:40 +0200 Jiri Pirko wrote:
> --- a/tools/net/ynl/ynl-gen-c.py
> +++ b/tools/net/ynl/ynl-gen-c.py
> @@ -723,6 +723,8 @@ class AttrSet(SpecAttrSet):
>              self.c_name = ''
>  
>      def new_attr(self, elem, value):
> +        if 'type' not in elem:
> +            raise Exception(f"Type has to be set for attribute {elem['name']}")
>          if elem['type'] in scalars:
>              t = TypeScalar(self.family, self, elem, value)
>          elif elem['type'] == 'unused':

Can this still be enforced using JSON schema? Using dependencies 
to make sure that if subset-of is not present type is?

Re: [patch net-next v2 1/3] tools: ynl-gen: lift type requirement for attribute subsets

[Jiri Pirko]

Thu, Oct 05, 2023 at 02:12:02AM CEST, kuba@kernel.org wrote:
>On Fri, 29 Sep 2023 15:47:40 +0200 Jiri Pirko wrote:
>> --- a/tools/net/ynl/ynl-gen-c.py
>> +++ b/tools/net/ynl/ynl-gen-c.py
>> @@ -723,6 +723,8 @@ class AttrSet(SpecAttrSet):
>>              self.c_name = ''
>>  
>>      def new_attr(self, elem, value):
>> +        if 'type' not in elem:
>> +            raise Exception(f"Type has to be set for attribute {elem['name']}")
>>          if elem['type'] in scalars:
>>              t = TypeScalar(self.family, self, elem, value)
>>          elif elem['type'] == 'unused':
>
>Can this still be enforced using JSON schema? Using dependencies 
>to make sure that if subset-of is not present type is?

I have no clue. I know very little about json schema. I can take a look.

[patch net-next v2 2/3] netlink: specs: remove redundant type keys from attributes in subsets

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

No longer needed to define type for subset attributes. Remove those.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- new patch
---
 Documentation/netlink/specs/devlink.yaml | 10 ----------
 Documentation/netlink/specs/dpll.yaml    |  8 --------
 Documentation/netlink/specs/ethtool.yaml |  3 ---
 3 files changed, 21 deletions(-)

diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
index d1ebcd927149..86a12c5bcff1 100644
--- a/Documentation/netlink/specs/devlink.yaml
+++ b/Documentation/netlink/specs/devlink.yaml
@@ -199,54 +199,44 @@ attribute-sets:
     attributes:
       -
         name: reload-stats
-        type: nest
       -
         name: remote-reload-stats
-        type: nest
   -
     name: dl-reload-stats
     subset-of: devlink
     attributes:
       -
         name: reload-action-info
-        type: nest
   -
     name: dl-reload-act-info
     subset-of: devlink
     attributes:
       -
         name: reload-action
-        type: u8
       -
         name: reload-action-stats
-        type: nest
   -
     name: dl-reload-act-stats
     subset-of: devlink
     attributes:
       -
         name: reload-stats-entry
-        type: nest
   -
     name: dl-reload-stats-entry
     subset-of: devlink
     attributes:
       -
         name: reload-stats-limit
-        type: u8
       -
         name: reload-stats-value
-        type: u32
   -
     name: dl-info-version
     subset-of: devlink
     attributes:
       -
         name: info-version-name
-        type: string
       -
         name: info-version-value
-        type: string
 
 operations:
   enum-model: directional
diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
index 8b86b28b47a6..1c1b53136c7b 100644
--- a/Documentation/netlink/specs/dpll.yaml
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -278,36 +278,28 @@ attribute-sets:
     attributes:
       -
         name: parent-id
-        type: u32
       -
         name: direction
-        type: u32
       -
         name: prio
-        type: u32
       -
         name: state
-        type: u32
   -
     name: pin-parent-pin
     subset-of: pin
     attributes:
       -
         name: parent-id
-        type: u32
       -
         name: state
-        type: u32
   -
     name: frequency-range
     subset-of: pin
     attributes:
       -
         name: frequency-min
-        type: u64
       -
         name: frequency-max
-        type: u64
 
 operations:
   enum-name: dpll_cmd
diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 837b565577ca..5c7a65b009b4 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -818,13 +818,10 @@ attribute-sets:
     attributes:
       -
         name: hist-bkt-low
-        type: u32
       -
         name: hist-bkt-hi
-        type: u32
       -
         name: hist-val
-        type: u64
   -
     name: stats
     attributes:
-- 
2.41.0

Re: [patch net-next v2 2/3] netlink: specs: remove redundant type keys from attributes in subsets

[Jakub Kicinski]

On Fri, 29 Sep 2023 15:47:41 +0200 Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> No longer needed to define type for subset attributes. Remove those.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

[patch net-next v2 3/3] tools: ynl-gen: raise exception when subset attribute contains more than "name" key

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

The only key used in the elem dictionary is "name" to lookup the real
attribute of a set. Raise exception in case there are other keys
present.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- new patch
---
 tools/net/ynl/lib/nlspec.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index 37bcb4d8b37b..12e15ac70309 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -208,6 +208,8 @@ class SpecAttrSet(SpecElement):
                 attr = real_set[elem['name']]
                 self.attrs[attr.name] = attr
                 self.attrs_by_val[attr.value] = attr
+                if (len(elem.keys()) > 1):
+                    raise Exception(f"Subset attribute '{elem['name']}' contains other keys")
 
     def new_attr(self, elem, value):
         return SpecAttr(self.family, self, elem, value)
-- 
2.41.0

Re: [patch net-next v2 3/3] tools: ynl-gen: raise exception when subset attribute contains more than "name" key

[Jakub Kicinski]

On Fri, 29 Sep 2023 15:47:42 +0200 Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> The only key used in the elem dictionary is "name" to lookup the real
> attribute of a set. Raise exception in case there are other keys
> present.

Mm, there are definitely other things that can be set. I'm not fully
sold that type can't change but even if - checks can easily be adjusted
or nested-attributes, based on the parsing path.

Re: [patch net-next v2 3/3] tools: ynl-gen: raise exception when subset attribute contains more than "name" key

[Jiri Pirko]

Thu, Oct 05, 2023 at 02:13:50AM CEST, kuba@kernel.org wrote:
>On Fri, 29 Sep 2023 15:47:42 +0200 Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> The only key used in the elem dictionary is "name" to lookup the real
>> attribute of a set. Raise exception in case there are other keys
>> present.
>
>Mm, there are definitely other things that can be set. I'm not fully

Which ones? The name is used, the rest is ignored in the existing code.
I just make this obvious to the user. If future show other keys are
needed here, the patch adding that would just adjust the exception
condition. Do you see any problem in that?


>sold that type can't change but even if - checks can easily be adjusted
>or nested-attributes, based on the parsing path.

Re: [patch net-next v2 3/3] tools: ynl-gen: raise exception when subset attribute contains more than "name" key

[Jakub Kicinski]

On Thu, 5 Oct 2023 09:25:55 +0200 Jiri Pirko wrote:
> >> The only key used in the elem dictionary is "name" to lookup the real
> >> attribute of a set. Raise exception in case there are other keys
> >> present.  
> >
> >Mm, there are definitely other things that can be set. I'm not fully  
> 
> Which ones? The name is used, the rest is ignored in the existing code.
> I just make this obvious to the user. If future show other keys are
> needed here, the patch adding that would just adjust the exception
> condition. Do you see any problem in that?

Just don't want to give people the impression that this is what's
intended, rather than it was simply not implemented yet.
If you want to keep the exception please update the message
(and the if, no outer brackets necessary in Python ;)).

Re: [patch net-next v2 3/3] tools: ynl-gen: raise exception when subset attribute contains more than "name" key

[Jiri Pirko]

Thu, Oct 05, 2023 at 04:21:51PM CEST, kuba@kernel.org wrote:
>On Thu, 5 Oct 2023 09:25:55 +0200 Jiri Pirko wrote:
>> >> The only key used in the elem dictionary is "name" to lookup the real
>> >> attribute of a set. Raise exception in case there are other keys
>> >> present.  
>> >
>> >Mm, there are definitely other things that can be set. I'm not fully  
>> 
>> Which ones? The name is used, the rest is ignored in the existing code.
>> I just make this obvious to the user. If future show other keys are
>> needed here, the patch adding that would just adjust the exception
>> condition. Do you see any problem in that?
>
>Just don't want to give people the impression that this is what's
>intended, rather than it was simply not implemented yet.
>If you want to keep the exception please update the message
>(and the if, no outer brackets necessary in Python ;)).

I don't mind dropping the patch entirely. I just thought it would be
nice to do some sanitization so the user is not surprised that other
possible keys are ignored. I tried and I was :)