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]
