On Mon, Dec 29, 2014 at 1:05 PM,  [off-list ref] wrote:

One per line?

+       u16 pvid;

Aren't you getting an "unused" compile warning on this ^^^?

+
+       /* handle the untagged */

This comment ^^^ doesn't make sense here.

+       for_each_set_bit(vid, vlan_bmp, VLAN_N_VID) {
+               if (start == 0) {
+                       start = vid;
+                       end = vid;
+               }
+               if ((vid - end) > 1) {
+                       memset(&vinfo_range, 0, sizeof(vinfo_range));
+                       vinfo_range.flags |= flags;
+                       vinfo_range.vid = start;
+                       vinfo_range.vid_end = end;
+                       if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO,
+                                   sizeof(vinfo_range), &vinfo_range))
+                               goto nla_put_failure;
+                       start = vid;
+                       end = vid;
+               } else {
+                       end = vid;
+               }
+       }

What happens with this set {1-10, 12, 20-25, 30}?  On vid 12, will
that put a VLAN_RANGE_INFO with start=end=12?  Seems strange to use
RANGE_INFO for single vlan.

Can the algorithm be simplified?  Maybe there are other examples in
the kernel of finding ranges/singles from a bitmap we could borrow
code from?

+       if (start != 0 && end != 0) {
+               if (start != end) {
+                       memset(&vinfo_range, 0, sizeof(vinfo_range));
+                       vinfo_range.flags |= flags;
+                       vinfo_range.vid = start;
+                       vinfo_range.vid_end = end;
+                       if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO,
+                                   sizeof(vinfo_range), &vinfo_range))
+                               goto nla_put_failure;
+               } else {
+                       memset(&vinfo, 0, sizeof(vinfo));
+                       vinfo.flags |= flags;
+                       vinfo.vid = start;
+                       if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
+                                   sizeof(vinfo), &vinfo))
+                               goto nla_put_failure;

How many IFLA_BRIDGE_VLAN_INFOs can you fit into skb?   It seems the
worst case is you'll have 4094/2 = 2047 VLAN_INFOs if using all
even-numbered vids, for example.  Will that fit?  I guess the original
code had the same concern...wonder if anyone checked this worst-case?

+               }
+       }
+
+nla_put_failure:
+       return -EMSGSIZE;
+}
+
+static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
+                                        const struct net_port_vlans *pv)
+{
+       unsigned long vlan_bmp_copy[BR_VLAN_BITMAP_LEN];
+       unsigned long untagged_bmp_copy[BR_VLAN_BITMAP_LEN];

Lots of automatic space on the stack...can you use dynamic mem
(kzalloc) for these?

+       struct bridge_vlan_range_info vinfo_range;
+       struct bridge_vlan_info vinfo;
+       u16 pvid;
+
+       memset(vlan_bmp_copy, 0,
+              sizeof(unsigned long) * BR_VLAN_BITMAP_LEN);
+       bitmap_copy(vlan_bmp_copy, pv->vlan_bitmap, VLAN_N_VID);
+
+       memset(untagged_bmp_copy, 0,
+              sizeof(unsigned long) * BR_VLAN_BITMAP_LEN);
+       bitmap_copy(untagged_bmp_copy, pv->untagged_bitmap, VLAN_N_VID);
+
+       /* send the pvid separately first */
+       pvid = br_get_pvid(pv);
+       if (pvid && (pvid != VLAN_N_VID)) {
+               memset(&vinfo, 0, sizeof(vinfo));
+               vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
+               if (test_bit(pvid, untagged_bmp_copy)) {
+                       vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED;
+                       clear_bit(pvid, untagged_bmp_copy);
+               }
+               clear_bit(pvid, vlan_bmp_copy);
+               vinfo.vid = pvid;
+               if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
+                           sizeof(vinfo), &vinfo))
+                       goto nla_put_failure;
+       }

What if pvid is not in vlan_bmp_copy?  This statement block seems
slightly different than the original logic where BRIDGE_VLAN_INFO_PVID
was set only when looping thru vlan_bitmap and vid == pvid.  Now can
you send a IFLA_BRIDGE_VLAN_INFO with pvid even if pvid isn't in
vlan_bitmap?  Maybe pvid is always in vlan_bitmap, so it doesn't
matter.  But there is a subtle logic change here, so something to
consider.

+
+       /* fill untagged vlans */
+       br_fill_ifvlaninfo_bitmap(skb, untagged_bmp_copy,
+                                 BRIDGE_VLAN_INFO_UNTAGGED);

This might be doing more than original logic.  Original logic looped
thru vid in vlan_btimap and checked if vid was in untagged_bitmap.
Here you're dumping all bits in untagged_bitmap, without looking if
bit was set in vlan_bitmap.  Maybe you want to do an intersection of
sets vlan_bitmap and untagged_bitmap.