[PATCH 08/16] WNM: Move driver MBO transition rejection into wnm_is_bss_excluded

[Benjamin Berg]

Hi,

looks like I initially only replied in private by accident.

On Fri, 2024-08-02 at 13:30 +0300, Jouni Malinen wrote:
> On Mon, Apr 29, 2024 at 01:51:49PM +0200, benjamin at sipsolutions.net wrote:
> > Change the logic a bit to not directly use the result of the
> > wpa_drv_get_bss_trans_status call and instead use the same selection
> > logic as usual but taking into account the driver rejections.
> > 
> > This changes the logic in minor ways. The main change is that this
> > aligns the ordering of BSSs to be identical in all cases. More
> > precisely, we'll select the best BSS as found by find_better_target.
> > 
> > Beyond that, it also means that in the case of an non-abridged BTM
> > request we'll also consider candidates that were found through the scan
> > and not in the neighbor report. In this case, the driver will not have a
> > chance to reject them.
> 
> This feels a bit scary especially if the updated design has not actually
> been tested with a driver that uses the driver-based mechanism for
> updating candidate lists (which I'm assuming has not been done here).

Yes, it is a bit scary. Honestly, I am not able to test the code. Maybe
I missed it, but I didn't even find a driver that provides the nl80211
API.

> And there is something strange here:
> 
> > diff --git a/wpa_supplicant/wnm_sta.c b/wpa_supplicant/wnm_sta.c
> >  #ifdef CONFIG_MBO
> > -static struct wpa_bss *
> > -get_mbo_transition_candidate(struct wpa_supplicant *wpa_s,
> > +static void
> > +fetch_drv_mbo_candidate_info(struct wpa_supplicant *wpa_s,
> >  			     enum mbo_transition_reject_reason *reason)
> >  {
> 
> > +		if (nei->preference_present && nei->preference == 0)
> >  			continue;
> > +
> > +		/* FIXME: Should we go through wpa_scan_res_match? */
> 
> Well, that is not the strange part, but again, something that makes me
> unhappy about applying this without full testing.
> 
> > +#else
> > +static void
> > +fetch_drv_mbo_transition_candidate_info(struct wpa_supplicant *wpa_s)
> > +{
> >  }
> >  #endif /* CONFIG_MBO */
> 
> That is the strange part.. What was that supposed to be? That same
> function with empty payload? Now the function name is different and so
> is the list of arguments. This would obviously not work without
> CONFIG_MBO defined.

Oops, yep, looks like I messed up the function rename and such.

> If this is just to skip the functionality within the function, defining
> the function once with the actually body within #ifdef CONFIG_MBO would
> be much cleaner.

Sure, that makes sense!

Benjamin