Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
> >>On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
> >>>>Add a binary sysfs file that will dump out vlans currently configured on the
> >>>>port.
> >>>>
> >>>>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>>
> >>>So what's the format here? I could not tell.
> >>>List of vlans? Why binary? Why not make it text in that case?
> >>>This would also allow reporting "all" if filtering
> >>>is disabled and "untagged" for untagged packets.
> >>
> >>I decided to do binary because text may result in more then page
> >>worth of data.
> >
> >Well yes, you need 3 bytes to print a vid and 1 extra for a space.
> 
> 4 bytes per vid and 1 for space or NULL.

Why 4? It's 12 bit so you can print in hex: 'ABC ' 
Whatever :)

> > But
> >in binary, 2 bytes per VID and there are 4K valid VIDs so binary needs
> >more than 1 page too?
> >
> >If using binary I would go for bit per valid bit
> >this solves it nicely.
> 
> True.  The tool reads it in chunks.  I thought about dumping the
> bitmap, but that meant that made the tool more complex as it had to
> know the bitmap construction.

To me, it makes more sense than knowing that vids are
zero-extended to 16 bytes and packed in an array,
and that you need to read it in chunks.
Could be just me :)

> >We can have a separate field that shows whether untagged
> >packets are filtered.
> >
> >>The display tool will know how to display things
> >>properly.
> >>
> >>-vlad
> >
> >I think you know that one of the points of sysfs
> >is to allow use without tools.
> >Tools are happier using other interfaces such as
> >ioctl or netlink.
> >
> 
> I initially though of creating a sysfs object per vlan.  That would
> have made it easy to see which vlans are configured without any
> tools.
> But that could result in a lot of objects being created, so I abandoned it.
> 
> I did think about a text interface, but due to a page of output
> limitation, I didn't go that route.  The reason is that if someone
> cats the file, they may not see all the vlans configured.  So I
> decided on the binary interface, since a binary interface with a
> tool to read it could avoid the single page limitation.
> 
> -vlad

Maybe it's not needed in sysfs then - expose it to
brctl or whatever.

> >
> >
> >>>
> >>>>---
> >>>>  include/linux/if_bridge.h |    1 +
> >>>>  net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
> >>>>  net/bridge/br_private.h   |    2 ++
> >>>>  net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
> >>>>  4 files changed, 65 insertions(+), 0 deletions(-)
> >>>>
> >>>>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>>>index ab750dd..d0f869b 100644
> >>>>--- a/include/linux/if_bridge.h
> >>>>+++ b/include/linux/if_bridge.h
> >>>>@@ -20,6 +20,7 @@
> >>>>  #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
> >>>>  #define SYSFS_BRIDGE_PORT_ATTR	"brport"
> >>>>  #define SYSFS_BRIDGE_PORT_LINK	"bridge"
> >>>>+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
> >>>>
> >>>>  #define BRCTL_VERSION 1
> >>>>
> >>>>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >>>>index 90c1038..3963748 100644
> >>>>--- a/net/bridge/br_if.c
> >>>>+++ b/net/bridge/br_if.c
> >>>>@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> >>>>  	return 0;
> >>>>  }
> >>>>
> >>>>+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
> >>>>+			unsigned long max, unsigned long skip)
> >>>>+{
> >>>>+	unsigned long *map;
> >>>>+	unsigned short *vid = (unsigned short *)buf;
> >>>>+	unsigned short i;
> >>>>+	int num = 0;
> >>>>+
> >>>>+	if (skip > (VLAN_N_VID+1))
> >>>>+		return -EINVAL;
> >>>>+
> >>>>+	memset(buf, 0, max * sizeof(unsigned short));
> >>>
> >>>Isn't max is in bytes? why is this safe?
> >>>
> >>>>+
> >>>>+	rcu_read_lock();
> >>>>+	map = rcu_dereference(p->vlan_map);
> >>>>+	if (!map)
> >>>>+		goto out;
> >>>>+
> >>>>+	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
> >>>
> >>>Isn't skip in bytes too? Why do you compare it to i which is
> >>>in dwords?
> >>>
> >>>>+		if (test_bit(i, map)) {
> >>>>+			if (num > max)
> >>>>+				goto out;
> >>>>+
> >>>>+			*vid = i-1;
> >>>>+			vid++;
> >>>>+			num++;
> >>>>+		}
> >>>>+	}
> >>>>+out:
> >>>>+	rcu_read_unlock();
> >>>>+
> >>>>+	return num*sizeof(unsigned short);
> >>>>+}
> >>>>+
> >>>>  void __net_exit br_net_exit(struct net *net)
> >>>>  {
> >>>>  	struct net_device *dev;
> >>>>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>>>index 5639c1c..cf95cd7 100644
> >>>>--- a/net/bridge/br_private.h
> >>>>+++ b/net/bridge/br_private.h
> >>>>@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
> >>>>  	netdev_features_t features);
> >>>>  extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>  extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
> >>>>+				unsigned long max, unsigned long skip);
> >>>>
> >>>>  /* br_input.c */
> >>>>  extern int br_handle_frame_finish(struct sk_buff *skb);
> >>>>diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> >>>>index 13b36bd..a81e2ef 100644
> >>>>--- a/net/bridge/br_sysfs_if.c
> >>>>+++ b/net/bridge/br_sysfs_if.c
> >>>>@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
> >>>>  };
> >>>>
> >>>>  /*
> >>>>+ * Export the vlan table for a given port as a binary file.
> >>>>+ * The records are unsgined shorts.
> >>>>+ *
> >>>>+ * Returns the number of bytes read.
> >>>>+ */
> >>>>+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
> >>>>+				struct bin_attribute *bin_attr,
> >>>>+				char *buf, loff_t off, size_t count)
> >>>>+{
> >>>>+	struct net_bridge_port *p = to_brport(kobj);
> >>>>+
> >>>>+	return br_port_fill_vlans(p, buf,
> >>>>+				count/sizeof(unsigned short),
> >>>>+				off/sizeof(unsigned short));
> >>>>+}
> >>>>+
> >>>>+static struct bin_attribute port_vlans = {
> >>>>+	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
> >>>>+		  .mode = S_IRUGO, },
> >>>>+	.read = brport_vlans_read,
> >>>>+};
> >>>>+
> >>>>+/*
> >>>>   * Add sysfs entries to ethernet device added to a bridge.
> >>>>   * Creates a brport subdirectory with bridge attributes.
> >>>>   * Puts symlink in bridge's brif subdirectory
> >>>>@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
> >>>>  			return err;
> >>>>  	}
> >>>>
> >>>>+	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
> >>>>+	if (err) {
> >>>>+		return err;
> >>>>+	}
> >>>>+
> >>>>  	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
> >>>>  	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
> >>>>  }
> >>>>--
> >>>>1.7.7.6
> >>>>
> >>>>--
> >>>>To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>>>the body of a message to majordomo@vger.kernel.org
> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html