* [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges
@ 2014-12-31 16:48 roopa
2015-01-01 8:54 ` Arad, Ronen
2015-01-01 19:20 ` Scott Feldman
0 siblings, 2 replies; 6+ messages in thread
From: roopa @ 2014-12-31 16:48 UTC (permalink / raw)
To: netdev, hemminger, vyasevic; +Cc: sfeldma, wkok, Roopa Prabhu
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)
+ goto err_inval;
+
+ for (v = vinfo_start->vid; v <= vinfo->vid;
+ v++) {
+ vinfo_start->vid = v;
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges
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
2015-01-01 19:34 ` Scott Feldman
2015-01-01 19:20 ` Scott Feldman
1 sibling, 2 replies; 6+ messages in thread
From: Arad, Ronen @ 2015-01-01 8:54 UTC (permalink / raw)
To: roopa@cumulusnetworks.com, netdev@vger.kernel.org,
hemminger@vyatta.com, vyasevic@redhat.com
Cc: sfeldma@gmail.com, wkok@cumulusnetworks.com
>-----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?
>+ goto err_inval;
Are additional validation such as consistency of flags between the RANGE_START
and RANGE_END vinfos is needed here?
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?
>+ 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges
2015-01-01 8:54 ` Arad, Ronen
@ 2015-01-01 10:01 ` roopa
2015-01-01 19:34 ` Scott Feldman
1 sibling, 0 replies; 6+ messages in thread
From: roopa @ 2015-01-01 10:01 UTC (permalink / raw)
To: Arad, Ronen
Cc: netdev@vger.kernel.org, hemminger@vyatta.com, vyasevic@redhat.com,
sfeldma@gmail.com, wkok@cumulusnetworks.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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges
2015-01-01 8:54 ` Arad, Ronen
2015-01-01 10:01 ` roopa
@ 2015-01-01 19:34 ` Scott Feldman
2015-01-03 8:39 ` roopa
1 sibling, 1 reply; 6+ messages in thread
From: Scott Feldman @ 2015-01-01 19:34 UTC (permalink / raw)
To: Arad, Ronen
Cc: roopa@cumulusnetworks.com, netdev@vger.kernel.org,
hemminger@vyatta.com, vyasevic@redhat.com,
wkok@cumulusnetworks.com
On Thu, Jan 1, 2015 at 12:54 AM, Arad, Ronen <ronen.arad@intel.com> 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?
Allowing both START and END to be set on single vinfo might simplify
the encoding of LIST, so maybe it should be allowed.
Roopa, I know you dropped the subsequent notification patches from the
set, but I suspect now with these new START/END markers, the
notification algorithm can be very close to the original loop, without
having to make copies of the vlan_bitmap and untagged_bitmap. Using
both START/END on a single vinfo will keep the loop simple for adding
single vids that are not in a range.
(hmmm...START/STOP or BEGIN/END? Seems START/END is mixing the two
concepts...BEGIN/END seems best)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges
2015-01-01 19:34 ` Scott Feldman
@ 2015-01-03 8:39 ` roopa
0 siblings, 0 replies; 6+ messages in thread
From: roopa @ 2015-01-03 8:39 UTC (permalink / raw)
To: Scott Feldman
Cc: Arad, Ronen, netdev@vger.kernel.org, hemminger@vyatta.com,
vyasevic@redhat.com, wkok@cumulusnetworks.com
On 1/1/15, 11:34 AM, Scott Feldman wrote:
> On Thu, Jan 1, 2015 at 12:54 AM, Arad, Ronen <ronen.arad@intel.com> 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?
> Allowing both START and END to be set on single vinfo might simplify
> the encoding of LIST, so maybe it should be allowed.
sorry, i did not understand this. How does it help ?.
>
> Roopa, I know you dropped the subsequent notification patches from the
> set, but I suspect now with these new START/END markers, the
> notification algorithm can be very close to the original loop, without
> having to make copies of the vlan_bitmap and untagged_bitmap.
> Using
> both START/END on a single vinfo will keep the loop simple for adding
> single vids that are not in a range.
The way i am seeing it is, both, making copies of vlan_bitmap and using
the existing loop can be used
with both old and the new proposed netlink attributes (unless i am
missing something).
Our original in-house code used copies of vlan bitmap (not authored by
me) and it was well tested,
so it was safer to post that code in terms of testing effort.
But during the course of this review, i do realize that the existing
loop can be used. And now i see that the existing loop
can be used with both the old proposed and the new netlink formats. I
have coded up the patches. In the process of testing it. Will post the
new series soon. Maybe you will spot something.
>
> (hmmm...START/STOP or BEGIN/END? Seems START/END is mixing the two
> concepts...BEGIN/END seems best)
Sure, no preference there. I will use BEGIN/END.
Thanks,
Roopa
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges
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 19:20 ` Scott Feldman
1 sibling, 0 replies; 6+ messages in thread
From: Scott Feldman @ 2015-01-01 19:20 UTC (permalink / raw)
To: Roopa Prabhu; +Cc: Netdev, hemminger, vyasevic@redhat.com, Wilson Kok
On Wed, Dec 31, 2014 at 8:48 AM, <roopa@cumulusnetworks.com> wrote:
> 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;
Size check isn't necessary here as it was already done with nla_parse_nested().
> +
> + err = br_vlan_info(br, p, cmd,
> + (struct bridge_vlan_info *)nla_data(attr));
> +
> + } else if (tb[IFLA_BRIDGE_VLAN_INFO_LIST]) {
Could you have both IFLA_BRIDGE_VLAN_INFO and
IFLA_BRIDGE_VLAN_INFO_LIST? If so, drop the else.
> + struct bridge_vlan_info *vinfo_start = NULL;
> + struct bridge_vlan_info *vinfo = NULL;
Initializer not needed on this ^^^ one.
> +
> + 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;
goto isn't necessary...just return -EINVAL...more like this below
> + 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)
> + goto err_inval;
> +
> + for (v = vinfo_start->vid; v <= vinfo->vid;
> + v++) {
Did the above exceed 80 chars? If not, one liner.
> + vinfo_start->vid = v;
> + 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-01-03 8:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2015-01-01 19:34 ` Scott Feldman
2015-01-03 8:39 ` roopa
2015-01-01 19:20 ` Scott Feldman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).