Re: [PATCH 1/2] nl80211: Use different attrs for BSSID and random MAC addr in scan req

[Johannes Berg <johannes@sipsolutions.net>]

> > + * @NL80211_ATTR_BSSID: The BSSID of the AP (various uses). Note
> > that
> 
> The BSSID may also be used for other things, like P2P GO, right? Also
> "various uses" is probably unnecessary? Every command using this
> attribute should describe it's use in their description.
> 
> 
> > 
> > + *	%NL80211_ATTR_MAC has also been used in various
> > commands/events for
> > + *	specifying the BSSID.
> 
> This can be a bit confusing.  Maybe you can specify which commands
> *used* to use NL80211_ATTR_MAC but now use NL80211_ATTR_BSSID?

I'd actually avoid that, let's not make the "compatibility quirks" part
of the documentation people read through normally ... In the code,
that's fine, but here, I think less so.

With that, perhaps just rephrase this to

"The BSSID of the AP. Note that %NL80211_ATTR_MAC is also used in
various commands/events for specifying the BSSID."

> > -	if (info->attrs[NL80211_ATTR_MAC])
> > +	/* Initial implementation used NL80211_ATTR_MAC to set the
> > specific
> > +	 * BSSID to scan for. This was problematic because that
> > same attribute
> > +	 * was already used for another purpose (local random MAC
> > address). The
> > +	 * NL80211_ATTR_BSSID attribute was added to fix this. For
> > backwards
> > +	 * compatibility with older userspace components, also use
> > the
> > +	 * NL80211_ATTR_MAC value here if it can be determined to
> > be used for
> > +	 * the specific BSSID use case instead of the random MAC
> > address
> > +	 * (NL80211_ATTR_SCAN_FLAGS is used to enable random MAC
> > address use).
> > +	 */
> 
> You should probably add this information to the
> NL80211_CMD_TRIGGER_SCAN description.

It may need an update to refer to ATTR_BSSID, but again I don't think
all the compatibility discussion should be there.

> 
> > 
> > +	if (info->attrs[NL80211_ATTR_BSSID])
> > +		memcpy(request->bssid,
> > +		       nla_data(info->attrs[NL80211_ATTR_BSSID]),
> > ETH_ALEN);
> > +	else if (!info->attrs[NL80211_ATTR_SCAN_FLAGS] &&
> 
> You should actually check that the SCAN_FLAGS attribute either
> doesn't
> exist (as you already do) or, if it exists, that it doesn't have the
> NL80211_SCAN_FLAG_RANDOM_ADDR flags.
> 

Agree with that, that would make sense.

johannes