Subject: Re: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to accomodate vlan list and ranges

From: roopa <[email-masked]>
Date: Sat, 03 Jan 2015 00:39:36 -0800

On 1/1/15, 11:34 AM, Scott Feldman wrote:
> On Thu, Jan 1, 2015 at 12:54 AM, Arad, Ronen <[email-masked]> wrote:
>>
>>> -----Original Message-----
>>> From: [email-masked] [mailto:[email-masked]] On
>>> Behalf Of [email-masked]
>>> Sent: Wednesday, December 31, 2014 6:49 PM
>>> To: [email-masked]; [email-masked]; [email-masked]
>>> Cc: [email-masked]; [email-masked]; Roopa Prabhu
>>> Subject: [PATCH net-next v2 2/2] bridge: modify bridge af spec parser to
>>> accomodate vlan list and ranges
>>>
>>> From: Roopa Prabhu <[email-masked]>
>>>
>>> This patch modifies br_afspec to parse incoming IFLA_BRIDGE_VLAN_INFO_LIST
>>>
>>> Signed-off-by: Roopa Prabhu <[email-masked]>
>>> ---
>>> 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