netdev - 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: <3b52386e-6127-4bdc-b7db-e3c885b03f72@fiberby.net>
Date: Thu, 11 Sep 2025 00:01:48 +0000
From: Asbjørn Sloth Tønnesen <ast@...erby.net>
To: Jakub Kicinski <kuba@...nel.org>
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 9/6/25 7:07 PM, Jakub Kicinski wrote:
> 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:
I left this for you to submit, there is a trivial conflict with patch 8
in my v2 posting.

It gives a pretty nice diffstat when comparing the generated code:

  devlink-user.c      |  187 +++----------------
  dpll-user.c         |   10 -
  ethtool-user.c      |   49 +----
  fou-user.c          |    5
  handshake-user.c    |    3
  mptcp_pm-user.c     |    3
  nfsd-user.c         |   16 -
  nl80211-user.c      |  159 +---------------
  nlctrl-user.c       |   21 --
  ovpn-user.c         |    7
  ovs_datapath-user.c |    9
  ovs_flow-user.c     |   89 ---------
  ovs_vport-user.c    |    7
  rt-addr-user.c      |   14 -
  rt-link-user.c      |  183 ++----------------
  rt-neigh-user.c     |   14 -
  rt-route-user.c     |   26 --
  rt-rule-user.c      |   11 -
  tc-user.c           |  380 +++++----------------------------------
  tcp_metrics-user.c  |    7
  team-user.c         |    5
  21 files changed, 175 insertions(+), 1030 deletions(-)