Re: [PATCH 3/6] bridge: modify IFLA_AF_SPEC parser to parse IFLA_BRIDGE_VLAN_RANGE_INFO

[roopa <roopa@cumulusnetworks.com>]

On 12/30/14, 12:40 AM, Arad, Ronen wrote:
>
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
>> Behalf Of roopa@cumulusnetworks.com
>> Sent: Monday, December 29, 2014 11:05 PM
>> To: netdev@vger.kernel.org; shemminger@vyatta.com; vyasevic@redhat.com
>> Cc: Roopa Prabhu; Wilson kok
>> Subject: [PATCH 3/6] bridge: modify IFLA_AF_SPEC parser to parse
>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> This patch modifies br_afspec to parse incoming IFLA_BRIDGE_VLAN_RANGE_INFO
>>
>> Signed-off-by: Wilson kok <wkok@cumulusnetworks.com>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>> net/bridge/br_netlink.c |   70 +++++++++++++++++++++++++++++++++-------------
>> -
>> 1 file changed, 49 insertions(+), 21 deletions(-)
>>
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index e7d1fc0..4c47ba0 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -227,48 +227,76 @@ static const struct nla_policy
>> ifla_br_policy[IFLA_MAX+1] = {
>> 			 .len = sizeof(struct bridge_vlan_range_info), },
>> };
>>
>> +static int br_afspec_vlan_add(struct net_bridge *br,
>> +			      struct net_bridge_port *p,
>> +			      u16 vid, u16 flags)
>> +{
>> +	int err = 0;
>> +
>> +	if (p) {
>> +		err = nbp_vlan_add(p, vid, flags);
>> +		if (err)
>> +			return err;
>> +
>> +		if (flags & BRIDGE_VLAN_INFO_MASTER)
>> +			err = br_vlan_add(p->br, vid, flags);
>> +	} else {
>> +		err = br_vlan_add(br, vid, flags);
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static void br_afspec_vlan_del(struct net_bridge *br,
>> +			       struct net_bridge_port *p,
>> +			       u16 vid, u16 flags)
>> +{
>> +	if (p) {
>> +		nbp_vlan_delete(p, vid);
>> +		if (flags & BRIDGE_VLAN_INFO_MASTER)
>> +			br_vlan_delete(p->br, vid);
>> +	} else {
>> +		br_vlan_delete(br, vid);
>> +	}
>> +}
>> +
>> static int br_afspec(struct net_bridge *br,
>> 		     struct net_bridge_port *p,
>> 		     struct nlattr *af_spec,
>> 		     int cmd)
>> {
>> -	struct bridge_vlan_info *vinfo;
>> -	int err = 0;
>> +	struct bridge_vlan_range_info *vinfo;
>> 	struct nlattr *attr;
>> 	int err = 0;
>> 	int rem;
>> 	u16 vid;
>>
>> 	nla_for_each_nested(attr, af_spec, rem) {
>> -		if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
>> +		if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO &&
>> +		    nla_type(attr) != IFLA_BRIDGE_VLAN_RANGE_INFO)
>> 			continue;
>> -
>> 		vinfo = nla_data(attr);
>> -		if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
>> +
>> +		if (nla_type(attr) == IFLA_BRIDGE_VLAN_INFO)
>> +			vinfo->vid_end = vinfo->vid;
>> +
>> +		if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK ||
>> +		    vinfo->vid_end >= VLAN_VID_MASK ||
>> +		    vinfo->vid > vinfo->vid_end)
>> 			return -EINVAL;
>>
>> 		switch (cmd) {
>> 		case RTM_SETLINK:
>> -			if (p) {
>> -				err = nbp_vlan_add(p, vinfo->vid, vinfo->flags);
>> +			for (vid = vinfo->vid; vid <= vinfo->vid_end; vid++) {
>> +				err = br_afspec_vlan_add(br, p, vid,
>> +							 vinfo->flags);
> vinfo->flags could have BRIDGE_VLAN_INFO_PVID set. It is really a port property and there could only be a single PVID for a port. The loop will make the port pvid set, in turn, to each vid in the range until it finally set to vid_end.
> This could be avoided by turning off this flag for all but the last vid in range:
>
> 				err = br_afspec_vlan_addr(br, p, vid,
>                                                      ((vid == vinfo->vid_end)
>                                                       ? vinfo->flags
>                                                       : vinfo->flags & ~BRIDGE_VLAN_INFO_PVID));
> Another alternative could be to add explicit pvid field to bridge_vlan_range_info and disallow PVID flag.
> This allows for setting the port pvid to any vid in the range.
>
> If both alternatives seem somewhat complex we could just disallow PVID flag in IFLA_BRIDGE_VLAN_RANGE_INFO and allow it only in IFLA_BRIDGE_VLAN_INFO.

I believe it is a user error to set PVID flag on a range of vlans using 
IFLA_BRIDGE_VLAN_RANGE_INFO
or for that matter using multiple IFLA_BRIDGE_VLAN_INFO with pvid flag 
set. Today, the last one probably sticks.
I will check current behavior and mimic that or return EINVAL when 
multiple attributes come in with the PVID flag.



>
>   
>> 				if (err)
>> 					break;
>> -
>> -				if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER)
>> -					err = br_vlan_add(p->br, vinfo->vid,
>> -							  vinfo->flags);
>> -			} else
>> -				err = br_vlan_add(br, vinfo->vid, vinfo->flags);
>> -
>> +			}
>> 			break;
>> -
>> 		case RTM_DELLINK:
>> -			if (p) {
>> -				nbp_vlan_delete(p, vinfo->vid);
>> -				if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER)
>> -					br_vlan_delete(p->br, vinfo->vid);
>> -			} else
>> -				br_vlan_delete(br, vinfo->vid);
>> +			for (vid = vinfo->vid; vid <= vinfo->vid_end; vid++)
>> +				br_afspec_vlan_del(br, p, vid, vinfo->flags);
>> 			break;
>> 		}
>> 	}
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html