On Tue, 2022-06-07 at 17:24 -0700, Jeff Johnson wrote:
> > + struct nlattr *nested;
> > +
> > + nested = nla_nest_start(msg, NL80211_ATTR_MLO_LINKS);
> > + if (!nested)
> > + goto nla_put_failure;
> > +
> > + for_each_valid_link(cr, link) {
> > + struct nlattr *nested_mlo_links;
> > + const u8 *bssid = cr->links[link].bss ?
> > + cr->links[link].bss->bssid :
> > + cr->links[link].bssid;
> > +
> > + nested_mlo_links = nla_nest_start(msg, i + 1);
>
> why i+1?
> if you don't want to use 0 for the first entry (why not?) then init i to 1
>
> alternately since this is the only place i is used, use ++i or i++ as
> you see fit and remove the i++ later
>
> ultimately we should only need to calculate i+1 once
>
> these comments apply to nl80211_send_roamed() as well
>
> (BTW I do see this pattern in some of the existing code, but I also see
> 0 being used for the first record)
>
Yeah, we're not very consistent with this ...
The background is that this ends up being an attribute, so the structure
is
NL80211_ATTR_MLO_LINKS = {
1 = {
NL80211_ATTR_MLO_LINK_ID = <integer>,
...
},
...
}
so the "1" there is an attribute. Netlink says that attribute number 0
is invalid, and that's enforced if you were to try nla_parse_nested() or
so on the data of the NL80211_ATTR_MLO_LINKS attribute.
Now, nla_for_each_nested() doesn't care, and that's almost certainly
what gets used in the parser side of this, since the attribute number
doesn't even matter.
(We could use the link ID as the attribute number, but 0 is a valid link
ID and technically documented to not be a valid attribute number, so
...)
So I can see where this is coming from, and I don't really think it
matters. The compiler will almost certainly emit the same code anyway,
or at least I'd hope so :)
We probably have this pattern a few times ...
If we ignore the documentation further, we could even always use 0 for
the number, nla_for_each_nested() still won't care... but then we _try_
to do the right thing, except sometimes probably get it wrong with 0 vs.
1 starting.
Anyway, I guess it makes sense to just write it as
int i = 1;
...
nla_nest_start(msg, i);
instead, and we need a [PATCH] resubmit of this anyway I guess.
johannes
