From: roopa <roopa@cumulusnetworks•com>
To: "Arad, Ronen" <ronen.arad@intel•com>
Cc: "netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"hemminger@vyatta•com" <hemminger@vyatta•com>,
"vyasevic@redhat•com" <vyasevic@redhat•com>,
"sfeldma@gmail•com" <sfeldma@gmail•com>,
"wkok@cumulusnetworks•com" <wkok@cumulusnetworks•com>
Subject: Re: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges
Date: Thu, 01 Jan 2015 02:01:34 -0800 [thread overview]
Message-ID: <54A51AFE.2010005@cumulusnetworks.com> (raw)
In-Reply-To: <E4CD12F19ABA0C4D8729E087A761DC3505DD2DE4@ORSMSX101.amr.corp.intel.com>
On 1/1/15, 12:54 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: Wednesday, December 31, 2014 6:49 PM
>> To: netdev@vger•kernel.org; hemminger@vyatta•com; vyasevic@redhat•com
>> Cc: sfeldma@gmail•com; wkok@cumulusnetworks•com; Roopa Prabhu
>> Subject: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to
>> accomodate vlan list and ranges
>>
>> From: Roopa Prabhu <roopa@cumulusnetworks•com>
>>
>> This patch modifies br_afspec to parse incoming IFLA_BRIDGE_VLAN_INFO_LIST
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks•com>
>> ---
>> net/bridge/br_netlink.c | 115 ++++++++++++++++++++++++++++++++++------------
>> -
>> 1 file changed, 85 insertions(+), 30 deletions(-)
>>
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 492ef6a..bcba9d2 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -226,53 +226,108 @@ static const struct nla_policy
>> ifla_br_policy[IFLA_MAX+1] = {
>> [IFLA_BRIDGE_VLAN_INFO_LIST] = { .type = NLA_NESTED, },
>> };
>>
>> +static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p,
>> + int cmd, struct bridge_vlan_info *vinfo)
>> +{
>> + int err = 0;
>> +
>> + switch (cmd) {
>> + case RTM_SETLINK:
>> + if (p) {
>> + err = nbp_vlan_add(p, vinfo->vid, vinfo->flags);
>> + 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);
>> + }
>> + break;
>> + }
>> +
>> + return err;
>> +}
>> +
>> static int br_afspec(struct net_bridge *br,
>> struct net_bridge_port *p,
>> struct nlattr *af_spec,
>> int cmd)
>> {
>> struct nlattr *tb[IFLA_BRIDGE_MAX+1];
>> + struct nlattr *attr;
>> int err = 0;
>> + int rem;
>>
>> err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, ifla_br_policy);
>> if (err)
>> return err;
>>
>> if (tb[IFLA_BRIDGE_VLAN_INFO]) {
>> - struct bridge_vlan_info *vinfo;
>> -
>> - vinfo = nla_data(tb[IFLA_BRIDGE_VLAN_INFO]);
>> -
>> - if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
>> - return -EINVAL;
>> -
>> - switch (cmd) {
>> - case RTM_SETLINK:
>> - if (p) {
>> - err = nbp_vlan_add(p, vinfo->vid, vinfo->flags);
>> - 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);
>> - break;
>> + attr = tb[IFLA_BRIDGE_VLAN_INFO];
>> + if (nla_len(attr) != sizeof(struct bridge_vlan_info))
>> + goto err_inval;
>> +
>> + err = br_vlan_info(br, p, cmd,
>> + (struct bridge_vlan_info *)nla_data(attr));
>> +
>> + } else if (tb[IFLA_BRIDGE_VLAN_INFO_LIST]) {
>> + struct bridge_vlan_info *vinfo_start = NULL;
>> + struct bridge_vlan_info *vinfo = NULL;
>> +
>> + nla_for_each_nested(attr, tb[IFLA_BRIDGE_VLAN_INFO_LIST], rem) {
>> + if (nla_len(attr) != sizeof(struct bridge_vlan_info) ||
>> + nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
>> + goto err_inval;
>> + vinfo = nla_data(attr);
>> + if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_START) {
>> + if (vinfo_start)
>> + goto err_inval;
>> + vinfo_start = vinfo;
>> + continue;
>> + }
>> +
>> + if (vinfo_start) {
>> + int v;
>> +
>> + if (!(vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END))
>> + goto err_inval;
>> +
>> + if (vinfo->vid < vinfo_start->vid)
> This check rejects inverted range. However it allows the RANGE_START and
> RANGE_END vinfos to have the same vid. Isn't it inconsistent with the rejection
> of a single vinfo with both RANGE_START and RANGE_END set?
sure, i will make it <= to error out if they are equal.
>> + goto err_inval;
> Are additional validation such as consistency of flags between the RANGE_START
> and RANGE_END vinfos is needed here?
sure, i can add them (I have some of them in the iproute2 patch as well).
> The loop below applies flags (more precisely all data except for vid) from the
> RANGE_START vinfo to all vids in the range. All data from the RANGE_END vinfo
> is ignored.
>
>> +
>> + for (v = vinfo_start->vid; v <= vinfo->vid;
>> + v++) {
>> + vinfo_start->vid = v;
> This changes the vinfo with RANGE_START flag within the incoming message. Would
> it be better to left the input message unmodified and use a local copy of
> struct bridge_vlan_info?
agreed, I will make a copy.
>> + err = br_vlan_info(br, p, cmd,
>> + vinfo_start);
>> + if (err)
>> + break;
>> + }
>> + vinfo_start = NULL;
>> + } else {
>> + err = br_vlan_info(br, p, cmd, vinfo);
>> + }
>> + if (err)
>> + break;
>> }
>> }
>>
>> return err;
>> +
>> +err_inval:
>> + return -EINVAL;
>> }
>>
>> static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
>> --
>> 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
next prev parent reply other threads:[~2015-01-01 10:01 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-31 16:48 [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges roopa
2015-01-01 8:54 ` Arad, Ronen
2015-01-01 10:01 ` roopa [this message]
2015-01-01 19:34 ` Scott Feldman
2015-01-03 8:39 ` roopa
2015-01-01 19:20 ` Scott Feldman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54A51AFE.2010005@cumulusnetworks.com \
--to=roopa@cumulusnetworks$(echo .)com \
--cc=hemminger@vyatta$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=ronen.arad@intel$(echo .)com \
--cc=sfeldma@gmail$(echo .)com \
--cc=vyasevic@redhat$(echo .)com \
--cc=wkok@cumulusnetworks$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox