* [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC @ 2014-12-29 21:05 roopa 2014-12-29 21:40 ` Scott Feldman 0 siblings, 1 reply; 9+ messages in thread From: roopa @ 2014-12-29 21:05 UTC (permalink / raw) To: netdev, shemminger, vyasevic; +Cc: Roopa Prabhu From: Roopa Prabhu <roopa@cumulusnetworks.com> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows userspace to pack more than one vlan in the setlink msg. Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> --- net/bridge/br_netlink.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 9f5eb55..75971b1 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, struct nlattr *af_spec, int cmd) { - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; + struct bridge_vlan_info *vinfo; int err = 0; + struct nlattr *attr; + int err = 0; + int rem; + u16 vid; - 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]); + nla_for_each_nested(attr, af_spec, rem) { + if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO) + continue; + vinfo = nla_data(attr); if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK) return -EINVAL; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC 2014-12-29 21:05 [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC roopa @ 2014-12-29 21:40 ` Scott Feldman 2014-12-29 22:10 ` roopa 0 siblings, 1 reply; 9+ messages in thread From: Scott Feldman @ 2014-12-29 21:40 UTC (permalink / raw) To: Roopa Prabhu; +Cc: Netdev, shemminger, vyasevic@redhat.com On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote: > From: Roopa Prabhu <roopa@cumulusnetworks.com> > > This patch changes bridge IFLA_AF_SPEC netlink attribute parser to > look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows > userspace to pack more than one vlan in the setlink msg. > > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> > --- > net/bridge/br_netlink.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 9f5eb55..75971b1 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, > struct nlattr *af_spec, > int cmd) > { > - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; > + struct bridge_vlan_info *vinfo; > int err = 0; > + struct nlattr *attr; > + int err = 0; > + int rem; > + u16 vid; > > - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, ifla_br_policy); Removing this call orphans ifla_br_policy...should ifla_br_policy be removed? > - if (err) > - return err; > - > - if (tb[IFLA_BRIDGE_VLAN_INFO]) { > - struct bridge_vlan_info *vinfo; > - > - vinfo = nla_data(tb[IFLA_BRIDGE_VLAN_INFO]); > + nla_for_each_nested(attr, af_spec, rem) { > + if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO) > + continue; Need to validate size of attr is sizeof(struct bridge_vlan_info). > > + vinfo = nla_data(attr); > if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK) > return -EINVAL; > > -- > 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] 9+ messages in thread
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC 2014-12-29 21:40 ` Scott Feldman @ 2014-12-29 22:10 ` roopa 2014-12-29 23:47 ` Scott Feldman 0 siblings, 1 reply; 9+ messages in thread From: roopa @ 2014-12-29 22:10 UTC (permalink / raw) To: Scott Feldman; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok On 12/29/14, 1:40 PM, Scott Feldman wrote: > On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote: >> From: Roopa Prabhu <roopa@cumulusnetworks.com> >> >> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to >> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows >> userspace to pack more than one vlan in the setlink msg. >> >> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >> --- >> net/bridge/br_netlink.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >> index 9f5eb55..75971b1 100644 >> --- a/net/bridge/br_netlink.c >> +++ b/net/bridge/br_netlink.c >> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, >> struct nlattr *af_spec, >> int cmd) >> { >> - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; >> + struct bridge_vlan_info *vinfo; >> int err = 0; >> + struct nlattr *attr; >> + int err = 0; >> + int rem; >> + u16 vid; >> >> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, ifla_br_policy); > Removing this call orphans ifla_br_policy...should ifla_br_policy be removed? good question. Its a good place to see the type. In-fact userspace programs also copy the same policy to parse netlink attributes. hmmm.. I would like to keep it if it does not throw a warning. > >> - if (err) >> - return err; >> - >> - if (tb[IFLA_BRIDGE_VLAN_INFO]) { >> - struct bridge_vlan_info *vinfo; >> - >> - vinfo = nla_data(tb[IFLA_BRIDGE_VLAN_INFO]); >> + nla_for_each_nested(attr, af_spec, rem) { >> + if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO) >> + continue; > Need to validate size of attr is sizeof(struct bridge_vlan_info). ack, will fix. > >> + vinfo = nla_data(attr); >> if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK) >> return -EINVAL; >> >> -- >> 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] 9+ messages in thread
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC 2014-12-29 22:10 ` roopa @ 2014-12-29 23:47 ` Scott Feldman 2014-12-30 0:07 ` Scott Feldman 0 siblings, 1 reply; 9+ messages in thread From: Scott Feldman @ 2014-12-29 23:47 UTC (permalink / raw) To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> wrote: > On 12/29/14, 1:40 PM, Scott Feldman wrote: >> >> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote: >>> >>> From: Roopa Prabhu <roopa@cumulusnetworks.com> >>> >>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to >>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows >>> userspace to pack more than one vlan in the setlink msg. >>> >>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >>> --- >>> net/bridge/br_netlink.c | 18 +++++++++--------- >>> 1 file changed, 9 insertions(+), 9 deletions(-) >>> >>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >>> index 9f5eb55..75971b1 100644 >>> --- a/net/bridge/br_netlink.c >>> +++ b/net/bridge/br_netlink.c >>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, >>> struct nlattr *af_spec, >>> int cmd) >>> { >>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; >>> + struct bridge_vlan_info *vinfo; >>> int err = 0; >>> + struct nlattr *attr; >>> + int err = 0; >>> + int rem; >>> + u16 vid; >>> >>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, >>> ifla_br_policy); >> >> Removing this call orphans ifla_br_policy...should ifla_br_policy be >> removed? > > > good question. Its a good place to see the type. In-fact userspace programs > also copy the same policy to parse netlink attributes. hmmm.. > I would like to keep it if it does not throw a warning. I don't know what the policy (sorry, no pun intended) on leaving dead code. I say remove it. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC 2014-12-29 23:47 ` Scott Feldman @ 2014-12-30 0:07 ` Scott Feldman 2014-12-30 0:26 ` Scott Feldman 0 siblings, 1 reply; 9+ messages in thread From: Scott Feldman @ 2014-12-30 0:07 UTC (permalink / raw) To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote: > On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> wrote: >> On 12/29/14, 1:40 PM, Scott Feldman wrote: >>> >>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote: >>>> >>>> From: Roopa Prabhu <roopa@cumulusnetworks.com> >>>> >>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to >>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows >>>> userspace to pack more than one vlan in the setlink msg. >>>> >>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >>>> --- >>>> net/bridge/br_netlink.c | 18 +++++++++--------- >>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >>>> index 9f5eb55..75971b1 100644 >>>> --- a/net/bridge/br_netlink.c >>>> +++ b/net/bridge/br_netlink.c >>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, >>>> struct nlattr *af_spec, >>>> int cmd) >>>> { >>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; >>>> + struct bridge_vlan_info *vinfo; >>>> int err = 0; >>>> + struct nlattr *attr; >>>> + int err = 0; >>>> + int rem; >>>> + u16 vid; >>>> >>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, >>>> ifla_br_policy); >>> >>> Removing this call orphans ifla_br_policy...should ifla_br_policy be >>> removed? >> >> >> good question. Its a good place to see the type. In-fact userspace programs >> also copy the same policy to parse netlink attributes. hmmm.. >> I would like to keep it if it does not throw a warning. > > I don't know what the policy (sorry, no pun intended) on leaving dead > code. I say remove it. You know, not using the policy seems like a step backwards, and maybe it suggests a problem with the attr packing. We had: ifla_br_policy IFLA_BRIDGE_FLAGS IFLA_BRIDGE_MODE IFLA_BRIDGE_VLAN_INFO This patch set makes it: ifla_br_policy IFLA_BRIDGE_FLAGS IFLA_BRIDGE_MODE IFLA_BRIDGE_VLAN_INFO IFLA_BRIDGE_VLAN_RANGE_INFO Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated. I think you want some nesting to clarify: ifla_br_policy IFLA_BRIDGE_FLAGS IFLA_BRIDGE_MODE IFLA_BRIDGE_VLAN_INFO IFLA_BRIDGE_VLAN_LIST_INFO // nested array of IFLA_BRIDGE_VLAN_INFO IFLA_BRIDGE_VLAN_RANGE_INFO Now you can keep the policy for the top-level parsing, and loop only on the nested array VLAN_LIST_INFO. Actually, now you can use just RANGE_INFO in array and have: ifla_br_policy IFLA_BRIDGE_FLAGS IFLA_BRIDGE_MODE IFLA_BRIDGE_VLAN_INFO IFLA_BRIDGE_VLAN_LIST_INFO // nested array of IFLA_BRIDGE_VLAN_RANGE_INFO And use VLAN_RANGE_INFO for both ranges of vids as well as single vids. That'll simplify your filling algo in patch 5. -scott ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC 2014-12-30 0:07 ` Scott Feldman @ 2014-12-30 0:26 ` Scott Feldman 2014-12-30 5:25 ` roopa 0 siblings, 1 reply; 9+ messages in thread From: Scott Feldman @ 2014-12-30 0:26 UTC (permalink / raw) To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@gmail.com> wrote: > On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote: >> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> wrote: >>> On 12/29/14, 1:40 PM, Scott Feldman wrote: >>>> >>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote: >>>>> >>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com> >>>>> >>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to >>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows >>>>> userspace to pack more than one vlan in the setlink msg. >>>>> >>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >>>>> --- >>>>> net/bridge/br_netlink.c | 18 +++++++++--------- >>>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >>>>> index 9f5eb55..75971b1 100644 >>>>> --- a/net/bridge/br_netlink.c >>>>> +++ b/net/bridge/br_netlink.c >>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, >>>>> struct nlattr *af_spec, >>>>> int cmd) >>>>> { >>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; >>>>> + struct bridge_vlan_info *vinfo; >>>>> int err = 0; >>>>> + struct nlattr *attr; >>>>> + int err = 0; >>>>> + int rem; >>>>> + u16 vid; >>>>> >>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, >>>>> ifla_br_policy); >>>> >>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be >>>> removed? >>> >>> >>> good question. Its a good place to see the type. In-fact userspace programs >>> also copy the same policy to parse netlink attributes. hmmm.. >>> I would like to keep it if it does not throw a warning. >> >> I don't know what the policy (sorry, no pun intended) on leaving dead >> code. I say remove it. > > You know, not using the policy seems like a step backwards, and maybe > it suggests a problem with the attr packing. > > We had: > > ifla_br_policy > IFLA_BRIDGE_FLAGS > IFLA_BRIDGE_MODE > IFLA_BRIDGE_VLAN_INFO > > This patch set makes it: > > ifla_br_policy > IFLA_BRIDGE_FLAGS > IFLA_BRIDGE_MODE > IFLA_BRIDGE_VLAN_INFO > IFLA_BRIDGE_VLAN_RANGE_INFO > > Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated. > I think you want some nesting to clarify: > > ifla_br_policy > IFLA_BRIDGE_FLAGS > IFLA_BRIDGE_MODE > IFLA_BRIDGE_VLAN_INFO > IFLA_BRIDGE_VLAN_LIST_INFO // nested array of > IFLA_BRIDGE_VLAN_INFO > IFLA_BRIDGE_VLAN_RANGE_INFO > > Now you can keep the policy for the top-level parsing, and loop only > on the nested array VLAN_LIST_INFO. Actually, now you can use just > RANGE_INFO in array and have: > > ifla_br_policy > IFLA_BRIDGE_FLAGS > IFLA_BRIDGE_MODE > IFLA_BRIDGE_VLAN_INFO > IFLA_BRIDGE_VLAN_LIST_INFO // nested array of > IFLA_BRIDGE_VLAN_RANGE_INFO > > And use VLAN_RANGE_INFO for both ranges of vids as well as single > vids. That'll simplify your filling algo in patch 5. Hmmmm...do you even need VLAN_RANGE_INFO? How about just using existing VLAN_INFO and add some more flags: #define BRIDGE_VLAN_INFO_RANGE_START (1<<3) #define BRIDGE_VLAN_INFO_RANGE_END (1<<4) Now you can have: IFLA_BRIDGE_FLAGS IFLA_BRIDGE_MODE IFLA_BRIDGE_VLAN_INFO IFLA_BRIDGE_VLAN_INFO_LIST // nested array of IFLA_BRIDGE_VLAN_INFO Don't set START or END for single vids in list. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC 2014-12-30 0:26 ` Scott Feldman @ 2014-12-30 5:25 ` roopa 2014-12-30 5:31 ` Scott Feldman 0 siblings, 1 reply; 9+ messages in thread From: roopa @ 2014-12-30 5:25 UTC (permalink / raw) To: Scott Feldman; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok On 12/29/14, 4:26 PM, Scott Feldman wrote: > On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@gmail.com> wrote: >> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote: >>> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> wrote: >>>> On 12/29/14, 1:40 PM, Scott Feldman wrote: >>>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote: >>>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com> >>>>>> >>>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to >>>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows >>>>>> userspace to pack more than one vlan in the setlink msg. >>>>>> >>>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >>>>>> --- >>>>>> net/bridge/br_netlink.c | 18 +++++++++--------- >>>>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>>>> >>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >>>>>> index 9f5eb55..75971b1 100644 >>>>>> --- a/net/bridge/br_netlink.c >>>>>> +++ b/net/bridge/br_netlink.c >>>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, >>>>>> struct nlattr *af_spec, >>>>>> int cmd) >>>>>> { >>>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; >>>>>> + struct bridge_vlan_info *vinfo; >>>>>> int err = 0; >>>>>> + struct nlattr *attr; >>>>>> + int err = 0; >>>>>> + int rem; >>>>>> + u16 vid; >>>>>> >>>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, >>>>>> ifla_br_policy); >>>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be >>>>> removed? >>>> >>>> good question. Its a good place to see the type. In-fact userspace programs >>>> also copy the same policy to parse netlink attributes. hmmm.. >>>> I would like to keep it if it does not throw a warning. >>> I don't know what the policy (sorry, no pun intended) on leaving dead >>> code. I say remove it. >> You know, not using the policy seems like a step backwards, and maybe >> it suggests a problem with the attr packing. >> >> We had: >> >> ifla_br_policy >> IFLA_BRIDGE_FLAGS >> IFLA_BRIDGE_MODE >> IFLA_BRIDGE_VLAN_INFO >> >> This patch set makes it: >> >> ifla_br_policy >> IFLA_BRIDGE_FLAGS >> IFLA_BRIDGE_MODE >> IFLA_BRIDGE_VLAN_INFO >> IFLA_BRIDGE_VLAN_RANGE_INFO >> >> Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated. >> I think you want some nesting to clarify: >> >> ifla_br_policy >> IFLA_BRIDGE_FLAGS >> IFLA_BRIDGE_MODE >> IFLA_BRIDGE_VLAN_INFO >> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of >> IFLA_BRIDGE_VLAN_INFO >> IFLA_BRIDGE_VLAN_RANGE_INFO >> >> Now you can keep the policy for the top-level parsing, and loop only >> on the nested array VLAN_LIST_INFO. Actually, now you can use just >> RANGE_INFO in array and have: >> >> ifla_br_policy >> IFLA_BRIDGE_FLAGS >> IFLA_BRIDGE_MODE >> IFLA_BRIDGE_VLAN_INFO >> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of >> IFLA_BRIDGE_VLAN_RANGE_INFO >> >> And use VLAN_RANGE_INFO for both ranges of vids as well as single >> vids. That'll simplify your filling algo in patch 5. > Hmmmm...do you even need VLAN_RANGE_INFO? How about just using > existing VLAN_INFO and add some more flags: > > #define BRIDGE_VLAN_INFO_RANGE_START (1<<3) > #define BRIDGE_VLAN_INFO_RANGE_END (1<<4) > > Now you can have: > > IFLA_BRIDGE_FLAGS > IFLA_BRIDGE_MODE > IFLA_BRIDGE_VLAN_INFO > IFLA_BRIDGE_VLAN_INFO_LIST // nested array of > IFLA_BRIDGE_VLAN_INFO > > Don't set START or END for single vids in list. ok. I was debating yesterday about introducing another nest. This looks good. My only reason to not use existing IFLA_BRIDGE_VLAN_INFO was to make sure it works for existing users. I see that in this case since IFLA_BRIDGE_VLAN_INFO_LIST is new, it will not affect existing users. But, i cant use IFLA_BRIDGE_VLAN_INFO (ie an attribute in ifla_br_policy) under IFLA_BRIDGE_VLAN_INFO_LIST ?. IFLA_BRIDGE_VLAN_INFO_LIST will have its own policy and its own attributes. Which will make it look something like below ? IFLA_BRIDGE_FLAGS IFLA_BRIDGE_MODE IFLA_BRIDGE_VLAN_INFO IFLA_BRIDGE_VLAN_INFO_LIST // nested array of IFLA_BRIDGE_VLAN_INFO_LIST_ENTRY Thanks, Roopa ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC 2014-12-30 5:25 ` roopa @ 2014-12-30 5:31 ` Scott Feldman 2014-12-30 6:01 ` roopa 0 siblings, 1 reply; 9+ messages in thread From: Scott Feldman @ 2014-12-30 5:31 UTC (permalink / raw) To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok On Mon, Dec 29, 2014 at 9:25 PM, roopa <roopa@cumulusnetworks.com> wrote: > On 12/29/14, 4:26 PM, Scott Feldman wrote: >> >> On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@gmail.com> wrote: >>> >>> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote: >>>> >>>> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> >>>> wrote: >>>>> >>>>> On 12/29/14, 1:40 PM, Scott Feldman wrote: >>>>>> >>>>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote: >>>>>>> >>>>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com> >>>>>>> >>>>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to >>>>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows >>>>>>> userspace to pack more than one vlan in the setlink msg. >>>>>>> >>>>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >>>>>>> --- >>>>>>> net/bridge/br_netlink.c | 18 +++++++++--------- >>>>>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>>>>> >>>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >>>>>>> index 9f5eb55..75971b1 100644 >>>>>>> --- a/net/bridge/br_netlink.c >>>>>>> +++ b/net/bridge/br_netlink.c >>>>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, >>>>>>> struct nlattr *af_spec, >>>>>>> int cmd) >>>>>>> { >>>>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; >>>>>>> + struct bridge_vlan_info *vinfo; >>>>>>> int err = 0; >>>>>>> + struct nlattr *attr; >>>>>>> + int err = 0; >>>>>>> + int rem; >>>>>>> + u16 vid; >>>>>>> >>>>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, >>>>>>> ifla_br_policy); >>>>>> >>>>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be >>>>>> removed? >>>>> >>>>> >>>>> good question. Its a good place to see the type. In-fact userspace >>>>> programs >>>>> also copy the same policy to parse netlink attributes. hmmm.. >>>>> I would like to keep it if it does not throw a warning. >>>> >>>> I don't know what the policy (sorry, no pun intended) on leaving dead >>>> code. I say remove it. >>> >>> You know, not using the policy seems like a step backwards, and maybe >>> it suggests a problem with the attr packing. >>> >>> We had: >>> >>> ifla_br_policy >>> IFLA_BRIDGE_FLAGS >>> IFLA_BRIDGE_MODE >>> IFLA_BRIDGE_VLAN_INFO >>> >>> This patch set makes it: >>> >>> ifla_br_policy >>> IFLA_BRIDGE_FLAGS >>> IFLA_BRIDGE_MODE >>> IFLA_BRIDGE_VLAN_INFO >>> IFLA_BRIDGE_VLAN_RANGE_INFO >>> >>> Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated. >>> I think you want some nesting to clarify: >>> >>> ifla_br_policy >>> IFLA_BRIDGE_FLAGS >>> IFLA_BRIDGE_MODE >>> IFLA_BRIDGE_VLAN_INFO >>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of >>> IFLA_BRIDGE_VLAN_INFO >>> IFLA_BRIDGE_VLAN_RANGE_INFO >>> >>> Now you can keep the policy for the top-level parsing, and loop only >>> on the nested array VLAN_LIST_INFO. Actually, now you can use just >>> RANGE_INFO in array and have: >>> >>> ifla_br_policy >>> IFLA_BRIDGE_FLAGS >>> IFLA_BRIDGE_MODE >>> IFLA_BRIDGE_VLAN_INFO >>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of >>> IFLA_BRIDGE_VLAN_RANGE_INFO >>> >>> And use VLAN_RANGE_INFO for both ranges of vids as well as single >>> vids. That'll simplify your filling algo in patch 5. >> >> Hmmmm...do you even need VLAN_RANGE_INFO? How about just using >> existing VLAN_INFO and add some more flags: >> >> #define BRIDGE_VLAN_INFO_RANGE_START (1<<3) >> #define BRIDGE_VLAN_INFO_RANGE_END (1<<4) >> >> Now you can have: >> >> IFLA_BRIDGE_FLAGS >> IFLA_BRIDGE_MODE >> IFLA_BRIDGE_VLAN_INFO >> IFLA_BRIDGE_VLAN_INFO_LIST // nested array of >> IFLA_BRIDGE_VLAN_INFO >> >> Don't set START or END for single vids in list. > > > ok. I was debating yesterday about introducing another nest. This looks > good. > My only reason to not use existing IFLA_BRIDGE_VLAN_INFO was to make sure it > works for existing users. > I see that in this case since IFLA_BRIDGE_VLAN_INFO_LIST is new, it will not > affect existing users. > > But, i cant use IFLA_BRIDGE_VLAN_INFO (ie an attribute in ifla_br_policy) > under IFLA_BRIDGE_VLAN_INFO_LIST ?. I don't see why not. You're not going to parse the array with a policy anyway (since it's an array). And attr INFO_LIST will be .type = NLA_NESTED in ifla_br_policy. > > IFLA_BRIDGE_VLAN_INFO_LIST will have its own policy and its own attributes. > > Which will make it look something like below ? > > IFLA_BRIDGE_FLAGS > IFLA_BRIDGE_MODE > IFLA_BRIDGE_VLAN_INFO > IFLA_BRIDGE_VLAN_INFO_LIST // nested array of > IFLA_BRIDGE_VLAN_INFO_LIST_ENTRY > > > Thanks, > Roopa ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC 2014-12-30 5:31 ` Scott Feldman @ 2014-12-30 6:01 ` roopa 0 siblings, 0 replies; 9+ messages in thread From: roopa @ 2014-12-30 6:01 UTC (permalink / raw) To: Scott Feldman; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok On 12/29/14, 9:31 PM, Scott Feldman wrote: > On Mon, Dec 29, 2014 at 9:25 PM, roopa <roopa@cumulusnetworks.com> wrote: >> On 12/29/14, 4:26 PM, Scott Feldman wrote: >>> On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@gmail.com> wrote: >>>> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote: >>>>> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> >>>>> wrote: >>>>>> On 12/29/14, 1:40 PM, Scott Feldman wrote: >>>>>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote: >>>>>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com> >>>>>>>> >>>>>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to >>>>>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows >>>>>>>> userspace to pack more than one vlan in the setlink msg. >>>>>>>> >>>>>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >>>>>>>> --- >>>>>>>> net/bridge/br_netlink.c | 18 +++++++++--------- >>>>>>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>>>>>> >>>>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >>>>>>>> index 9f5eb55..75971b1 100644 >>>>>>>> --- a/net/bridge/br_netlink.c >>>>>>>> +++ b/net/bridge/br_netlink.c >>>>>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, >>>>>>>> struct nlattr *af_spec, >>>>>>>> int cmd) >>>>>>>> { >>>>>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; >>>>>>>> + struct bridge_vlan_info *vinfo; >>>>>>>> int err = 0; >>>>>>>> + struct nlattr *attr; >>>>>>>> + int err = 0; >>>>>>>> + int rem; >>>>>>>> + u16 vid; >>>>>>>> >>>>>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, >>>>>>>> ifla_br_policy); >>>>>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be >>>>>>> removed? >>>>>> >>>>>> good question. Its a good place to see the type. In-fact userspace >>>>>> programs >>>>>> also copy the same policy to parse netlink attributes. hmmm.. >>>>>> I would like to keep it if it does not throw a warning. >>>>> I don't know what the policy (sorry, no pun intended) on leaving dead >>>>> code. I say remove it. >>>> You know, not using the policy seems like a step backwards, and maybe >>>> it suggests a problem with the attr packing. >>>> >>>> We had: >>>> >>>> ifla_br_policy >>>> IFLA_BRIDGE_FLAGS >>>> IFLA_BRIDGE_MODE >>>> IFLA_BRIDGE_VLAN_INFO >>>> >>>> This patch set makes it: >>>> >>>> ifla_br_policy >>>> IFLA_BRIDGE_FLAGS >>>> IFLA_BRIDGE_MODE >>>> IFLA_BRIDGE_VLAN_INFO >>>> IFLA_BRIDGE_VLAN_RANGE_INFO >>>> >>>> Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated. >>>> I think you want some nesting to clarify: >>>> >>>> ifla_br_policy >>>> IFLA_BRIDGE_FLAGS >>>> IFLA_BRIDGE_MODE >>>> IFLA_BRIDGE_VLAN_INFO >>>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of >>>> IFLA_BRIDGE_VLAN_INFO >>>> IFLA_BRIDGE_VLAN_RANGE_INFO >>>> >>>> Now you can keep the policy for the top-level parsing, and loop only >>>> on the nested array VLAN_LIST_INFO. Actually, now you can use just >>>> RANGE_INFO in array and have: >>>> >>>> ifla_br_policy >>>> IFLA_BRIDGE_FLAGS >>>> IFLA_BRIDGE_MODE >>>> IFLA_BRIDGE_VLAN_INFO >>>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of >>>> IFLA_BRIDGE_VLAN_RANGE_INFO >>>> >>>> And use VLAN_RANGE_INFO for both ranges of vids as well as single >>>> vids. That'll simplify your filling algo in patch 5. >>> Hmmmm...do you even need VLAN_RANGE_INFO? How about just using >>> existing VLAN_INFO and add some more flags: >>> >>> #define BRIDGE_VLAN_INFO_RANGE_START (1<<3) >>> #define BRIDGE_VLAN_INFO_RANGE_END (1<<4) >>> >>> Now you can have: >>> >>> IFLA_BRIDGE_FLAGS >>> IFLA_BRIDGE_MODE >>> IFLA_BRIDGE_VLAN_INFO >>> IFLA_BRIDGE_VLAN_INFO_LIST // nested array of >>> IFLA_BRIDGE_VLAN_INFO >>> >>> Don't set START or END for single vids in list. >> >> ok. I was debating yesterday about introducing another nest. This looks >> good. >> My only reason to not use existing IFLA_BRIDGE_VLAN_INFO was to make sure it >> works for existing users. >> I see that in this case since IFLA_BRIDGE_VLAN_INFO_LIST is new, it will not >> affect existing users. >> >> But, i cant use IFLA_BRIDGE_VLAN_INFO (ie an attribute in ifla_br_policy) >> under IFLA_BRIDGE_VLAN_INFO_LIST ?. > I don't see why not. You're not going to parse the array with a > policy anyway (since it's an array). And attr INFO_LIST will be .type > = NLA_NESTED in ifla_br_policy. > > agree that it will work if everybody assumes that this is an array of just this attrtype. wasn't sure if it is a good practice to reuse an attribute from an upper nest. will work on v2. thanks for the review. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-12-30 6:01 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-29 21:05 [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC roopa 2014-12-29 21:40 ` Scott Feldman 2014-12-29 22:10 ` roopa 2014-12-29 23:47 ` Scott Feldman 2014-12-30 0:07 ` Scott Feldman 2014-12-30 0:26 ` Scott Feldman 2014-12-30 5:25 ` roopa 2014-12-30 5:31 ` Scott Feldman 2014-12-30 6:01 ` roopa
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).