linux-kernel - Re: [PATCH net-next 05/11] tools: ynl-gen: define nlattr *array in a block scope

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250906120754.7b90c718@kernel.org>
Date: Sat, 6 Sep 2025 12:07:54 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Asbjørn Sloth Tønnesen
 <ast@...erby.net>
Cc: "Jason A. Donenfeld" <Jason@...c4.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>, Simon Horman
 <horms@...nel.org>, Jacob Keller <jacob.e.keller@...el.com>, Andrew Lunn
 <andrew+netdev@...n.ch>, wireguard@...ts.zx2c4.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 05/11] tools: ynl-gen: define nlattr *array in
 a block scope

On Sat, 6 Sep 2025 13:13:29 +0000 Asbjørn Sloth Tønnesen wrote:
> In patch 4, it is about a variable used by multiple Type classes having
> presence_type() = 'count', which is currently 3 classes:
> - TypeBinaryScalarArray
> - TypeMultiAttr
> - TypeArrayNest (later renamed to TypeIndexedArray)
> 
> In patch 5, I move code for a special variable used by one Type class,
> to be contained within that class. It makes it easier to ensure that the
> variable is only defined, when used, and vice versa. This comes at the
> cost of the generated code looking generated.

So you're agreeing?

> If we should make the generated code look like it was written by humans,
> then I would move the definition of these local variables into a class
> method, so `i` can be generated by the generic implementation, and `array`
> can be implemented in it's class. I will take a stab at this, but it might
> be too much refactoring for this series, eg. `len` is also defined local
> to conditional blocks multiple branches in a row.
> 
> tools/net/ynl/generated/nl80211-user.c:
> nl80211_iftype_data_attrs_parse(..) {
>    [..]
>    ynl_attr_for_each_nested(attr, nested) {
>      unsigned int type = ynl_attr_type(attr);
> 
>      if (type == NL80211_BAND_IFTYPE_ATTR_IFTYPES) {
>        unsigned int len;
>        [..]
>      } else if (type == NL80211_BAND_IFTYPE_ATTR_HE_CAP_MAC) {
>        unsigned int len;
>        [..]
>      [same pattern 8 times, so 11 times in total]
>      } else if (type == NL80211_BAND_IFTYPE_ATTR_EHT_CAP_PPE) {
>        unsigned int len;
>        [..]
>      }
>    }
>    return 0;
> }

It's pretty easily doable, I already gave up on not calling _attr_get()
for sub-messages.

> That looks very generated, I would have `len` defined together with `type`,
> and a switch statement would also look a lot more natural, but maybe leave
> the if->switch conversion for the compiler to detect.

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index fb7e03805a11..8a1f8a477566 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -243,7 +243,7 @@ from lib import SpecSubMessage, SpecSubMessageFormat
         raise Exception(f"Attr get not implemented for class type {self.type}")
 
     def attr_get(self, ri, var, first):
-        lines, init_lines, local_vars = self._attr_get(ri, var)
+        lines, init_lines, _ = self._attr_get(ri, var)
         if type(lines) is str:
             lines = [lines]
         if type(init_lines) is str:
@@ -251,10 +251,6 @@ from lib import SpecSubMessage, SpecSubMessageFormat
 
         kw = 'if' if first else 'else if'
         ri.cw.block_start(line=f"{kw} (type == {self.enum_name})")
-        if local_vars:
-            for local in local_vars:
-                ri.cw.p(local)
-            ri.cw.nl()
 
         if not self.is_multi_val():
             ri.cw.p("if (ynl_attr_validate(yarg, attr))")
@@ -2101,6 +2097,7 @@ _C_KW = {
             else:
                 raise Exception(f"Per-op fixed header not supported, yet")
 
+    var_set = set()
     array_nests = set()
     multi_attrs = set()
     needs_parg = False
@@ -2118,6 +2115,13 @@ _C_KW = {
             multi_attrs.add(arg)
         needs_parg |= 'nested-attributes' in aspec
         needs_parg |= 'sub-message' in aspec
+
+        try:
+            _, _, l_vars = aspec._attr_get(ri, '')
+            var_set |= set(l_vars) if l_vars else set()
+        except:
+            pass  # _attr_get() not implemented by simple types, ignore
+    local_vars += list(var_set)
     if array_nests or multi_attrs:
         local_vars.append('int i;')
     if needs_parg: