[PATCH 3/3] bridge: Add support for VLANs

[David Ahern]

On 11/25/15 8:47 AM, Tobias Jungel wrote:
>> diff --git a/include/netlink/route/link/bridge.h
>> b/include/netlink/route/link/bridge.h
>> index 16a4505f8b74..5dc6a367595c 100644
>> --- a/include/netlink/route/link/bridge.h
>> +++ b/include/netlink/route/link/bridge.h
>> @@ -19,6 +19,22 @@
>>   extern "C" {
>>   #endif
>>
>> +/* maximum vlan id */
>> +#define VLAN_N_VID		4096
>> +
>> +#define BITS_PER_BYTE		8
>> +#define BITS_PER_LONG		(BITS_PER_BYTE *
>> sizeof(unsigned long))
>> +#define DIV_ROUND_UP(n,d)	(((n) + (d) - 1) / (d))
>> +#define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_LONG)
>> +#define BR_VLAN_BITMAP_LEN	BITS_TO_LONGS(VLAN_N_VID)
>> +
>> +struct bridge_vlan
>> +{
>> +	uint16_t                pvid;
>> +	unsigned long           vlan_bitmap[BR_VLAN_BITMAP_LEN];
>> +	unsigned long           untagged_bitmap[BR_VLAN_BITMAP_LEN];
>> +};
>> +
>
> Fine with me, even if I personnaly would rather opt for the kernels
> compressed data structure, but this is probably just a matter of taste.

A bitmap has better lookup qualities as well as a consistent memory 
footprint. No idea why the kernel implementation is a list. Roopa?

-----8<-----

>> @@ -141,6 +185,112 @@ static int bridge_parse_protinfo(struct
>> rtnl_link *link, struct nlattr *attr,
>>   	return 0;
>>   }
>>
>> +static int bridge_parse_af(struct rtnl_link *link, struct nlattr
>> *attr,
>> +			   void *data)
>> +{
>> +	struct bridge_data *bd = data;
>> +	struct bridge_vlan_info *vinfo = NULL;
>> +
>> +	if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
>> +		return 0;
>> +
>> +	if (nla_len(attr) != sizeof(struct bridge_vlan_info))
>> +		return -EINVAL;
>> +
>> +	vinfo = nla_data(attr);
>> +	if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
>> +		return -EINVAL;
>> +
>> +	if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
>> +		NL_DBG(1, "Unexpected BRIDGE_VLAN_INFO_RANGE_BEGIN
>> flag; can not handle it.\n");
>> +		return -EINVAL;
>
> afaik RTEXT_FILTER_BRVLAN_COMPRESSED is the default for the kernel to
> send notifications, so I see this rather mandatory to have.

sure, can add that one in time. Right now the request below is for 
non-compressed, that's why it is marked as unexpected here. The 
complication with the compressed version is that it crosses consecutive 
attributes: start is in the first, end in the second.

-----8<-----

>> @@ -503,6 +666,44 @@ int rtnl_link_bridge_str2flags(const char *name)
>>
>>   /** @} */
>>
>> +int rtnl_link_bridge_pvid(struct rtnl_link *link)
>
> this function seems to be public, so its missing in the .sym file.
>
>> +{
>> +	struct bridge_data *bd = link->l_af_data[AF_BRIDGE];
>> +
>> +	if (bd->ce_mask & BRIDGE_ATTR_PORT_VLAN)
>> +		return (int) bd->vlan_info.pvid;
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +int rtnl_link_bridge_has_vlan(struct rtnl_link *link)
>
> same as above
>
>> +{
>> +	struct bridge_data *bd = link->l_af_data[AF_BRIDGE];
>> +	int i;
>> +
>> +	if (bd->ce_mask & BRIDGE_ATTR_PORT_VLAN) {
>> +		if (bd->vlan_info.pvid)
>> +			return 1;
>> +
>> +		for (i = 0; i < BR_VLAN_BITMAP_LEN; ++i) {
>> +			if (bd->vlan_info.vlan_bitmap[i] ||
>> +			    bd->vlan_info.untagged_bitmap[i])
>> +				return 1;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +struct bridge_vlan *rtnl_bridge_get_port_vlan(struct rtnl_link
>> *link)
>
> same as above and I guess the function should be renamed to
> rtnl_link_bridge_get_port_vlan
>

ack on all 3. Will fix in the next version.

Thanks for the review.