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);
| ^~~~~
Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
tools/net/ynl/pyynl/ynl_gen_c.py | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index b0eeedfca2f2..e6a84e13ec0a 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -842,6 +842,9 @@ class TypeArrayNest(Type):
return get_lines, None, local_vars
def attr_put(self, ri, var):
+ ri.cw.block_start()
+ ri.cw.p('struct nlattr *array;')
+ ri.cw.nl()
ri.cw.p(f'array = ynl_attr_nest_start(nlh, {self.enum_name});')
if self.sub_type in scalars:
put_type = self.sub_type
@@ -857,6 +860,7 @@ class TypeArrayNest(Type):
else:
raise Exception(f"Put for ArrayNest sub-type {self.attr['sub-type']} not supported, yet")
ri.cw.p('ynl_attr_nest_end(nlh, array);')
+ ri.cw.block_end()
def _setter_lines(self, ri, member, presence):
return [f"{member} = {self.c_name};",
@@ -2063,13 +2067,9 @@ def put_req_nested(ri, struct):
init_lines.append(f"hdr = ynl_nlmsg_put_extra_header(nlh, {struct_sz});")
init_lines.append(f"memcpy(hdr, &obj->_hdr, {struct_sz});")
- has_anest = False
has_count = False
for _, arg in struct.member_list():
- has_anest |= arg.type == 'indexed-array'
has_count |= arg.presence_type() == 'count'
- if has_anest:
- local_vars.append('struct nlattr *array;')
if has_count:
local_vars.append('unsigned int i;')
--
2.51.0
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.
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.
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:
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(-)
On Thu, 11 Sep 2025 00:01:48 +0000 Asbjørn Sloth Tønnesen wrote: > I left this for you to submit, there is a trivial conflict with patch 8 > in my v2 posting. Hah, no, please take this and submit as yours if you can. You can add me as Suggested-by. I already dropped it form my tree. Maintainers tend to throw random snippets of code at people, it's just quicker than explaining but we have enough commits in our name..
Asbjørn Sloth Tønnesen <ast@fiberby.net> writes: > 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); > | ^~~~~ > > Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
© 2016 - 2025 Red Hat, Inc.