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

["Asbjørn Sloth Tønnesen" <ast@fiberby.net>]

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.