[PATCHv3 2/6] vlan: transform untagged/tagged vlan description into vlan_id

[Jouni Malinen]

On Thu, Apr 11, 2013 at 11:52:40AM +0200, michael-dev at fami-braun.de wrote:
> diff --git a/src/ap/vlan_init.c b/src/ap/vlan_init.c
> @@ -35,6 +35,18 @@
> +#ifdef CONFIG_VLAN_TAGGED
> +struct vlan_info_list {
> +       unsigned int counter;
> +       unsigned int size;
> +       struct vlan_info *vlan;
> +};
> +static struct vlan_info_list vlan_mapping;
> +#endif /* CONFIG_VLAN_TAGGED */

A static variable for this? How does this work if hostapd controls
multiple radios? Shouldn't this be somewhere in struct hostapd_data or
struct hostapd_iface? Or struct hapd_interfaces?

> +/* make tagged vlans unique and sorted */
> +void vlan_cleanup(struct vlan_info *a) {

'{' should on its own as another line when defining functions to match
coding style.

> +	int i, offset=0;

"offset = 0"

> +	qsort(a->tagged, a->num_tagged, sizeof(int), cmp_vlan);
> +	for (i=1; i < a->num_tagged; i++) {

"i = 1"

> +		if (a->tagged[i-1-offset] == a->tagged[i])

"i - 1 - offset"

> +int vlan_equals(struct vlan_info *a, struct vlan_info *b) {
> +	int i;
> +	if (!a && !b) return 1;

"return 1;" should not be on the same line with "if"

> +int vlan_get_id(struct vlan_info* a) {
> +	int i;
> +
> +	/* no tagged/untagged vlan => vlan_id = 0 */
> +	if (!a) return 0;
> +	if (!a->untagged && !a->num_tagged) return 0;
> +
> +	/* all other untagged/tagged configurations get vlan_id > 0 */
> +	if(vlan_mapping.counter > vlan_mapping.size) {

"if (vlan..."

> +		wpa_printf(MSG_ERROR, "VLAN: internal mapping data structure is corrupt.");
> +		return -1;
> +	}
> +	for (i=0; i < vlan_mapping.counter; i++)
> +		if (vlan_equals(a, &vlan_mapping.vlan[i]))
> +			return i + 1;
> +	/* resize cache, amortized O(1) */
> +	if (vlan_mapping.size == vlan_mapping.counter) {
> +		unsigned int newsize = 2 * vlan_mapping.size;
> +		if (newsize == 0)
> +			newsize = 1;
> +		if (newsize < vlan_mapping.size) /* overflow */
> +			return -1;
> +		struct vlan_info *new_vlan = os_malloc(sizeof(struct vlan_info) * newsize);

Need to declare variables in the beginning of the block.

> +		if (!new_vlan)
> +			return -1;
> +		if (vlan_mapping.size > 0)
> +			os_memcpy(new_vlan, vlan_mapping.vlan, sizeof(struct vlan_info) * vlan_mapping.size);
> +		os_free(vlan_mapping.vlan);
> +		vlan_mapping.vlan = new_vlan; new_vlan = NULL;

That should be two separate lines.

This function seems to be allocating memory and leaving those pointers
into the vlan_mapping. Where are these freed?

> diff --git a/src/ap/vlan_init.h b/src/ap/vlan_init.h
> @@ -17,6 +17,12 @@
>  #define VLAN_INIT_H
>  
>  #ifndef CONFIG_NO_VLAN
> +struct vlan_info {
> +	int untagged;
> +	unsigned int num_tagged;
> +	int* tagged;

"int *tagged"

-- 
Jouni Malinen                                            PGP id EFC895FA