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

[Thomas Haller]

On Wed, 2015-11-25 at 11:14 -0800, David Ahern wrote:
> Add operation for requesting VLAN data for AF_BRIDGE and parsing of
> IFLA_AF_SPEC for AF_BRIDGE. VLANs are saved in a bitmap.
> 
> Also add dumping of vlan info to link list and neigh list.
> For example:
> 
> $ nl-link-list --details --family=bridge
> br1 ether 8e:6e:0e:86:e5:86 master br1
> <broadcast,multicast,up,running,lowerup>
>     mtu 1500 txqlen 0 weight 0 index 18
>     mode default carrier down
>     bridge: pvid 1   all vlans: 1 301-400 601-610   untagged vlans: 1
> bond1 ether 46:ef:e1:c9:46:fe <broadcast,multicast,master>
>     mtu 1500 txqlen 0 weight 0 index 20
>     state down mode default carrier down
>     bridge:
> 
> Signed-off-by: Wilson Kok <wkok at cumulusnetworks.com>
> Signed-off-by: David Ahern <dsa at cumulusnetworks.com>
> ---
>  include/linux-private/linux/if_bridge.h |  14 +++
>  include/linux-private/linux/rtnetlink.h |   6 +

when updating the kernel headers, we should copy an entire header
at once so that it is clear from which version it is taken.

Don't bother to do that, I'm just saying, before I merge the patch, I
will update the entire header file not only parts.



> 
> 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];
> +};
> +

all defines and declarations in public headers should have a distinct
prefix, "rtnl_" for libnl-route-3.so and "nl_" for libnl-3.so.




How about having the type as "uint32_t", instead of "unsigned long".
Leaving it undefined (at using C's long) only makes it more complicated
and I don't think we support an architecture where BITS_PER_BYTES isn't
8 (or uint32_t isn't 32 bits).


Also, the defines aren't really useful in the public API, because it's
clear the Vlan ids range from 0 to 4095. Even if one day that would
change (extremely unlikely), you couldn't just change the define
without breaking ABI. So users using the define don't get any actual
advantage.


How about only:

struct rtnl_bridge_vlan
{
	uint16_t                pvid;
 	uint32_t                vlan_bitmap[4096 / 32];
	uint32_t                untagged_bitmap[4096 / 32];
};






There are the internal utilities set_bit() and clear_bit().
clear_bit() is unused, but it seems they would be useful enough to
expose them in public API like:

int rtnl_bridge_vlan_bitmap_get(const uint32_t *bitmap, uint idx) {
     return idx < 4096 && ((bitmap[idx / 32] & ((uint32_t) 1 << (idx %
32))));
}

void rtnl_bridge_vlan_bitmap_set(uint32_t *bitmap, uint idx, int is_set) {
     ....
}



Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/libnl/attachments/20151202/fa977610/attachment.sig>