[RFC PATCH 0/2] Allow full bridge configuration via sysfs

[RFC PATCH 0/2] Allow full bridge configuration via sysfs

[Bill Nottingham]

Right now, you can configure most bridge device parameters via sysfs.
However, you cannot either:
- add or remove bridge interfaces
- add or remove physical interfaces from a bridge

The attached patch set rectifies this. With this patch set, brctl
(theoretically) becomes completely optional, much like ifenslave is
now for bonding. (In fact, the idea for this patch, and the syntax
used herein, is inspired by the sysfs bonding configuration.)

Patchset is against net-next-2.6, but should apply to anything reasonably
recent with minimal fuzz.

Bill Nottingham (2):
  Add a 'bridging_masters' file in sysfs under class/net
  Add a 'interfaces' file to the bridge device configuration in sysfs

 net/bridge/br.c          |    2 +
 net/bridge/br_private.h  |    4 ++
 net/bridge/br_sysfs_br.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 0 deletions(-)

[PATCH 1/2] Add a 'bridging_masters' file in sysfs under class/net

[Bill Nottingham]

To use this file:
	echo "+<bridge device name>" > bridging_masters
to add a new bridge, and:
	 echo "-<bridge device name>" > bridging masters
to remove a device. Reading the file lists the current bridge devices.

Signed-off-by: Bill Nottingham <notting@redhat.com>
---
 net/bridge/br.c          |    2 +
 net/bridge/br_private.h  |    4 ++
 net/bridge/br_sysfs_br.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index 573acdf..89c6c7c 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -59,6 +59,7 @@ static int __init br_init(void)
 
 	br_fdb_get_hook = br_fdb_get;
 	br_fdb_put_hook = br_fdb_put;
+	br_create_sysfs();
 
 	return 0;
 err_out3:
@@ -83,6 +84,7 @@ static void __exit br_deinit(void)
 	br_cleanup_bridges();
 
 	synchronize_net();
+	br_destroy_sysfs();
 
 	br_netfilter_fini();
 	br_fdb_get_hook = NULL;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 815ed38..d5ff1c6 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -249,6 +249,8 @@ extern void br_ifinfo_notify(int event, struct net_bridge_port *port);
 #ifdef CONFIG_SYSFS
 /* br_sysfs_if.c */
 extern struct sysfs_ops brport_sysfs_ops;
+extern int br_create_sysfs(void);
+extern void br_destroy_sysfs(void);
 extern int br_sysfs_addif(struct net_bridge_port *p);
 
 /* br_sysfs_br.c */
@@ -257,6 +259,8 @@ extern void br_sysfs_delbr(struct net_device *dev);
 
 #else
 
+#define br_create_sysfs()	(0)
+#define br_destroy_sysfs()	(0)
 #define br_sysfs_addif(p)	(0)
 #define br_sysfs_addbr(dev)	(0)
 #define br_sysfs_delbr(dev)	do { } while(0)
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 27d6a51..a5d5fef 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -24,6 +24,77 @@
 #define to_dev(obj)	container_of(obj, struct device, kobj)
 #define to_bridge(cd)	((struct net_bridge *)(to_net_dev(cd)->priv))
 
+static ssize_t show_bridges(struct class *cls, char *buf)
+{
+	int res = 0;
+	struct net_device *dev, *next;
+
+	rtnl_lock();
+	for_each_netdev_safe(&init_net, dev, next) {
+		if (dev->priv_flags & IFF_EBRIDGE) {
+			if (res > (PAGE_SIZE - IFNAMSIZ)) {
+				if ((PAGE_SIZE - res) > 10)
+					res = PAGE_SIZE - 10;
+				res += sprintf(buf + res, "++more++ ");
+				break;
+			}
+			res += sprintf(buf + res, "%s ", dev->name);
+		}
+	}
+	if (res)
+		buf[res-1] = '\n';
+	rtnl_unlock();
+	return res;
+}
+
+static ssize_t store_bridges(struct class *cls, const char *buffer, size_t len)
+{
+	char command[IFNAMSIZ + 1] = {0, };
+	char *ifname;
+	int res = 0;
+
+	sscanf(buffer, "%16s", command); /* IFNAMSIZ */
+	ifname = command + 1;
+	if ((strlen(command) <= 1) || !dev_valid_name(ifname))
+		goto err_no_cmd;
+
+	if (command[0] == '+') {
+		res = br_add_bridge(ifname);
+		goto out;
+	}
+	if (command[0] == '-') {
+		res = br_del_bridge(ifname);
+		goto out;
+	}
+err_no_cmd:
+	printk(KERN_ERR "bridge: no command found in bridging_masters. Use +ifname or -ifname.\n");
+	res = -EPERM;
+out:
+	return res ? res : len;
+}
+
+static CLASS_ATTR(bridging_masters, S_IWUSR | S_IRUGO, show_bridges, store_bridges);
+
+static struct class *netdev_class;
+
+int br_create_sysfs(void)
+{
+	int ret;
+
+	netdev_class = (&init_net)->loopback_dev->dev.class;
+	if (!netdev_class)
+		return -ENODEV;
+
+	ret = class_create_file(netdev_class, &class_attr_bridging_masters);
+	return ret;
+}
+
+void br_destroy_sysfs(void)
+{
+	if (netdev_class)
+		class_remove_file(netdev_class, &class_attr_bridging_masters);
+}
+
 /*
  * Common code for storing bridge parameters.
  */
-- 
1.5.5.1

[PATCH 2/2] Add a 'interfaces' file to the bridge device configuration in sysfs

[Bill Nottingham]

To use this file:
	echo "+<device name>" > /sys/class/net/<name>/bridge/interfaces
to add a new physical interface to bridge device <name>, and:
	echo "-<device name>" > interfaces
to remove a device. Reading the file lists the current phyiscal interfaces for that bridge.

Signed-off-by: Bill Nottingham <notting@redhat.com>
---
 net/bridge/br_sysfs_br.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index a5d5fef..2dae8f3 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -405,6 +405,70 @@ static ssize_t store_flush(struct device *d,
 }
 static DEVICE_ATTR(flush, S_IWUSR, NULL, store_flush);
 
+static ssize_t show_interfaces(struct device *d,
+			       struct device_attribute *attr, char *buf)
+{
+	struct net_bridge *br = to_bridge(d);
+	struct net_bridge_port *p;
+	int res = 0;
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (res > (PAGE_SIZE - IFNAMSIZ)) {
+			if ((PAGE_SIZE - res) > 10)
+				res = PAGE_SIZE - 10;
+			res += sprintf(buf + res, "++more++ ");
+			break;
+		}
+		res += sprintf(buf + res, "%s ", p->dev->name);
+	}
+	if (res)
+		buf[res-1] = '\n';
+	return res;
+}
+
+static ssize_t store_interfaces(struct device *d,
+				struct device_attribute *attr,
+				const char *buf, size_t len)
+{
+	char command[IFNAMSIZ + 1] = { 0, };
+	char *ifname;
+	struct net_bridge *br = to_bridge(d);
+	struct net_device *dev;
+	int ret = 0;
+
+	sscanf(buf, "%16s", command);	/* IFNAMSIZ */
+	ifname = command + 1;
+	if ((strlen(command) <= 1) || !dev_valid_name(ifname))
+		goto err_no_cmd;
+	if (command[0] == '+') {
+		rtnl_lock();
+		dev = __dev_get_by_name(&init_net, ifname);
+		if (dev == NULL)
+			ret = -ENXIO;
+		else
+			ret = br_add_if(br, dev);
+		rtnl_unlock();
+		goto out;
+	}
+	if (command[0] == '-') {
+		rtnl_lock();
+		dev = __dev_get_by_name(&init_net, ifname);
+		if (dev == NULL)
+			ret = -ENXIO;
+		else
+			ret = br_del_if(br, dev);
+		rtnl_unlock();
+		goto out;
+	}
+err_no_cmd:
+	printk(KERN_ERR "bridge: no command found in interfaces file for bridge %s. Use +ifname or -ifname.\n", br->dev->name);
+	ret = -EPERM;
+out:
+	return ret ? ret: len;
+}
+
+static DEVICE_ATTR(interfaces, S_IRUGO | S_IWUSR, show_interfaces, store_interfaces);
+
 static struct attribute *bridge_attrs[] = {
 	&dev_attr_forward_delay.attr,
 	&dev_attr_hello_time.attr,
@@ -424,6 +488,7 @@ static struct attribute *bridge_attrs[] = {
 	&dev_attr_gc_timer.attr,
 	&dev_attr_group_addr.attr,
 	&dev_attr_flush.attr,
+	&dev_attr_interfaces.attr,
 	NULL
 };
 
-- 
1.5.5.1

Re: [RFC PATCH 0/2] Allow full bridge configuration via sysfs

[Patrick McHardy]

Bill Nottingham wrote:
> Right now, you can configure most bridge device parameters via sysfs.
> However, you cannot either:
> - add or remove bridge interfaces
> - add or remove physical interfaces from a bridge
> 
> The attached patch set rectifies this. With this patch set, brctl
> (theoretically) becomes completely optional, much like ifenslave is
> now for bonding. (In fact, the idea for this patch, and the syntax
> used herein, is inspired by the sysfs bonding configuration.)

Both should use netlink instead of extending their sysfs interfaces.
For bridging I have a patch for the bridge device itself, the API
is so far missing support for adding ports though.

Re: [RFC PATCH 0/2] Allow full bridge configuration via sysfs

[Bill Nottingham]

Patrick McHardy (kaber@trash.net) said: 
> Bill Nottingham wrote:
>> Right now, you can configure most bridge device parameters via sysfs.
>> However, you cannot either:
>> - add or remove bridge interfaces
>> - add or remove physical interfaces from a bridge
>>
>> The attached patch set rectifies this. With this patch set, brctl
>> (theoretically) becomes completely optional, much like ifenslave is
>> now for bonding. (In fact, the idea for this patch, and the syntax
>> used herein, is inspired by the sysfs bonding configuration.)
>
> Both should use netlink instead of extending their sysfs interfaces.
> For bridging I have a patch for the bridge device itself, the API
> is so far missing support for adding ports though.

How does that improve the situation for bridge devices? Are all
bridging parameters (forward_delay, stp, etc.) going to be configurable
via netlink, or would we still then have multiple tools/interfaces
to configuration? Also, moving bonding configuration to netlink seems
like a step backwards.

Bill

Re: [RFC PATCH 0/2] Allow full bridge configuration via sysfs

[Patrick McHardy]

Bill Nottingham wrote:
> Patrick McHardy (kaber@trash.net) said: 
>> Bill Nottingham wrote:
>>> Right now, you can configure most bridge device parameters via sysfs.
>>> However, you cannot either:
>>> - add or remove bridge interfaces
>>> - add or remove physical interfaces from a bridge
>>>
>>> The attached patch set rectifies this. With this patch set, brctl
>>> (theoretically) becomes completely optional, much like ifenslave is
>>> now for bonding. (In fact, the idea for this patch, and the syntax
>>> used herein, is inspired by the sysfs bonding configuration.)
>> Both should use netlink instead of extending their sysfs interfaces.
>> For bridging I have a patch for the bridge device itself, the API
>> is so far missing support for adding ports though.
> 
> How does that improve the situation for bridge devices? Are all
> bridging parameters (forward_delay, stp, etc.) going to be configurable
> via netlink, or would we still then have multiple tools/interfaces
> to configuration?

Of course its all going to be configurable via netlink, otherwise
it really wouldn't make sense.

> Also, moving bonding configuration to netlink seems
> like a step backwards.

Please read up on what the standard interface for network
configuration is, I'm tired of reiterating this once a week.

Re: [RFC PATCH 0/2] Allow full bridge configuration via sysfs

[Bill Nottingham]

Patrick McHardy (kaber@trash.net) said: 
>> Also, moving bonding configuration to netlink seems
>> like a step backwards.
>
> Please read up on what the standard interface for network
> configuration is

OK, let's see.

In the code: Hm, no TODO or FIXME.

In the included documentation:

Documentation/networking/bonding.txt:
  Module options, modprobe.conf, or 'distro-specific
  configuration file', ifenslave, or sysfs.

Documentation/networking/ip-sysctl.txt:
  sysctl, obviously.

Documentation/networking/generic_netlink.txt
  Hey, here's netlink! Doucmentation points only to a wiki. Referred
  to by zero other included in-kernel documentation.

Well, that's helpful.

Let's try the OSDL web!

http://www.linuxfoundation.org/en/Net:Bridge
  brctl (which uses ioctl and sysfs). And /etc/net.
  
http://www.linuxfoundation.org/en/Net:Bonding
  Module parameters only, including the lovely 'load driver multiple times'
  method. Doesn't even mention sysfs.

http://www.linuxfoundation.org/en/Net:VLAN
  vconfig
  
I could look at wireless network configuration, but I doubt that's going to
help your argument.

> I'm tired of reiterating this once a week.

Well, if the documentation that described this as the standard existed,
or wasn't such crap, perhaps you wouldn't have to.

That being said, how is moving from adding a bonding slave from:
  echo "+eth0" >  /sys/class/net/bond0/bonding/slaves to:
to:
  http://www.linuxfoundation.org/en/Net:Generic_Netlink_HOWTO

a worthwhile improvement for the admin? Let's see, a kernel-userspace
protocol with magic message formats. Hey, we reinvented ioctl!

Why, if netlink is the standard (and it's been around for a long
damn time), was sysfs configuration for bonding added in 2005? Why
was bridge configuration added in 2005, and *extended* in 2006 and
2007? Why were the user-space tools such as brctl ported from ioctl
to sysfs?

Bill

Re: [RFC PATCH 0/2] Allow full bridge configuration via sysfs

[David Miller]

From: Bill Nottingham <notting@redhat.com>
Date: Mon, 7 Jul 2008 17:34:20 -0400

> I could look at wireless network configuration, but I doubt that's going to
> help your argument.

Just like any system with age, we have a lot of legacy to
convert over.  But it will happen.

> That being said, how is moving from adding a bonding slave from:
>   echo "+eth0" >  /sys/class/net/bond0/bonding/slaves to:
> to:
>   http://www.linuxfoundation.org/en/Net:Generic_Netlink_HOWTO
> 
> a worthwhile improvement for the admin? Let's see, a kernel-userspace
> protocol with magic message formats. Hey, we reinvented ioctl!
> 
> Why, if netlink is the standard (and it's been around for a long
> damn time), was sysfs configuration for bonding added in 2005? Why
> was bridge configuration added in 2005, and *extended* in 2006 and
> 2007? Why were the user-space tools such as brctl ported from ioctl
> to sysfs?

Because often a lot of shit slips in when someone who understands
the ramifications is too busy or on vacation.

We do want everything to be netlink based.

Why?

Because it means that you can run one monitoring tool to listen
for netlink events and report them to the user for diagnosis.

It means that network configuration events can be sent over
the wire and used remotely at some point.

The latter can never happen as long as we keep adding ad-hoc
config stuff.

Re: [Bridge] [RFC PATCH 0/2] Allow full bridge configuration via sysfs

[Stephen Hemminger]

On Mon, 07 Jul 2008 14:52:59 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Bill Nottingham <notting@redhat.com>
> Date: Mon, 7 Jul 2008 17:34:20 -0400
> 
> > I could look at wireless network configuration, but I doubt that's going to
> > help your argument.
> 
> Just like any system with age, we have a lot of legacy to
> convert over.  But it will happen.
> 
> > That being said, how is moving from adding a bonding slave from:
> >   echo "+eth0" >  /sys/class/net/bond0/bonding/slaves to:
> > to:
> >   http://www.linuxfoundation.org/en/Net:Generic_Netlink_HOWTO
> > 
> > a worthwhile improvement for the admin? Let's see, a kernel-userspace
> > protocol with magic message formats. Hey, we reinvented ioctl!
> > 
> > Why, if netlink is the standard (and it's been around for a long
> > damn time), was sysfs configuration for bonding added in 2005? Why
> > was bridge configuration added in 2005, and *extended* in 2006 and
> > 2007? Why were the user-space tools such as brctl ported from ioctl
> > to sysfs?
> 
> Because often a lot of shit slips in when someone who understands
> the ramifications is too busy or on vacation.
> 
> We do want everything to be netlink based.
> 
> Why?
> 
> Because it means that you can run one monitoring tool to listen
> for netlink events and report them to the user for diagnosis.
> 
> It means that network configuration events can be sent over
> the wire and used remotely at some point.
> 
> The latter can never happen as long as we keep adding ad-hoc
> config stuff.

There are always historical reasons. In this case it was because I knew
more about sysfs than netlink, and there was no netlink interface for managing
interfaces back in 2005.  Sysfs is okay for simple stuff (set forward-delay to 10seconds),
but it falls down when anything interesting and transactional happens. Think of
sysfs as more an extension of per-device sysctl's or module parameters, rather
than a good configuration interface.

Re: [RFC PATCH 0/2] Allow full bridge configuration via sysfs

[Bill Nottingham]

David Miller (davem@davemloft.net) said: 
> > Why, if netlink is the standard (and it's been around for a long
> > damn time), was sysfs configuration for bonding added in 2005? Why
> > was bridge configuration added in 2005, and *extended* in 2006 and
> > 2007? Why were the user-space tools such as brctl ported from ioctl
> > to sysfs?
> 
> Because often a lot of shit slips in when someone who understands
> the ramifications is too busy or on vacation.

Duly noted, will time all patch submissions to land during your
vacations in the future.

More seriously, if there's not a mechanism to prevent ABIs the
kernel doesn't want like this being added, that's a problem.

> We do want everything to be netlink based.
> 
> Why?
> 
> Because it means that you can run one monitoring tool to listen
> for netlink events and report them to the user for diagnosis.
> 
> It means that network configuration events can be sent over
> the wire and used remotely at some point.
> 
> The latter can never happen as long as we keep adding ad-hoc
> config stuff.

Sure, but it does make them more opaque to the normal user,
leaving them wrapped in the same old ip/brctl/ifenslave/vconfig
tools - for better or worse, people like the discoverability
and obviousness of sysfs.

Bill