From: roopa <roopa@cumulusnetworks•com>
To: Scott Feldman <sfeldma@gmail•com>
Cc: Netdev <netdev@vger•kernel.org>,
shemminger@vyatta•com,
"vyasevic@redhat•com" <vyasevic@redhat•com>,
Wilson Kok <wkok@cumulusnetworks•com>
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 [thread overview]
Message-ID: <54A23FC1.90403@cumulusnetworks.com> (raw)
In-Reply-To: <CAE4R7bDjmHrW+bzoPJ-fpN51ykTnqjrXy7hCqXgJNv-EHayiHg@mail.gmail.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.
prev parent reply other threads:[~2014-12-30 6:01 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
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=54A23FC1.90403@cumulusnetworks.com \
--to=roopa@cumulusnetworks$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=sfeldma@gmail$(echo .)com \
--cc=shemminger@vyatta$(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