Search Linux Wireless

Re: [RFC v2] cfg80211: Indicate MLO connection info in connect and roam callbacks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 6/8/2022 12:53 PM, Johannes Berg wrote:
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


Thanks Jeff and Johannes for the comments. I will address the comments and will resend it as [PATCH]




[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux