From mboxrd@z Thu Jan 1 00:00:00 1970 From: roopa Subject: Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC Date: Mon, 29 Dec 2014 22:01:37 -0800 Message-ID: <54A23FC1.90403@cumulusnetworks.com> References: <1419887132-7084-2-git-send-email-roopa@cumulusnetworks.com> <54A1D157.1000002@cumulusnetworks.com> <54A2374F.6050901@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Netdev , shemminger@vyatta.com, "vyasevic@redhat.com" , Wilson Kok To: Scott Feldman Return-path: Received: from mail-pa0-f47.google.com ([209.85.220.47]:50421 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751642AbaL3GBi (ORCPT ); Tue, 30 Dec 2014 01:01:38 -0500 Received: by mail-pa0-f47.google.com with SMTP id kq14so18863615pab.6 for ; Mon, 29 Dec 2014 22:01:38 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On 12/29/14, 9:31 PM, Scott Feldman wrote: > On Mon, Dec 29, 2014 at 9:25 PM, roopa wrote: >> On 12/29/14, 4:26 PM, Scott Feldman wrote: >>> On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman wrote: >>>> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman wrote: >>>>> On Mon, Dec 29, 2014 at 2:10 PM, roopa >>>>> wrote: >>>>>> On 12/29/14, 1:40 PM, Scott Feldman wrote: >>>>>>> On Mon, Dec 29, 2014 at 1:05 PM, wrote: >>>>>>>> From: Roopa Prabhu >>>>>>>> >>>>>>>> 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 >>>>>>>> --- >>>>>>>> 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.