[PATCH 04/13] mbssid: get and set configuration parameters

[Jouni Malinen]

On Wed, Mar 02, 2022 at 02:26:25PM -0800, Aloka Dixit wrote:
> diff --git a/src/ap/beacon.c b/src/ap/beacon.c

> +static void hostapd_set_mbssid_beacon(struct hostapd_data *hapd,
> +				      struct wpa_driver_ap_params *params)
> +{
> +	struct hostapd_iface *iface = hapd->iface;
> +
> +	if (!iface->conf->mbssid || iface->num_bss == 1)
> +		return;
> +
> +	if (!iface->mbssid_max_interfaces) {
> +		wpa_printf(MSG_DEBUG,
> +			   "MBSSID: Driver doesn't support multi-BSSID advertisements");
> +		return;
> +	} else if (iface->num_bss > iface->mbssid_max_interfaces) {
> +		wpa_printf(MSG_DEBUG,
> +			   "MBSSID: Driver supports maximum %u interfaces for multi-BSSID advertisements",
> +			   iface->mbssid_max_interfaces);
> +		return;
> +	}

Can these be reached in practice? Shouldn't such cases be prevented from
starting AP operation?

> +	params->mbssid_tx_iface = hostapd_mbssid_get_tx_bss(hapd)->conf->iface;
> +	params->mbssid_index = hostapd_mbssid_get_bss_index(hapd);
> +	params->mbssid_count = iface->num_bss;
> +	return;
> +}

No "return;" at the end of void function.

> @@ -1149,7 +1175,6 @@ static u8 * hostapd_probe_resp_offloads(struct hostapd_data *hapd,
>  	/* Generate a Probe Response template for the non-P2P case */
>  	return hostapd_gen_probe_resp(hapd, NULL, 0, resp_len);
>  }
> -
>  #endif /* NEED_AP_MLME */
>  
>  

Please do not include unrelated (and in this case, an undesired
whitespace change in general) changes in the patch.

> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> @@ -1582,6 +1582,21 @@ struct wpa_driver_ap_params {
> +	/**
> +	 * mbssid_tx_iface - Transmitting interface of the set
> +	 */
> +	const char *mbssid_tx_iface;
> +
> +	/**
> +	 * mbssid_index - The index of this BSS in the group
> +	 */
> +	unsigned int mbssid_index;
> +
> +	/**
> +	 * mbssid_count - Total number of BSSs in the group
> +	 */
> +	unsigned int mbssid_count;
>  };

Why is that mbssid_count added here? It does not seem to be used in any
of the following patches.. Or well, it is just used to check whether
MBSSID mechanism is to be enabled and a single bool would seem to be a
clearer way of indicating that to cover the case of mbssid_index == 0.
Or well, I guess even that bool would be unnecessary since
mbssid_tx_iface != NULL would indicate if this is enabled for both the
transmitting BSSID and the nontransmitting ones.
 
-- 
Jouni Malinen                                            PGP id EFC895FA