[PATCHv2 01/12] Separate station grouping and uplink configuration

[Jouni Malinen]

On Sun, Dec 06, 2015 at 09:47:32PM +0100, Michael Braun wrote:
> diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c
> @@ -1334,7 +1334,9 @@ static int hostapd_ctrl_iface_set(struct hostapd_data *hapd, char *cmd)
> -				    (!vlan_id || vlan_id == sta->vlan_id))
> +				    (!vlan_id.notempty ||
> +				     !os_memcmp(&vlan_id, &sta->vlan_desc,
> +						sizeof(vlan_id))))

This style of comparing structures with memcmp does not look safe. While
most compilers are unlikely to pad the two int variables in the struct
in this first patch, they would be allowed to do so. Wouldn't it be
clearer to add a helper function for comparing two instances of struct
vlan_description?

> diff --git a/src/ap/ap_config.c b/src/ap/ap_config.c
> -int hostapd_vlan_id_valid(struct hostapd_vlan *vlan, int vlan_id)
> +int hostapd_vlan_id_valid(struct hostapd_vlan *vlan,
> +			  struct vlan_description vlan_id)

Why would this pass a copy of the full struct instead of just a pointer
to the data? It looks unnecessary to copy all the data for each function
call.

> diff --git a/src/ap/pmksa_cache_auth.h b/src/ap/pmksa_cache_auth.h
> @@ -28,7 +28,7 @@ struct rsn_pmksa_cache_entry {
> -	int vlan_id;
> +	struct vlan_description vlan_id;

Does this really need a full copy of the struct data (272 bytes on
64-bit builds at the end of this patch series) for each PMKSA cache
entry?

> +int ap_sta_set_vlan(struct hostapd_data *hapd, struct sta_info *sta,
> +		    struct vlan_description vlan_desc)

That same comment about function calls using a full copy vs. pointer..

> diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h
> @@ -44,8 +45,10 @@
>   * Supported Rates IEs). */
>  #define WLAN_SUPP_RATES_MAX 32
>  
> +struct hostapd_data;
>  
>  struct sta_info {
> +	struct hostapd_data *bss; /* required for set vlan in preauth */

Cannot say that I'm exactly fond of adding sta->bss pointer, but that's
small compared to this:

> @@ -119,6 +122,7 @@ struct sta_info {
>  	int vlan_id; /* 0: none, >0: VID */
> +	struct vlan_description vlan_desc;

Does this really need that full copy of the 272 byte structure for each
STA entry? Why would this not be a pointer to the data stored in
interface configuration?

-- 
Jouni Malinen                                            PGP id EFC895FA