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 for Android: free password hash cracker in your pocket

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

Message-ID: <4eda9c57-bde0-43c3-b8a0-3e45f2e672ac@fiberby.net>
Date: Sat, 6 Sep 2025 13:13:29 +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 12:18 AM, Jakub Kicinski wrote:
> On Thu,  4 Sep 2025 22:01:28 +0000 Asbjørn Sloth Tønnesen wrote:
>> Instead of trying to define "struct nlattr *array;" in the all
>> the right places, then simply define it in a block scope,
>> as it's only used here.
>>
>> Before this patch it was generated for attribute set _put()
>> functions, like wireguard_wgpeer_put(), but missing and caused a
>> compile error for the command function wireguard_set_device().
>>
>> $ make -C tools/net/ynl/generated wireguard-user.o
>> -e      CC wireguard-user.o
>> wireguard-user.c: In function ‘wireguard_set_device’:
>> wireguard-user.c:548:9: error: ‘array’ undeclared (first use in ..)
>>    548 |         array = ynl_attr_nest_start(nlh, WGDEVICE_A_PEERS);
>>        |         ^~~~~
> 
> Dunno about this one. In patch 4 you basically add another instance of
> the "let's declare local vars at function level" approach. And here
> you're going the other way. This patch will certainly work, but I felt
> like I wouldn't have written it this way if I was typing in the parsers
> by hand.

Thanks for the reviews.

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.

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;
}

(I didn't have to search for this, I saw the pattern in wireguard-user.c,
looked for it in nl80211-user.c and this was the first `len` usage there.)

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.