Skip to contentSkip to conversation

bridge: sysfs cleanup

PATCH In review Wang Chen 3 participants 7 messages Jun 20, 2008 – Jun 21, 2008

Subsystems (3)
Networking Network drivers Ethernet bridge
lore
Share & tools
lore

Wang Chen

author
#0 lore raw

PATCH 4/8 bridge: sysfs cleanup

When I am doing this series of patch, I notice that bridge's sysfs are not cleanuped after failure and bridge delete.

Here is the patch for it.

Signed-off-by Wang Chen <[email protected]>
---
 net/bridge/br_if.c       |    2 ++
 net/bridge/br_sysfs_if.c |    9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)
net/bridge/br_if.c +2 -0
index 49dd433..4b730fa 100644
@@ -161,6 +161,7 @@ static void del_br(struct net_bridge *br)
 	struct net_bridge_port *p, *n;
 
 	list_for_each_entry_safe(p, n, &br->port_list, list) {
+		br_sysfs_removeif(p);
 		del_nbp(p);
 	}
 

@@ -435,6 +436,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	if (!p || p->br != br)
 		return -EINVAL;
 
+	br_sysfs_removeif(p);
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
net/bridge/br_sysfs_if.c +8 -1
index 7f80462..9e9d17c 100644
@@ -226,10 +226,17 @@ int br_sysfs_addif(struct net_bridge_port *p)
 	for (a = brport_attrs; *a; ++a) {
 		err = sysfs_create_file(&p->kobj, &((*a)->attr));
 		if (err)
-			goto out2;
+			goto out1;
 	}
 
 	err = sysfs_create_link(br->ifobj, &p->kobj, p->dev->name);
+	if (err)
+		goto out1;
+	return 0;
+out1:
+	for (a = brport_attrs; *a; ++a)
+		sysfs_remove_file(&p->kobj, &((*a)->attr));
+	sysfs_remove_link(&br->dev->dev.kobj, SYSFS_BRIDGE_PORT_LINK);
 out2:
 	return err;
 }
Signature · 1 line
1.5.3.4
#1 lore raw
Quoted 6 lines · On Fri, 20 Jun 2008 08:55:15 +0800 Wang Chen <[email protected]> wrote:

When I am doing this series of patch, I notice that bridge's sysfs are not cleanuped after failure and bridge delete.

Here is the patch for it.

Signed-off-by Wang Chen <[email protected]>

Let me look at this, I don't think it used to be a problem before the conversion from class_device to just device model.

author
#2 lore raw

Stephen Hemminger said the following on 2008-6-20 9:19:

Quoted 6 lines
Quoted 3 lines · On Fri, 20 Jun 2008 08:55:15 +0800 Wang Chen <[email protected]> wrote:

When I am doing this series of patch, I notice that bridge's sysfs are not cleanuped after failure and bridge delete.

Quoted 7 lines
Quoted 3 lines

Here is the patch for it.

Signed-off-by Wang Chen <[email protected]>

Let me look at this, I don't think it used to be a problem before the conversion from class_device to just device model.

But, sysfs_create_link/file will leave some junk after failing. Don't we care about that?

author
#3 lore raw

Wang Chen said the following on 2008-6-20 10:17:

Quoted 9 lines

Stephen Hemminger said the following on 2008-6-20 9:19:

Quoted 8 lines
Quoted 5 lines · On Fri, 20 Jun 2008 08:55:15 +0800 Wang Chen <[email protected]> wrote:

When I am doing this series of patch, I notice that bridge's sysfs are not cleanuped after failure and bridge delete.

Here is the patch for it.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Wang Chen <[email protected]>

Let me look at this, I don't think it used to be a problem before the conversion from class_device to just device model.

But, sysfs_create_link/file will leave some junk after failing. Don't we care about that?

Stephen, since I changed PATCH 3/8, I have to resend a new one for 4/8.
But, about whether this fix is needed, we can talk.

---
Do sysfs cleanup after failure and bridge deleting.

---
 net/bridge/br_if.c       |    2 ++
 net/bridge/br_private.h  |    2 ++
 net/bridge/br_sysfs_if.c |   26 +++++++++++++++++++++++++-
 3 files changed, 29 insertions(+), 1 deletions(-)
net/bridge/br_if.c +2 -0
index 805dbb7..5d22d23 100644
@@ -161,6 +161,7 @@ static void del_br(struct net_bridge *br)
 	struct net_bridge_port *p, *n;
 
 	list_for_each_entry_safe(p, n, &br->port_list, list) {
+		br_sysfs_removeif(p);
 		del_nbp(p);
 	}
 

@@ -434,6 +435,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	if (!p || p->br != br)
 		return -EINVAL;
 
+	br_sysfs_removeif(p);
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
net/bridge/br_private.h +2 -0
index c11b554..c1bbfea 100644
@@ -250,6 +250,7 @@ extern void br_ifinfo_notify(int event, struct net_bridge_port *port);
 /* br_sysfs_if.c */
 extern struct sysfs_ops brport_sysfs_ops;
 extern int br_sysfs_addif(struct net_bridge_port *p);
+extern void br_sysfs_removeif(struct net_bridge_port *p);
 
 /* br_sysfs_br.c */
 extern int br_sysfs_addbr(struct net_device *dev);

@@ -258,6 +259,7 @@ extern void br_sysfs_delbr(struct net_device *dev);
 #else
 
 #define br_sysfs_addif(p)	(0)
+#define br_sysfs_removeif(p)	do { } while (0)
 #define br_sysfs_addbr(dev)	(0)
 #define br_sysfs_delbr(dev)	do { } while(0)
 #endif /* CONFIG_SYSFS */
net/bridge/br_sysfs_if.c +25 -1
index 02b2d50..9e9d17c 100644
@@ -226,10 +226,34 @@ int br_sysfs_addif(struct net_bridge_port *p)
 	for (a = brport_attrs; *a; ++a) {
 		err = sysfs_create_file(&p->kobj, &((*a)->attr));
 		if (err)
-			goto out2;
+			goto out1;
 	}
 
 	err = sysfs_create_link(br->ifobj, &p->kobj, p->dev->name);
+	if (err)
+		goto out1;
+	return 0;
+out1:
+	for (a = brport_attrs; *a; ++a)
+		sysfs_remove_file(&p->kobj, &((*a)->attr));
+	sysfs_remove_link(&br->dev->dev.kobj, SYSFS_BRIDGE_PORT_LINK);
 out2:
 	return err;
 }
+
+/*
+ * Remove sysfs entries of ethernet device added to a bridge.
+ * Revert all the things be done by br_sysfs_addif().
+ */
+void br_sysfs_removeif(struct net_bridge_port *p)
+{
+	struct net_bridge *br = p->br;
+	struct brport_attribute **a;
+
+	sysfs_remove_link(&p->kobj, p->dev->name);
+
+	for (a = brport_attrs; *a; ++a)
+		sysfs_remove_file(&p->kobj, &((*a)->attr));
+
+	sysfs_remove_link(&br->dev->dev.kobj, SYSFS_BRIDGE_PORT_LINK);
+}
Signature · 1 line
1.5.3.4
#4 lore raw
In reply to Wang Chen
Quoted 44 lines · On Fri, 20 Jun 2008 08:55:15 +0800 Wang Chen <[email protected]> wrote:

When I am doing this series of patch, I notice that bridge's sysfs are not cleanuped after failure and bridge delete.

Here is the patch for it.

Signed-off-by Wang Chen <[email protected]>
---
 net/bridge/br_if.c       |    2 ++
 net/bridge/br_sysfs_if.c |    9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)
net/bridge/br_if.c +2 -0
index 49dd433..4b730fa 100644
@@ -161,6 +161,7 @@ static void del_br(struct net_bridge *br)
 	struct net_bridge_port *p, *n;
 
 	list_for_each_entry_safe(p, n, &br->port_list, list) {
+		br_sysfs_removeif(p);
 		del_nbp(p);
 	}
 

@@ -435,6 +436,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	if (!p || p->br != br)
 		return -EINVAL;
 
+	br_sysfs_removeif(p);
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
net/bridge/br_sysfs_if.c +1 -1
index 7f80462..9e9d17c 100644
@@ -226,10 +226,17 @@ int br_sysfs_addif(struct net_bridge_port *p)
 	for (a = brport_attrs; *a; ++a) {
 		err = sysfs_create_file(&p->kobj, &((*a)->attr));
 		if (err)
-			goto out2;
+			goto out1;
 	}
 
 	err = sysfs_create_link(br->ifobj, &p->kobj, p->dev->name);

net/bridge/br_sysfs_if.c int br_sysfs_addif(struct net_bridge_port *p) · line 242

Quoted 10 lines
net/bridge/br_sysfs_if.c +7 -0
+	if (err)
+		goto out1;
+	return 0;
+out1:
+	for (a = brport_attrs; *a; ++a)
+		sysfs_remove_file(&p->kobj, &((*a)->attr));
+	sysfs_remove_link(&br->dev->dev.kobj, SYSFS_BRIDGE_PORT_LINK);
 out2:
 	return err;
 }

This is unneeded since the bridge port kobject is deleted when removed or in case of error and kobject_del calls sysfs_remove_dir.

author
#5 lore raw

Stephen Hemminger said the following on 2008-6-21 1:44:

Quoted 46 lines
Quoted 43 lines · On Fri, 20 Jun 2008 08:55:15 +0800 Wang Chen <[email protected]> wrote:

When I am doing this series of patch, I notice that bridge's sysfs are not cleanuped after failure and bridge delete.

Here is the patch for it.

Signed-off-by Wang Chen <[email protected]>
---
 net/bridge/br_if.c       |    2 ++
 net/bridge/br_sysfs_if.c |    9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)
net/bridge/br_if.c +2 -0
index 49dd433..4b730fa 100644
@@ -161,6 +161,7 @@ static void del_br(struct net_bridge *br)
 	struct net_bridge_port *p, *n;
 
 	list_for_each_entry_safe(p, n, &br->port_list, list) {
+		br_sysfs_removeif(p);
 		del_nbp(p);
 	}
 

@@ -435,6 +436,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	if (!p || p->br != br)
 		return -EINVAL;
 
+	br_sysfs_removeif(p);
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
net/bridge/br_sysfs_if.c +1 -1
index 7f80462..9e9d17c 100644
@@ -226,10 +226,17 @@ int br_sysfs_addif(struct net_bridge_port *p)
 	for (a = brport_attrs; *a; ++a) {
 		err = sysfs_create_file(&p->kobj, &((*a)->attr));
 		if (err)
-			goto out2;
+			goto out1;
 	}
Quoted 15 lines

net/bridge/br_sysfs_if.c int br_sysfs_addif(struct net_bridge_port *p) · line 242

Quoted 11 lines
net/bridge/br_sysfs_if.c +7 -0
 	err = sysfs_create_link(br->ifobj, &p->kobj, p->dev->name);
+	if (err)
+		goto out1;
+	return 0;
+out1:
+	for (a = brport_attrs; *a; ++a)
+		sysfs_remove_file(&p->kobj, &((*a)->attr));
+	sysfs_remove_link(&br->dev->dev.kobj, SYSFS_BRIDGE_PORT_LINK);
 out2:
 	return err;
 }

This is unneeded since the bridge port kobject is deleted when removed or in case of error and kobject_del calls sysfs_remove_dir.

Stephen, thank you for your explanation. Since kobject_del will unwind what sysfs_create_* did, I also think this patch is unneeded.

David, would you please ignore this patch.