[PATCH 09/18] wpa_supplicant: Always propagate scan results to all interfaces

[Jouni Malinen]

On Mon, Sep 05, 2016 at 05:33:02PM +0300, andrei.otcheretianski at intel.com wrote:
> Scan results are not propogated to all interfaces if scan results
> started a new operation, in order to prevent concurrent operations.
> But this causes other interfaces to trigger a new scan when scan
> results are already available.
> Instead, always notify other interfaces of the scan results, but note
> that new operations are not allowed.

> diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
> -/* Return != 0 if no scan results could be fetched or if scan results should not
> - * be shared with other virtual interfaces. */
> +/*
> + * Return a negative value if no scan results could be fetched or if scan
> + * Return 0 if scan results were fetched and may be shared with other interfaces.
> + * results should not be shared with other virtual interfaces.
> + * Return 1 if scan results may be shared with other virtual interfaces but may not
> + * trigger any operations.
> + * Return 2 if the interface was removed and cannot no be used.
> + */

This new comment looks pretty strange.. Is that first " or if scan"
supposed to be replace with "." for the negative value? And is the
"results should not be shared with other virtual interfaces." line
supposed to be deleted?

>  static int _wpa_supplicant_event_scan_results(struct wpa_supplicant *wpa_s,
>  					      union wpa_event_data *data,
> -					      int own_request)
> +					      int own_request, int update_only)
>  {
>  	struct wpa_scan_results *scan_res = NULL;
>  	int ret = 0;
> @@ -1521,6 +1527,17 @@ static int _wpa_supplicant_event_scan_results(struct wpa_supplicant *wpa_s,
>  	}
>  #endif /* CONFIG_NO_RANDOM_POOL */
>  
> +	if (update_only) {
> +		/*
> +		 * When updating scan results from another interface's scan and
> +		 * new operations are not allowed, don't cancel this interface
> +		 * scan work because it may be needed to trigger some operations
> +		 * on this interface.
> +		 */
> +		wpa_scan_results_free(scan_res);
> +		return 1;
> +	}

Which scan work would this be referring to? scan_work_done is checking
for own_request and that's set only for the actual interface that did
the scan. Surely that one should be canceled. In fact, I don't see how
this function could be called with own_request == 0 and update_only !=
1, so it would seem to make sense to do ret = 1; goto scan_work_done;
here and get rid of this comment to avoid adding new return paths.

> @@ -1750,7 +1767,7 @@ static int wpa_supplicant_event_scan_results(struct wpa_supplicant *wpa_s,
> -	res = _wpa_supplicant_event_scan_results(wpa_s, data, 1);
> +	res = _wpa_supplicant_event_scan_results(wpa_s, data, 1, 0);

So this is the initial call for the interface that did the scan. As
noted above, this is the one that gets own_request == 1 and update_only
= 0. All other interfaces use own_request == 0.

> @@ -1778,7 +1796,8 @@ static int wpa_supplicant_event_scan_results(struct wpa_supplicant *wpa_s,
>  		if (ifs != wpa_s) {
>  			wpa_printf(MSG_DEBUG, "%s: Updating scan results from "
>  				   "sibling", ifs->ifname);
> -			_wpa_supplicant_event_scan_results(ifs, data, 0);
> +			_wpa_supplicant_event_scan_results(ifs, data, 0,
> +							   res > 0 ? 1 : 0);
>  		}
>  	}

That "res > 0 ? 1 : 0" should be replaced with "res > 0"..

This loop does not update res, though. Shouldn't we set update_only == 1
if any of these sibling interfaces starts some kind of action based on
the scan results? In other words, only one of the virtual interfaces
sharing the same radio should be allowed to start an operation based on
a single scan results event.
 
-- 
Jouni Malinen                                            PGP id EFC895FA