Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC

[roopa <roopa@cumulusnetworks.com>]

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.