[PATCH net-next v3 0/6] tools/net/ynl: Add support for nlctrl netlink family

[PATCH net-next v3 0/6] tools/net/ynl: Add support for nlctrl netlink family

[Donald Hunter]

This series adds a new YNL spec for the nlctrl family, plus some fixes
and enhancements for ynl.

Patch 1 fixes an extack decoding bug
Patch 2 gives cleaner netlink error reporting
Patch 3 fixes an array-nest codegen bug
Patch 4 adds nest-type-value support to ynl
Patch 5 fixes the ynl schemas to allow empty enum-name attrs
Patch 6 contains the nlctrl spec

Changes from v2 -> v3:
 - Add a better description to the nlctrl spec (Jakub)
 - Format all codegen names using lower-kebab-case (Jakub)
 - Add a patch to fix the schemas

Changes from v1 -> v2:
 - Added patch 3 to fix codegen bug
 - Removed multi-level array-nest patch
 - Added nest-type-value patch
 - Updated nlctrl spec to align with headers and fix compilation

Donald Hunter (6):
  tools/net/ynl: Fix extack decoding for netlink-raw
  tools/net/ynl: Report netlink errors without stacktrace
  tools/net/ynl: Fix c codegen for array-nest
  tools/net/ynl: Add nest-type-value decoding
  doc/netlink: Allow empty enum-name in ynl specs
  doc/netlink/specs: Add spec for nlctrl netlink family

 Documentation/netlink/genetlink-c.yaml      |  15 +-
 Documentation/netlink/genetlink-legacy.yaml |  15 +-
 Documentation/netlink/netlink-raw.yaml      |  15 +-
 Documentation/netlink/specs/nlctrl.yaml     | 207 ++++++++++++++++++++
 tools/net/ynl/cli.py                        |  18 +-
 tools/net/ynl/lib/__init__.py               |   4 +-
 tools/net/ynl/lib/ynl.py                    |  19 +-
 tools/net/ynl/ynl-gen-c.py                  |   2 +-
 8 files changed, 266 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/netlink/specs/nlctrl.yaml

-- 
2.42.0

[PATCH net-next v3 1/6] tools/net/ynl: Fix extack decoding for netlink-raw

[Donald Hunter]

Extack decoding was using a hard-coded msg header size of 20 but
netlink-raw has a header size of 16.

Use a protocol specific msghdr_size() when decoding the attr offssets.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 239e22b7a85f..b810a478a304 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -353,6 +353,9 @@ class NetlinkProtocol:
             raise Exception(f'Multicast group "{mcast_name}" not present in the spec')
         return mcast_groups[mcast_name].value
 
+    def msghdr_size(self):
+        return 16
+
 
 class GenlProtocol(NetlinkProtocol):
     def __init__(self, family_name):
@@ -378,6 +381,8 @@ class GenlProtocol(NetlinkProtocol):
             raise Exception(f'Multicast group "{mcast_name}" not present in the family')
         return self.genl_family['mcast'][mcast_name]
 
+    def msghdr_size(self):
+        return super().msghdr_size() + 4
 
 
 class SpaceAttrs:
@@ -721,7 +726,7 @@ class YnlFamily(SpecFamily):
             return
 
         msg = self.nlproto.decode(self, NlMsg(request, 0, op.attr_set))
-        offset = 20 + self._struct_size(op.fixed_header)
+        offset = self.nlproto.msghdr_size() + self._struct_size(op.fixed_header)
         path = self._decode_extack_path(msg.raw_attrs, op.attr_set, offset,
                                         extack['bad-attr-offs'])
         if path:
-- 
2.42.0

[PATCH net-next v3 2/6] tools/net/ynl: Report netlink errors without stacktrace

[Donald Hunter]

ynl does not handle NlError exceptions so they get reported like program
failures. Handle the NlError exceptions and report the netlink errors
more cleanly.

Example now:

Netlink error: No such file or directory
nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
	error: -2	extack: {'bad-attr': '.op'}

Example before:

Traceback (most recent call last):
  File "/home/donaldh/net-next/./tools/net/ynl/cli.py", line 81, in <module>
    main()
  File "/home/donaldh/net-next/./tools/net/ynl/cli.py", line 69, in main
    reply = ynl.dump(args.dump, attrs)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/donaldh/net-next/tools/net/ynl/lib/ynl.py", line 906, in dump
    return self._op(method, vals, [], dump=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/donaldh/net-next/tools/net/ynl/lib/ynl.py", line 872, in _op
    raise NlError(nl_msg)
lib.ynl.NlError: Netlink error: No such file or directory
nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
	error: -2	extack: {'bad-attr': '.op'}

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/cli.py          | 18 +++++++++++-------
 tools/net/ynl/lib/__init__.py |  4 ++--
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/net/ynl/cli.py b/tools/net/ynl/cli.py
index e8a65fbc3698..f131e33ac3ee 100755
--- a/tools/net/ynl/cli.py
+++ b/tools/net/ynl/cli.py
@@ -6,7 +6,7 @@ import json
 import pprint
 import time
 
-from lib import YnlFamily, Netlink
+from lib import YnlFamily, Netlink, NlError
 
 
 class YnlEncoder(json.JSONEncoder):
@@ -66,12 +66,16 @@ def main():
     if args.sleep:
         time.sleep(args.sleep)
 
-    if args.do:
-        reply = ynl.do(args.do, attrs, args.flags)
-        output(reply)
-    if args.dump:
-        reply = ynl.dump(args.dump, attrs)
-        output(reply)
+    try:
+        if args.do:
+            reply = ynl.do(args.do, attrs, args.flags)
+            output(reply)
+        if args.dump:
+            reply = ynl.dump(args.dump, attrs)
+            output(reply)
+    except NlError as e:
+        print(e)
+        exit(1)
 
     if args.ntf:
         ynl.check_ntf()
diff --git a/tools/net/ynl/lib/__init__.py b/tools/net/ynl/lib/__init__.py
index f7eaa07783e7..9137b83e580a 100644
--- a/tools/net/ynl/lib/__init__.py
+++ b/tools/net/ynl/lib/__init__.py
@@ -2,7 +2,7 @@
 
 from .nlspec import SpecAttr, SpecAttrSet, SpecEnumEntry, SpecEnumSet, \
     SpecFamily, SpecOperation
-from .ynl import YnlFamily, Netlink
+from .ynl import YnlFamily, Netlink, NlError
 
 __all__ = ["SpecAttr", "SpecAttrSet", "SpecEnumEntry", "SpecEnumSet",
-           "SpecFamily", "SpecOperation", "YnlFamily", "Netlink"]
+           "SpecFamily", "SpecOperation", "YnlFamily", "Netlink", "NlError"]
-- 
2.42.0

[PATCH net-next v3 3/6] tools/net/ynl: Fix c codegen for array-nest

[Donald Hunter]

ynl-gen-c generates e.g. 'calloc(mcast_groups, sizeof(*dst->mcast_groups))'
for array-nest attrs when it should be 'n_mcast_groups'.

Add a 'n_' prefix in the generated code for array-nests.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/ynl-gen-c.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 2f5febfe66a1..67bfaff05154 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -1667,7 +1667,7 @@ def _multi_parse(ri, struct, init_lines, local_vars):
         aspec = struct[anest]
 
         ri.cw.block_start(line=f"if (n_{aspec.c_name})")
-        ri.cw.p(f"dst->{aspec.c_name} = calloc({aspec.c_name}, sizeof(*dst->{aspec.c_name}));")
+        ri.cw.p(f"dst->{aspec.c_name} = calloc(n_{aspec.c_name}, sizeof(*dst->{aspec.c_name}));")
         ri.cw.p(f"dst->n_{aspec.c_name} = n_{aspec.c_name};")
         ri.cw.p('i = 0;')
         ri.cw.p(f"parg.rsp_policy = &{aspec.nested_render_name}_nest;")
-- 
2.42.0

[PATCH net-next v3 4/6] tools/net/ynl: Add nest-type-value decoding

[Donald Hunter]

The nlctrl genetlink-legacy family uses nest-type-value encoding as
described in Documentation/userspace-api/netlink/genetlink-legacy.rst

Add nest-type-value decoding to ynl.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index b810a478a304..2d7fdd903d9e 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -595,6 +595,16 @@ class YnlFamily(SpecFamily):
             decoded.append({ item.type: subattrs })
         return decoded
 
+    def _decode_nest_type_value(self, attr, attr_spec):
+        decoded = {}
+        value = attr
+        for name in attr_spec['type-value']:
+            value = NlAttr(value.raw, 0)
+            decoded[name] = value.type
+        subattrs = self._decode(NlAttrs(value.raw), attr_spec['nested-attributes'])
+        decoded.update(subattrs)
+        return decoded
+
     def _decode_unknown(self, attr):
         if attr.is_nest:
             return self._decode(NlAttrs(attr.raw), None)
@@ -686,6 +696,8 @@ class YnlFamily(SpecFamily):
                 decoded = {"value": value, "selector": selector}
             elif attr_spec["type"] == 'sub-message':
                 decoded = self._decode_sub_msg(attr, attr_spec, search_attrs)
+            elif attr_spec["type"] == 'nest-type-value':
+                decoded = self._decode_nest_type_value(attr, attr_spec)
             else:
                 if not self.process_unknown:
                     raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
-- 
2.42.0

[PATCH net-next v3 5/6] doc/netlink: Allow empty enum-name in ynl specs

[Donald Hunter]

Update the ynl schemas to allow the specification of empty enum names
for all enum code generation.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/genetlink-c.yaml      | 15 +++++++++------
 Documentation/netlink/genetlink-legacy.yaml | 15 +++++++++------
 Documentation/netlink/netlink-raw.yaml      | 15 +++++++++------
 3 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml
index c58f7153fcf8..3ebd50d78820 100644
--- a/Documentation/netlink/genetlink-c.yaml
+++ b/Documentation/netlink/genetlink-c.yaml
@@ -126,8 +126,9 @@ properties:
             Prefix for the C enum name of the attributes. Default family[name]-set[name]-a-
           type: string
         enum-name:
-          description: Name for the enum type of the attribute.
-          type: string
+          description: |
+            Name for the enum type of the attribute, if empty no name will be used.
+          type: [ string, "null" ]
         doc:
           description: Documentation of the space.
           type: string
@@ -261,14 +262,16 @@ properties:
           the prefix with the upper case name of the command, with dashes replaced by underscores.
         type: string
       enum-name:
-        description: Name for the enum type with commands.
-        type: string
+        description: |
+          Name for the enum type with commands, if empty no name will be used.
+        type: [ string, "null" ]
       async-prefix:
         description: Same as name-prefix but used to render notifications and events to separate enum.
         type: string
       async-enum:
-        description: Name for the enum type with notifications/events.
-        type: string
+        description: |
+          Name for the enum type with commands, if empty no name will be used.
+        type: [ string, "null" ]
       list:
         description: List of commands
         type: array
diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index 938703088306..1d3fe3637707 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -168,8 +168,9 @@ properties:
             Prefix for the C enum name of the attributes. Default family[name]-set[name]-a-
           type: string
         enum-name:
-          description: Name for the enum type of the attribute.
-          type: string
+          description: |
+            Name for the enum type of the attribute, if empty no name will be used.
+          type: [ string, "null" ]
         doc:
           description: Documentation of the space.
           type: string
@@ -304,14 +305,16 @@ properties:
           the prefix with the upper case name of the command, with dashes replaced by underscores.
         type: string
       enum-name:
-        description: Name for the enum type with commands.
-        type: string
+        description: |
+          Name for the enum type with commands, if empty no name will be used.
+        type: [ string, "null" ]
       async-prefix:
         description: Same as name-prefix but used to render notifications and events to separate enum.
         type: string
       async-enum:
-        description: Name for the enum type with notifications/events.
-        type: string
+        description: |
+          Name for the enum type with commands, if empty no name will be used.
+        type: [ string, "null" ]
       # Start genetlink-legacy
       fixed-header: &fixed-header
         description: |
diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml
index ac4e05415f2f..40fc8ab1ee44 100644
--- a/Documentation/netlink/netlink-raw.yaml
+++ b/Documentation/netlink/netlink-raw.yaml
@@ -189,8 +189,9 @@ properties:
             Prefix for the C enum name of the attributes. Default family[name]-set[name]-a-
           type: string
         enum-name:
-          description: Name for the enum type of the attribute.
-          type: string
+          description: |
+            Name for the enum type of the attribute, if empty no name will be used.
+          type: [ string, "null" ]
         doc:
           description: Documentation of the space.
           type: string
@@ -371,14 +372,16 @@ properties:
           the prefix with the upper case name of the command, with dashes replaced by underscores.
         type: string
       enum-name:
-        description: Name for the enum type with commands.
-        type: string
+        description: |
+          Name for the enum type with commands, if empty no name will be used.
+        type: [ string, "null" ]
       async-prefix:
         description: Same as name-prefix but used to render notifications and events to separate enum.
         type: string
       async-enum:
-        description: Name for the enum type with notifications/events.
-        type: string
+        description: |
+          Name for the enum type with commands, if empty no name will be used.
+        type: [ string, "null" ]
       # Start genetlink-legacy
       fixed-header: &fixed-header
         description: |
-- 
2.42.0

[PATCH net-next v3 6/6] doc/netlink/specs: Add spec for nlctrl netlink family

[Donald Hunter]

Add a spec for the nlctrl family.

Example usage:

./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/nlctrl.yaml \
    --do getfamily --json '{"family-name": "nlctrl"}'

./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/nlctrl.yaml \
    --dump getpolicy --json '{"family-name": "nlctrl"}'

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

diff --git a/Documentation/netlink/specs/nlctrl.yaml b/Documentation/netlink/specs/nlctrl.yaml
new file mode 100644
index 000000000000..08a8d047d52d
--- /dev/null
+++ b/Documentation/netlink/specs/nlctrl.yaml
@@ -0,0 +1,206 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: nlctrl
+protocol: genetlink-legacy
+uapi-header: linux/genetlink.h
+
+doc: |
+  genetlink meta-family that exposes information about all genetlink
+  families registered in the kernel (including itself).
+
+definitions:
+  -
+    name: op-flags
+    type: flags
+    enum-name:
+    entries:
+      - admin-perm
+      - cmd-cap-do
+      - cmd-cap-dump
+      - cmd-cap-haspol
+      - uns-admin-perm
+  -
+    name: attr-type
+    enum-name: netlink-attribute-type
+    type: enum
+    entries:
+      - invalid
+      - flag
+      - u8
+      - u16
+      - u32
+      - u64
+      - s8
+      - s16
+      - s32
+      - s64
+      - binary
+      - string
+      - nul-string
+      - nested
+      - nested-array
+      - bitfield32
+      - sint
+      - uint
+
+attribute-sets:
+  -
+    name: ctrl-attrs
+    name-prefix: ctrl-attr-
+    attributes:
+      -
+        name: family-id
+        type: u16
+      -
+        name: family-name
+        type: string
+      -
+        name: version
+        type: u32
+      -
+        name: hdrsize
+        type: u32
+      -
+        name: maxattr
+        type: u32
+      -
+        name: ops
+        type: array-nest
+        nested-attributes: op-attrs
+      -
+        name: mcast-groups
+        type: array-nest
+        nested-attributes: mcast-group-attrs
+      -
+        name: policy
+        type: nest-type-value
+        type-value: [ policy-id, attr-id ]
+        nested-attributes: policy-attrs
+      -
+        name: op-policy
+        type: nest-type-value
+        type-value: [ op-id ]
+        nested-attributes: op-policy-attrs
+      -
+        name: op
+        type: u32
+  -
+    name: mcast-group-attrs
+    name-prefix: ctrl-attr-mcast-grp-
+    enum-name:
+    attributes:
+      -
+        name: name
+        type: string
+      -
+        name: id
+        type: u32
+  -
+    name: op-attrs
+    name-prefix: ctrl-attr-op-
+    enum-name:
+    attributes:
+      -
+        name: id
+        type: u32
+      -
+        name: flags
+        type: u32
+        enum: op-flags
+        enum-as-flags: true
+  -
+    name: policy-attrs
+    name-prefix: nl-policy-type-attr-
+    enum-name:
+    attributes:
+      -
+        name: type
+        type: u32
+        enum: attr-type
+      -
+        name: min-value-s
+        type: s64
+      -
+        name: max-value-s
+        type: s64
+      -
+        name: min-value-u
+        type: u64
+      -
+        name: max-value-u
+        type: u64
+      -
+        name: min-length
+        type: u32
+      -
+        name: max-length
+        type: u32
+      -
+        name: policy-idx
+        type: u32
+      -
+        name: policy-maxtype
+        type: u32
+      -
+        name: bitfield32-mask
+        type: u32
+      -
+        name: mask
+        type: u64
+      -
+        name: pad
+        type: pad
+  -
+    name: op-policy-attrs
+    name-prefix: ctrl-attr-policy-
+    enum-name:
+    attributes:
+      -
+        name: do
+        type: u32
+      -
+        name: dump
+        type: u32
+
+operations:
+  enum-model: directional
+  name-prefix: ctrl-cmd-
+  list:
+    -
+      name: getfamily
+      doc: Get / dump genetlink families
+      attribute-set: ctrl-attrs
+      do:
+        request:
+          value: 3
+          attributes:
+            - family-name
+        reply: &all-attrs
+          value: 1
+          attributes:
+            - family-id
+            - family-name
+            - hdrsize
+            - maxattr
+            - mcast-groups
+            - ops
+            - version
+      dump:
+        reply: *all-attrs
+    -
+      name: getpolicy
+      doc: Get / dump genetlink policies
+      attribute-set: ctrl-attrs
+      dump:
+        request:
+          value: 10
+          attributes:
+            - family-name
+            - family-id
+            - op
+        reply:
+          value: 10
+          attributes:
+            - family-id
+            - op-policy
+            - policy
-- 
2.42.0

Re: [PATCH net-next v3 5/6] doc/netlink: Allow empty enum-name in ynl specs

[Jakub Kicinski]

On Wed,  6 Mar 2024 23:10:45 +0000 Donald Hunter wrote:
> Update the ynl schemas to allow the specification of empty enum names
> for all enum code generation.

Does ethtool.yaml work for you without this change? 
It has an empty enum.

IDK how the check the exact schema version, I have:

$ rpm -qa | grep jsonschema
python3-jsonschema-specifications-2023.7.1-1.fc39.noarch
python3-jsonschema-4.19.1-1.fc39.noarch

The patch seems legit, but would be good to note for posterity 
what made this fail out of the sudden.

Re: [PATCH net-next v3 5/6] doc/netlink: Allow empty enum-name in ynl specs

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Wed,  6 Mar 2024 23:10:45 +0000 Donald Hunter wrote:
>> Update the ynl schemas to allow the specification of empty enum names
>> for all enum code generation.
>
> Does ethtool.yaml work for you without this change? 
> It has an empty enum.
>
> IDK how the check the exact schema version, I have:
>
> $ rpm -qa | grep jsonschema
> python3-jsonschema-specifications-2023.7.1-1.fc39.noarch
> python3-jsonschema-4.19.1-1.fc39.noarch
>
> The patch seems legit, but would be good to note for posterity 
> what made this fail out of the sudden.

As I mentioned in the earlier thread, the schema already allowed empty
enum-name in definitions. The patch adds it to enum-name in
attribute-sets and operations.

Re: [PATCH net-next v3 2/6] tools/net/ynl: Report netlink errors without stacktrace

[Breno Leitao]

Hello Donald,

On Wed, Mar 06, 2024 at 11:10:42PM +0000, Donald Hunter wrote:
> ynl does not handle NlError exceptions so they get reported like program
> failures. Handle the NlError exceptions and report the netlink errors
> more cleanly.
> 
> Example now:
> 
> Netlink error: No such file or directory
> nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
> 	error: -2	extack: {'bad-attr': '.op'}
> 
> Example before:
> 
> Traceback (most recent call last):
>   File "/home/donaldh/net-next/./tools/net/ynl/cli.py", line 81, in <module>
>     main()
>   File "/home/donaldh/net-next/./tools/net/ynl/cli.py", line 69, in main
>     reply = ynl.dump(args.dump, attrs)
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File "/home/donaldh/net-next/tools/net/ynl/lib/ynl.py", line 906, in dump
>     return self._op(method, vals, [], dump=True)
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File "/home/donaldh/net-next/tools/net/ynl/lib/ynl.py", line 872, in _op
>     raise NlError(nl_msg)
> lib.ynl.NlError: Netlink error: No such file or directory
> nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
> 	error: -2	extack: {'bad-attr': '.op'}

Basically this is just hidding the stack, which may make it harder for
someone not used to the code to find the problem.

Usually fatal exception is handled to make the error more meaningful,
i.e, better than just the exception message + stack. Hidding the stack
and exitting may make the error less meaningful.

On a different topic, I am wondering if we want to add type hitting for
these python program. They make the review process easier, and the
development a bit more structured. (Maybe that is what we expect from
upcoming new python code in netdev?!)

If that is where we want to go, this is *not*, by any mean, a blocker to
this code. Maybe something we can add to our public ToDo list (CC:
Jakub).

Re: [PATCH net-next v3 2/6] tools/net/ynl: Report netlink errors without stacktrace

[Donald Hunter]

On Thu, 7 Mar 2024 at 09:31, Breno Leitao <leitao@debian.org> wrote:
>
> Hello Donald,
>
> On Wed, Mar 06, 2024 at 11:10:42PM +0000, Donald Hunter wrote:
> > ynl does not handle NlError exceptions so they get reported like program
> > failures. Handle the NlError exceptions and report the netlink errors
> > more cleanly.
> >
> > Example now:
> >
> > Netlink error: No such file or directory
> > nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
> >       error: -2       extack: {'bad-attr': '.op'}
> >
> > Example before:
> >
> > Traceback (most recent call last):
> >   File "/home/donaldh/net-next/./tools/net/ynl/cli.py", line 81, in <module>
> >     main()
> >   File "/home/donaldh/net-next/./tools/net/ynl/cli.py", line 69, in main
> >     reply = ynl.dump(args.dump, attrs)
> >             ^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   File "/home/donaldh/net-next/tools/net/ynl/lib/ynl.py", line 906, in dump
> >     return self._op(method, vals, [], dump=True)
> >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   File "/home/donaldh/net-next/tools/net/ynl/lib/ynl.py", line 872, in _op
> >     raise NlError(nl_msg)
> > lib.ynl.NlError: Netlink error: No such file or directory
> > nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
> >       error: -2       extack: {'bad-attr': '.op'}
>
> Basically this is just hidding the stack, which may make it harder for
> someone not used to the code to find the problem.
>
> Usually fatal exception is handled to make the error more meaningful,
> i.e, better than just the exception message + stack. Hidding the stack
> and exitting may make the error less meaningful.

NlError is used to report a usage error reported by netlink as opposed
to a fatal exception. My thinking here is that it is better UX to
report netlink error responses without the stack trace precisely
because they are not exceptional. An NlError is not ynl program
breakage or subsystem breakage, it's e.g. nlctrl telling you that you
requested an op that does not exist.

> On a different topic, I am wondering if we want to add type hitting for
> these python program. They make the review process easier, and the
> development a bit more structured. (Maybe that is what we expect from
> upcoming new python code in netdev?!)

It's a good suggestion. I have never used python type hints so I'll
need to learn about them. I defer to the netdev maintainers about
whether this is something they want.

> If that is where we want to go, this is *not*, by any mean, a blocker to
> this code. Maybe something we can add to our public ToDo list (CC:
> Jakub).

Re: [PATCH net-next v3 5/6] doc/netlink: Allow empty enum-name in ynl specs

[Jakub Kicinski]

On Thu, 07 Mar 2024 09:08:30 +0000 Donald Hunter wrote:
> > On Wed,  6 Mar 2024 23:10:45 +0000 Donald Hunter wrote:  
> >> Update the ynl schemas to allow the specification of empty enum names
> >> for all enum code generation.  
> >
> > Does ethtool.yaml work for you without this change? 
> > It has an empty enum.
> >
> > IDK how the check the exact schema version, I have:
> >
> > $ rpm -qa | grep jsonschema
> > python3-jsonschema-specifications-2023.7.1-1.fc39.noarch
> > python3-jsonschema-4.19.1-1.fc39.noarch
> >
> > The patch seems legit, but would be good to note for posterity 
> > what made this fail out of the sudden.  
> 
> As I mentioned in the earlier thread, the schema already allowed empty
> enum-name in definitions. The patch adds it to enum-name in
> attribute-sets and operations.

Not sure how I missed that, thanks!

Re: [PATCH net-next v3 2/6] tools/net/ynl: Report netlink errors without stacktrace

[Jakub Kicinski]

On Thu, 7 Mar 2024 11:56:59 +0000 Donald Hunter wrote:
> > Basically this is just hidding the stack, which may make it harder for
> > someone not used to the code to find the problem.
> >
> > Usually fatal exception is handled to make the error more meaningful,
> > i.e, better than just the exception message + stack. Hidding the stack
> > and exitting may make the error less meaningful.  
> 
> NlError is used to report a usage error reported by netlink as opposed
> to a fatal exception. My thinking here is that it is better UX to
> report netlink error responses without the stack trace precisely
> because they are not exceptional. An NlError is not ynl program
> breakage or subsystem breakage, it's e.g. nlctrl telling you that you
> requested an op that does not exist.

Right, I think the YNL library should still throw, but since this is
a case of "kernel gave us this specific error in response" the stack
trace adds relatively little for the CLI.

> > On a different topic, I am wondering if we want to add type hitting for
> > these python program. They make the review process easier, and the
> > development a bit more structured. (Maybe that is what we expect from
> > upcoming new python code in netdev?!)  
> 
> It's a good suggestion. I have never used python type hints so I'll
> need to learn about them. I defer to the netdev maintainers about
> whether this is something they want.

I'm far from a Python expert, so up to you :)
I used type hints a couple of times in the past, they are somewhat
useful, but didn't feel useful enough to bother. Happy for someone
else to do the work, tho :)

FWIW I reckon that trying to get the CLI ready for distro packaging 
may be higher prio. Apart from basic requirements to packaging python
code (I have no idea what they are), we should probably extend the
script to search some system paths? My thinking is that if someone
installs the CLI as an RPM, they should be able to use it like this:

 $ ynl-cli --family nlctrl \
	--do getfamily --json '{"family-name": "nlctrl"}'

the --family would be used instead of --spec and look for the exact
spec file in /usr/share/.../specs/ and probably also imply --no-schema,
since hopefully the schema is already validated during development,
and no point wasting time validating it on every user invocation.

WDYT?

Re: [PATCH net-next v3 0/6] tools/net/ynl: Add support for nlctrl netlink family

[patchwork-bot+netdevbpf]

Hello:

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

On Wed,  6 Mar 2024 23:10:40 +0000 you wrote:
> This series adds a new YNL spec for the nlctrl family, plus some fixes
> and enhancements for ynl.
> 
> Patch 1 fixes an extack decoding bug
> Patch 2 gives cleaner netlink error reporting
> Patch 3 fixes an array-nest codegen bug
> Patch 4 adds nest-type-value support to ynl
> Patch 5 fixes the ynl schemas to allow empty enum-name attrs
> Patch 6 contains the nlctrl spec
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/6] tools/net/ynl: Fix extack decoding for netlink-raw
    https://git.kernel.org/netdev/net-next/c/cecbc52c46e2
  - [net-next,v3,2/6] tools/net/ynl: Report netlink errors without stacktrace
    https://git.kernel.org/netdev/net-next/c/771b7012e5f3
  - [net-next,v3,3/6] tools/net/ynl: Fix c codegen for array-nest
    https://git.kernel.org/netdev/net-next/c/6fe7de5e9c08
  - [net-next,v3,4/6] tools/net/ynl: Add nest-type-value decoding
    https://git.kernel.org/netdev/net-next/c/b6e6a76dec33
  - [net-next,v3,5/6] doc/netlink: Allow empty enum-name in ynl specs
    https://git.kernel.org/netdev/net-next/c/bc52b39309c3
  - [net-next,v3,6/6] doc/netlink/specs: Add spec for nlctrl netlink family
    https://git.kernel.org/netdev/net-next/c/768e044a5fd4

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

Re: [PATCH net-next v3 2/6] tools/net/ynl: Report netlink errors without stacktrace

[Breno Leitao]

On Thu, Mar 07, 2024 at 07:58:15AM -0800, Jakub Kicinski wrote:
> On Thu, 7 Mar 2024 11:56:59 +0000 Donald Hunter wrote:
> > > Basically this is just hidding the stack, which may make it harder for
> > > someone not used to the code to find the problem.
> > >
> > > Usually fatal exception is handled to make the error more meaningful,
> > > i.e, better than just the exception message + stack. Hidding the stack
> > > and exitting may make the error less meaningful.  
> > 
> > NlError is used to report a usage error reported by netlink as opposed
> > to a fatal exception. My thinking here is that it is better UX to
> > report netlink error responses without the stack trace precisely
> > because they are not exceptional. An NlError is not ynl program
> > breakage or subsystem breakage, it's e.g. nlctrl telling you that you
> > requested an op that does not exist.
> 
> Right, I think the YNL library should still throw, but since this is
> a case of "kernel gave us this specific error in response" the stack
> trace adds relatively little for the CLI.
> 
> > > On a different topic, I am wondering if we want to add type hitting for
> > > these python program. They make the review process easier, and the
> > > development a bit more structured. (Maybe that is what we expect from
> > > upcoming new python code in netdev?!)  
> > 
> > It's a good suggestion. I have never used python type hints so I'll
> > need to learn about them. I defer to the netdev maintainers about
> > whether this is something they want.
> 
> I'm far from a Python expert, so up to you :)
> I used type hints a couple of times in the past, they are somewhat
> useful, but didn't feel useful enough to bother. Happy for someone
> else to do the work, tho :)

I am a big fan of type hitting, since it help in reviewing code, as also
with tooling that help you to find problems, since the function returns
and arguments now have a type.

What are the top 3 python scripts we have in network today? I can try to
find some time to help.

> FWIW I reckon that trying to get the CLI ready for distro packaging 
> may be higher prio. Apart from basic requirements to packaging python
> code (I have no idea what they are), we should probably extend the
> script to search some system paths? My thinking is that if someone
> installs the CLI as an RPM, they should be able to use it like this:
> 
>  $ ynl-cli --family nlctrl \
> 	--do getfamily --json '{"family-name": "nlctrl"}'
> 
> the --family would be used instead of --spec and look for the exact
> spec file in /usr/share/.../specs/ and probably also imply --no-schema,
> since hopefully the schema is already validated during development,
> and no point wasting time validating it on every user invocation.
> 
> WDYT?

This is a good idea. I've had a chat with Michel, and he can help with
the distro part. Adding him in the CC.

Re: [PATCH net-next v3 2/6] tools/net/ynl: Report netlink errors without stacktrace

[Jakub Kicinski]

On Fri, 8 Mar 2024 10:27:01 -0800 Breno Leitao wrote:
> > I'm far from a Python expert, so up to you :)
> > I used type hints a couple of times in the past, they are somewhat
> > useful, but didn't feel useful enough to bother. Happy for someone
> > else to do the work, tho :)  
> 
> I am a big fan of type hitting, since it help in reviewing code, as also
> with tooling that help you to find problems, since the function returns
> and arguments now have a type.
> 
> What are the top 3 python scripts we have in network today? I can try to
> find some time to help.

I think ynl.py (and nlspec.py) is by far the most used / active piece
of Python we have today.