[PATCH net-next v2 0/3] tools: ynl: decode link types present in tests

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests
@ 2025-01-07  2:28 Jakub Kicinski
  2025-01-07  2:28 ` [PATCH net-next v2 1/3] tools: ynl: correctly handle overrides of fields in subset Jakub Kicinski
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Jakub Kicinski @ 2025-01-07  2:28 UTC (permalink / raw)
  To: davem; +Cc: donald.hunter, netdev, edumazet, pabeni, Jakub Kicinski

Using a kernel built for the net selftest target to run drivers/net
tests currently fails, because the net kernel automatically spawns
a handful of tunnel devices which YNL can't decode.

Fill in those missing link types in rt_link. We need to extend subset
support a bit for it to work.

v2:
 - adjust C code gen in patch 1, it dependend on reusing attr objects
v1: https://lore.kernel.org/20250105012523.1722231-1-kuba@kernel.org

Jakub Kicinski (3):
  tools: ynl: correctly handle overrides of fields in subset
  tools: ynl: print some information about attribute we can't parse
  netlink: specs: rt_link: decode ip6tnl, vti and vti6 link attrs

 Documentation/netlink/specs/rt_link.yaml | 87 ++++++++++++++++++++++++
 tools/net/ynl/lib/nlspec.py              |  5 +-
 tools/net/ynl/lib/ynl.py                 | 72 +++++++++++---------
 tools/net/ynl/ynl-gen-c.py               | 26 +++++--
 4 files changed, 151 insertions(+), 39 deletions(-)

-- 
2.47.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net-next v2 1/3] tools: ynl: correctly handle overrides of fields in subset
  2025-01-07  2:28 [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests Jakub Kicinski
@ 2025-01-07  2:28 ` Jakub Kicinski
  2025-01-07 10:53   ` Donald Hunter
  2025-01-07  2:28 ` [PATCH net-next v2 2/3] tools: ynl: print some information about attribute we can't parse Jakub Kicinski
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2025-01-07  2:28 UTC (permalink / raw)
  To: davem; +Cc: donald.hunter, netdev, edumazet, pabeni, Jakub Kicinski

We stated in documentation [1] and previous discussions [2]
that the need for overriding fields in members of subsets
is anticipated. Implement it.

Since each attr is now a new object we need to make sure
that the modifications are propagated. Specifically C codegen
wants to annotate which attrs are used in requests and replies
to generate the right validation artifacts.

[1] https://docs.kernel.org/next/userspace-api/netlink/specs.html#subset-of
[2] https://lore.kernel.org/netdev/20231004171350.1f59cd1d@kernel.org/

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - adjust C code gen in patch 1, it dependend on reusing attr objects
v1: https://lore.kernel.org/20250105012523.1722231-1-kuba@kernel.org
---
 tools/net/ynl/lib/nlspec.py |  5 ++++-
 tools/net/ynl/ynl-gen-c.py  | 26 ++++++++++++++++++++++----
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index a745739655ad..314ec8007496 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -219,7 +219,10 @@ jsonschema = None
         else:
             real_set = family.attr_sets[self.subset_of]
             for elem in self.yaml['attributes']:
-                attr = real_set[elem['name']]
+                real_attr = real_set[elem['name']]
+                combined_elem = real_attr.yaml | elem
+                attr = self.new_attr(combined_elem, real_attr.value)
+
                 self.attrs[attr.name] = attr
                 self.attrs_by_val[attr.value] = attr
 
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index ec2288948795..58657dd7dedb 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -79,6 +79,20 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         self.enum_name = None
         delattr(self, "enum_name")
 
+    def _get_real_attr(self):
+        # if the attr is for a subset return the "real" attr (just one down, does not recurse)
+        return self.family.attr_sets[self.attr_set.subset_of][self.name]
+
+    def set_request(self):
+        self.request = True
+        if self.attr_set.subset_of:
+            self._get_real_attr().set_request()
+
+    def set_reply(self):
+        self.reply = True
+        if self.attr_set.subset_of:
+            self._get_real_attr().set_reply()
+
     def get_limit(self, limit, default=None):
         value = self.checks.get(limit, default)
         if value is None:
@@ -106,6 +120,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
             enum_name = f"{self.attr_set.name_prefix}{self.name}"
         self.enum_name = c_upper(enum_name)
 
+        if self.attr_set.subset_of:
+            if self.checks != self._get_real_attr().checks:
+                raise Exception("Overriding checks not supported by codegen, yet")
+
     def is_multi_val(self):
         return None
 
@@ -1119,17 +1137,17 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         for _, struct in self.pure_nested_structs.items():
             if struct.request:
                 for _, arg in struct.member_list():
-                    arg.request = True
+                    arg.set_request()
             if struct.reply:
                 for _, arg in struct.member_list():
-                    arg.reply = True
+                    arg.set_reply()
 
         for root_set, rs_members in self.root_sets.items():
             for attr, spec in self.attr_sets[root_set].items():
                 if attr in rs_members['request']:
-                    spec.request = True
+                    spec.set_request()
                 if attr in rs_members['reply']:
-                    spec.reply = True
+                    spec.set_reply()
 
     def _load_global_policy(self):
         global_set = set()
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v2 1/3] tools: ynl: correctly handle overrides of fields in subset
  2025-01-07  2:28 ` [PATCH net-next v2 1/3] tools: ynl: correctly handle overrides of fields in subset Jakub Kicinski
@ 2025-01-07 10:53   ` Donald Hunter
  0 siblings, 0 replies; 6+ messages in thread
From: Donald Hunter @ 2025-01-07 10:53 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni

Jakub Kicinski <kuba@kernel.org> writes:

> We stated in documentation [1] and previous discussions [2]
> that the need for overriding fields in members of subsets
> is anticipated. Implement it.
>
> Since each attr is now a new object we need to make sure
> that the modifications are propagated. Specifically C codegen
> wants to annotate which attrs are used in requests and replies
> to generate the right validation artifacts.
>
> [1] https://docs.kernel.org/next/userspace-api/netlink/specs.html#subset-of
> [2] https://lore.kernel.org/netdev/20231004171350.1f59cd1d@kernel.org/
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net-next v2 2/3] tools: ynl: print some information about attribute we can't parse
  2025-01-07  2:28 [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests Jakub Kicinski
  2025-01-07  2:28 ` [PATCH net-next v2 1/3] tools: ynl: correctly handle overrides of fields in subset Jakub Kicinski
@ 2025-01-07  2:28 ` Jakub Kicinski
  2025-01-07  2:28 ` [PATCH net-next v2 3/3] netlink: specs: rt_link: decode ip6tnl, vti and vti6 link attrs Jakub Kicinski
  2025-01-08  2:20 ` [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests patchwork-bot+netdevbpf
  3 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2025-01-07  2:28 UTC (permalink / raw)
  To: davem
  Cc: donald.hunter, netdev, edumazet, pabeni, Jakub Kicinski,
	Stanislav Fomichev

When parsing throws an exception one often has to figure out which
attribute couldn't be parsed from first principles. For families
with large message parsing trees like rtnetlink guessing the
attribute can be hard.

Print a bit of information as the exception travels out, e.g.:

  # when dumping rt links
  Error decoding 'flags' from 'linkinfo-ip6tnl-attrs'
  Error decoding 'data' from 'linkinfo-attrs'
  Error decoding 'linkinfo' from 'link-attrs'
  Traceback (most recent call last):
    File "/home/kicinski/linux/./tools/net/ynl/cli.py", line 119, in <module>
      main()
    File "/home/kicinski/linux/./tools/net/ynl/cli.py", line 100, in main
      reply = ynl.dump(args.dump, attrs)
    File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 1064, in dump
      return self._op(method, vals, dump=True)
    File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 1058, in _op
      return self._ops(ops)[0]
    File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 1045, in _ops
      rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name)
    File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 738, in _decode
      subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], search_attrs)
    File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 763, in _decode
      decoded = self._decode_sub_msg(attr, attr_spec, search_attrs)
    File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 714, in _decode_sub_msg
      subdict = self._decode(NlAttrs(attr.raw, offset), msg_format.attr_set)
    File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 749, in _decode
      decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order)
    File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 147, in as_scalar
      return format.unpack(self.raw)[0]
  struct.error: unpack requires a buffer of 2 bytes

The Traceback is what we would previously see, the "Error..."
messages are new. We print a message per level (in the stack
order). Printing single combined message gets tricky quickly
given sub-messages etc.

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl.py | 72 +++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 34 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index eea29359a899..08f8bf89cfc2 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -733,41 +733,45 @@ genl_family_name_to_id = None
                 self._rsp_add(rsp, attr_name, None, self._decode_unknown(attr))
                 continue
 
-            if attr_spec["type"] == 'nest':
-                subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], search_attrs)
-                decoded = subdict
-            elif attr_spec["type"] == 'string':
-                decoded = attr.as_strz()
-            elif attr_spec["type"] == 'binary':
-                decoded = self._decode_binary(attr, attr_spec)
-            elif attr_spec["type"] == 'flag':
-                decoded = True
-            elif attr_spec.is_auto_scalar:
-                decoded = attr.as_auto_scalar(attr_spec['type'], attr_spec.byte_order)
-            elif attr_spec["type"] in NlAttr.type_formats:
-                decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order)
-                if 'enum' in attr_spec:
-                    decoded = self._decode_enum(decoded, attr_spec)
-                elif attr_spec.display_hint:
-                    decoded = self._formatted_string(decoded, attr_spec.display_hint)
-            elif attr_spec["type"] == 'indexed-array':
-                decoded = self._decode_array_attr(attr, attr_spec)
-            elif attr_spec["type"] == 'bitfield32':
-                value, selector = struct.unpack("II", attr.raw)
-                if 'enum' in attr_spec:
-                    value = self._decode_enum(value, attr_spec)
-                    selector = self._decode_enum(selector, attr_spec)
-                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"]}')
-                decoded = self._decode_unknown(attr)
+            try:
+                if attr_spec["type"] == 'nest':
+                    subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], search_attrs)
+                    decoded = subdict
+                elif attr_spec["type"] == 'string':
+                    decoded = attr.as_strz()
+                elif attr_spec["type"] == 'binary':
+                    decoded = self._decode_binary(attr, attr_spec)
+                elif attr_spec["type"] == 'flag':
+                    decoded = True
+                elif attr_spec.is_auto_scalar:
+                    decoded = attr.as_auto_scalar(attr_spec['type'], attr_spec.byte_order)
+                elif attr_spec["type"] in NlAttr.type_formats:
+                    decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order)
+                    if 'enum' in attr_spec:
+                        decoded = self._decode_enum(decoded, attr_spec)
+                    elif attr_spec.display_hint:
+                        decoded = self._formatted_string(decoded, attr_spec.display_hint)
+                elif attr_spec["type"] == 'indexed-array':
+                    decoded = self._decode_array_attr(attr, attr_spec)
+                elif attr_spec["type"] == 'bitfield32':
+                    value, selector = struct.unpack("II", attr.raw)
+                    if 'enum' in attr_spec:
+                        value = self._decode_enum(value, attr_spec)
+                        selector = self._decode_enum(selector, attr_spec)
+                    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"]}')
+                    decoded = self._decode_unknown(attr)
 
-            self._rsp_add(rsp, attr_spec["name"], attr_spec.is_multi, decoded)
+                self._rsp_add(rsp, attr_spec["name"], attr_spec.is_multi, decoded)
+            except:
+                print(f"Error decoding '{attr_spec.name}' from '{space}'")
+                raise
 
         return rsp
 
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH net-next v2 3/3] netlink: specs: rt_link: decode ip6tnl, vti and vti6 link attrs
  2025-01-07  2:28 [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests Jakub Kicinski
  2025-01-07  2:28 ` [PATCH net-next v2 1/3] tools: ynl: correctly handle overrides of fields in subset Jakub Kicinski
  2025-01-07  2:28 ` [PATCH net-next v2 2/3] tools: ynl: print some information about attribute we can't parse Jakub Kicinski
@ 2025-01-07  2:28 ` Jakub Kicinski
  2025-01-08  2:20 ` [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests patchwork-bot+netdevbpf
  3 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2025-01-07  2:28 UTC (permalink / raw)
  To: davem
  Cc: donald.hunter, netdev, edumazet, pabeni, Jakub Kicinski,
	Stanislav Fomichev

Some of our tests load vti and ip6tnl so not being able to decode
the link attrs gets in the way of using Python YNL for testing.

Decode link attributes for ip6tnl, vti and vti6.

ip6tnl uses IFLA_IPTUN_FLAGS as u32, while ipv4 and sit expect
a u16 attribute, so we have a (first?) subset type override...

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/rt_link.yaml | 87 ++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml
index 96465376d6fe..363c4d4f0779 100644
--- a/Documentation/netlink/specs/rt_link.yaml
+++ b/Documentation/netlink/specs/rt_link.yaml
@@ -1825,6 +1825,48 @@ protonum: 0
       -
         name: erspan-hwid
         type: u16
+  -
+    name: linkinfo-vti-attrs
+    name-prefix: ifla-vti-
+    attributes:
+      -
+        name: link
+        type: u32
+      -
+        name: ikey
+        type: u32
+      -
+        name: okey
+        type: u32
+      -
+        name: local
+        type: binary
+        display-hint: ipv4
+      -
+        name: remote
+        type: binary
+        display-hint: ipv4
+      -
+        name: fwmark
+        type: u32
+  -
+    name: linkinfo-vti6-attrs
+    subset-of: linkinfo-vti-attrs
+    attributes:
+      -
+        name: link
+      -
+        name: ikey
+      -
+        name: okey
+      -
+        name: local
+        display-hint: ipv6
+      -
+        name: remote
+        display-hint: ipv6
+      -
+        name: fwmark
   -
     name: linkinfo-geneve-attrs
     name-prefix: ifla-geneve-
@@ -1941,6 +1983,42 @@ protonum: 0
       -
         name: fwmark
         type: u32
+  -
+    name: linkinfo-ip6tnl-attrs
+    subset-of: linkinfo-iptun-attrs
+    attributes:
+      -
+        name: link
+      -
+        name: local
+        display-hint: ipv6
+      -
+        name: remote
+        display-hint: ipv6
+      -
+        name: ttl
+      -
+        name: encap-limit
+      -
+        name: flowinfo
+      -
+        name: flags
+        # ip6tnl unlike ipip and sit has 32b flags
+        type: u32
+      -
+        name: proto
+      -
+        name: encap-type
+      -
+        name: encap-flags
+      -
+        name: encap-sport
+      -
+        name: encap-dport
+      -
+        name: collect-metadata
+      -
+        name: fwmark
   -
     name: linkinfo-tun-attrs
     name-prefix: ifla-tun-
@@ -2195,6 +2273,9 @@ protonum: 0
       -
         value: ipip
         attribute-set: linkinfo-iptun-attrs
+      -
+        value: ip6tnl
+        attribute-set: linkinfo-ip6tnl-attrs
       -
         value: sit
         attribute-set: linkinfo-iptun-attrs
@@ -2207,6 +2288,12 @@ protonum: 0
       -
         value: vrf
         attribute-set: linkinfo-vrf-attrs
+      -
+        value: vti
+        attribute-set: linkinfo-vti-attrs
+      -
+        value: vti6
+        attribute-set: linkinfo-vti6-attrs
       -
         value: netkit
         attribute-set: linkinfo-netkit-attrs
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests
  2025-01-07  2:28 [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests Jakub Kicinski
                   ` (2 preceding siblings ...)
  2025-01-07  2:28 ` [PATCH net-next v2 3/3] netlink: specs: rt_link: decode ip6tnl, vti and vti6 link attrs Jakub Kicinski
@ 2025-01-08  2:20 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-01-08  2:20 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, donald.hunter, netdev, edumazet, pabeni

Hello:

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

On Mon,  6 Jan 2025 18:28:17 -0800 you wrote:
> Using a kernel built for the net selftest target to run drivers/net
> tests currently fails, because the net kernel automatically spawns
> a handful of tunnel devices which YNL can't decode.
> 
> Fill in those missing link types in rt_link. We need to extend subset
> support a bit for it to work.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/3] tools: ynl: correctly handle overrides of fields in subset
    https://git.kernel.org/netdev/net-next/c/69072db934df
  - [net-next,v2,2/3] tools: ynl: print some information about attribute we can't parse
    https://git.kernel.org/netdev/net-next/c/7aae6505351e
  - [net-next,v2,3/3] netlink: specs: rt_link: decode ip6tnl, vti and vti6 link attrs
    https://git.kernel.org/netdev/net-next/c/6ffdbb93a59c

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-01-08  2:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-07  2:28 [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests Jakub Kicinski
2025-01-07  2:28 ` [PATCH net-next v2 1/3] tools: ynl: correctly handle overrides of fields in subset Jakub Kicinski
2025-01-07 10:53   ` Donald Hunter
2025-01-07  2:28 ` [PATCH net-next v2 2/3] tools: ynl: print some information about attribute we can't parse Jakub Kicinski
2025-01-07  2:28 ` [PATCH net-next v2 3/3] netlink: specs: rt_link: decode ip6tnl, vti and vti6 link attrs Jakub Kicinski
2025-01-08  2:20 ` [PATCH net-next v2 0/3] tools: ynl: decode link types present in tests patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).