Skip to contentSkip to conversation

net: Avoid problems with bonding and device rename

PATCH In review Stephen Hemminger 5 participants 25 messages May 15, 2008 – Jun 10, 2008 1 message not in the archive

Subsystems (4)
Networking Network drivers Driver core Bonding driver
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

PATCH 1/1 net: Avoid problems with bonding and device rename

See: http://bugzilla.kernel.org/show_bug.cgi?id=10698

The use of /sys/class/net/bonding_masters was a poor ABI choice that now we have to live with. Best choice is to just block other usage of that name.

Signed-off-by Stephen Hemminger <[email protected]>
net/core/dev.c +3 -0
@@ -758,6 +758,9 @@ int dev_valid_name(const char *name)
 		return 0;
 	if (!strcmp(name, ".") || !strcmp(name, ".."))
 		return 0;
+	/* Sigh. need better ABI discipline -- see bond_sysfs */
+	if (!strcmp(name, "bonding_masters"))
+		return 0;
 
 	while (*name) {
 		if (*name == '/' || isspace(*name))
maintainer
#1 lore raw

From: Stephen Hemminger <[email protected]>
Date: Wed, 14 May 2008 17:03:16 -0700

Quoted 7 lines

See: http://bugzilla.kernel.org/show_bug.cgi?id=10698

The use of /sys/class/net/bonding_masters was a poor ABI choice that now we have to live with. Best choice is to just block other usage of that name.

Signed-off-by Stephen Hemminger <[email protected]>

I think sysfs is way too noisy in this case and we should handle the sysfs add failure just like any other -EEXIST case.

A message that is not in the archive (sent off-list or not yet received): 20080514181257.74fbb5aa@extreme

author
#3 lore raw

PATCH 2/3 sysfs: remove error messages for -EEXIST case

It is possible that the entry in sysfs already exists, one case of this is when a network device is renamed to bonding_masters. Anyway, in this case the proper error path is for device_rename to return an error code, not to generate bogus backtrace and errors.

Also, to avoid possible races, the create link should be done before the remove link. This makes a device rename atomic operation like other renames.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/base/core.c +3 -5
@@ -1218,13 +1218,11 @@ int device_rename(struct device *dev, ch
 	}
 #else
 	if (dev->class) {
-		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
 		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
 					  dev->bus_id);
-		if (error) {
-			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
-				__func__, error);
-		}
+		if (error)
+			goto out;
+		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
 	}
 #endif
fs/sysfs/dir.c +1 -5
@@ -419,12 +419,8 @@ void sysfs_addrm_start(struct sysfs_addr
  */
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
-	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
-		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
-		WARN_ON(1);
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;
-	}
 
 	sd->s_parent = sysfs_get(acxt->parent_sd);
maintainer
#4 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

From: Stephen Hemminger <[email protected]>
Date: Wed, 14 May 2008 18:16:03 -0700

Quoted 9 lines

It is possible that the entry in sysfs already exists, one case of this is when a network device is renamed to bonding_masters. Anyway, in this case the proper error path is for device_rename to return an error code, not to generate bogus backtrace and errors.

Also, to avoid possible races, the create link should be done before the remove link. This makes a device rename atomic operation like other renames.

Signed-off-by Stephen Hemminger <[email protected]>

I definitely agree with this change.

We have several cases where device names are user configurable, yet the devices live in a directory which also has subdirectories created by other subsystems.

It's pointless to require the top-level guy to look for any purge out any subdirectory cases, that's none of it's business.

I realize the backtrace is useful for finding bugs, but in this case it's definitely not appropriate.

#5 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

Quoted 19 lines · David Miller

From: Stephen Hemminger <[email protected]>
Date: Wed, 14 May 2008 18:16:03 -0700

Quoted 9 lines

It is possible that the entry in sysfs already exists, one case of this is when a network device is renamed to bonding_masters. Anyway, in this case the proper error path is for device_rename to return an error code, not to generate bogus backtrace and errors.

Also, to avoid possible races, the create link should be done before the remove link. This makes a device rename atomic operation like other renames.

Signed-off-by Stephen Hemminger <[email protected]>

I definitely agree with this change.

We have several cases where device names are user configurable, yet the devices live in a directory which also has subdirectories created by other subsystems.

Quoted 5 lines

It's pointless to require the top-level guy to look for any purge out any subdirectory cases, that's none of it's business.

I realize the backtrace is useful for finding bugs, but in this case it's definitely not appropriate.

Fair enough, I have no objection to these.

David, do you want me to take them through my tree? Or are they dependant on the network core changes as well? If so, I have no problem you taking them. Feel free to add a:

Signed-off-by Greg Kroah-Hartman <[email protected]>

to them in that case.

thanks,

greg k-h

maintainer
#6 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

From: Greg KH <[email protected]>
Date: Wed, 14 May 2008 20:14:58 -0700

Quoted 7 lines

Fair enough, I have no objection to these.

David, do you want me to take them through my tree? Or are they dependant on the network core changes as well? If so, I have no problem you taking them. Feel free to add a:

Signed-off-by Greg Kroah-Hartman <[email protected]>

to them in that case.

I'll take them, thanks for reviewing and the signoff Greg.

maintainer Applied
#7 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Wed, 14 May 2008 18:16:03 -0700

Quoted 9 lines

It is possible that the entry in sysfs already exists, one case of this is when a network device is renamed to bonding_masters. Anyway, in this case the proper error path is for device_rename to return an error code, not to generate bogus backtrace and errors.

Also, to avoid possible races, the create link should be done before the remove link. This makes a device rename atomic operation like other renames.

Signed-off-by Stephen Hemminger <[email protected]>

Applied.

#8 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

In reply to Stephen Hemminger
Quoted 9 lines · Stephen Hemminger
drivers/base/core.c +0 -1
@@ -1218,13 +1218,11 @@ int device_rename(struct device *dev, ch
 	}
 #else
 	if (dev->class) {
-		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
 		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
 					  dev->bus_id);

drivers/base/core.c int device_rename(struct device *dev, ch · line 1228

Quoted 10 lines
drivers/base/core.c +3 -4
-		if (error) {
-			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
-				__func__, error);
-		}
+		if (error)
+			goto out;
+		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
 	}
 #endif

This looks reasonable...

Quoted 5 lines
fs/sysfs/dir.c +0 -0
@@ -419,12 +419,8 @@ void sysfs_addrm_start(struct sysfs_addr
  */
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)

fs/sysfs/dir.c void sysfs_addrm_start(struct sysfs_addr · line 425

Quoted 10 lines
fs/sysfs/dir.c +1 -5
 {
-	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
-		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
-		WARN_ON(1);
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;
-	}
 
 	sd->s_parent = sysfs_get(acxt->parent_sd);

...but this will cause many useful warnings to disappear.

What to do here?

- Rely on people checking all __must_check stuff and printing a warning when desired. Not everyone seems to like that (see, for example,
http://marc.info/?l=linux-kernel&m=121012176111154&w=2).
- Put the burden unto the callers of sysfs_add_one() (or maybe even further up). They should hopefully know whether -EEXIST is "might happen" or "argh, we've messed up" (and print a warning in the latter case so that it gets reported).
- Make this a debugging thing. Unfortunately then we won't neccessarily get reports when things are busted.

maintainer
#9 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

From: Cornelia Huck <[email protected]>
Date: Thu, 15 May 2008 09:52:46 +0200

Quoted 10 lines
Quoted 10 lines

int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) {

fs/sysfs/dir.c +1 -5
-	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
-		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
-		WARN_ON(1);
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;
-	}
Quoted 5 lines
Quoted 1 line

sd->s_parent = sysfs_get(acxt->parent_sd);

...but this will cause many useful warnings to disappear.

What to do here?

Make a __sysfs_add_one() that doesn't warn. Make
sysfs_add_one() be a wrapper around __sysfs_add_one() that warns.

Change networking to call __sysfs_add_one().

Repeat and rinse up the call chain, as needed.

#10 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

Quoted 17 lines · David Miller

From: Cornelia Huck <[email protected]>
Date: Thu, 15 May 2008 09:52:46 +0200

Quoted 14 lines

Review

Quoted 11 lines

int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) {

(quoted diff) +1 -5
-	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
-		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
-		WARN_ON(1);
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;
-	}
 
 	sd->s_parent = sysfs_get(acxt->parent_sd);

...but this will cause many useful warnings to disappear.

Quoted 7 lines
Quoted 1 line

What to do here?

Make a __sysfs_add_one() that doesn't warn. Make
sysfs_add_one() be a wrapper around __sysfs_add_one() that warns.

Change networking to call __sysfs_add_one().

Like this (not even compile tested)? This depends on being able to properly isolate the functions called though, or we need to get the nowarn flag one level further up.

drivers/base/core.c +4 -4
index be288b5..a9058f4 100644
@@ -1209,8 +1209,8 @@ #ifdef CONFIG_SYSFS_DEPRECATED
 	if (old_class_name) {
 		new_class_name = make_class_name(dev->class->name, &dev->kobj);
 		if (new_class_name) {
-			error = sysfs_create_link(&dev->parent->kobj,
-						  &dev->kobj, new_class_name);
+			error = __sysfs_create_link(&dev->parent->kobj,
+						  &dev->kobj, new_class_name, 1);
 			if (error)
 				goto out;
 			sysfs_remove_link(&dev->parent->kobj, old_class_name);

@@ -1219,8 +1219,8 @@ #ifdef CONFIG_SYSFS_DEPRECATED
 #else
 	if (dev->class) {
 		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
-		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
-					  dev->bus_id);
+		error = __sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
+					  dev->bus_id, 1);
 		if (error) {
 			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
 				__func__, error);
fs/sysfs/dir.c +21 -13
index a1c3a1f..b7914a0 100644
@@ -397,6 +397,23 @@ void sysfs_addrm_start(struct sysfs_addr
 		iput(inode);
 }
 
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+{
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
+		return -EEXIST;
+
+	sd->s_parent = sysfs_get(acxt->parent_sd);
+
+	if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)
+		inc_nlink(acxt->parent_inode);
+
+	acxt->cnt++;
+
+	sysfs_link_sibling(sd);
+
+	return 0;
+}
+
 /**
  *	sysfs_add_one - add sysfs_dirent to parent
  *	@acxt: addrm context to use

@@ -419,23 +436,14 @@ void sysfs_addrm_start(struct sysfs_addr
  */
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
-	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
+	int err = __sysfs_add_one(acxt, sd);
+
+	if (err == -EEXIST) {
 		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
 		       "can not be created\n", sd->s_name);
 		WARN_ON(1);
-		return -EEXIST;
 	}
-
-	sd->s_parent = sysfs_get(acxt->parent_sd);
-
-	if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)
-		inc_nlink(acxt->parent_inode);
-
-	acxt->cnt++;
-
-	sysfs_link_sibling(sd);
-
-	return 0;
+	return err;
 }
 
 /**
fs/sysfs/symlink.c +12 -2
index 817f596..c08184d 100644
@@ -25,7 +25,8 @@ #include "sysfs.h"
  *	@target:	object we're pointing to.
  *	@name:		name of the symlink.
  */
-int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char * name)
+int __sysfs_create_link(struct kobject * kobj, struct kobject * target,
+			const char * name, int nowarn)
 {
 	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;

@@ -65,7 +66,10 @@ int sysfs_create_link(struct kobject * k
 	target_sd = NULL;	/* reference is now owned by the symlink */
 
 	sysfs_addrm_start(&acxt, parent_sd);
-	error = sysfs_add_one(&acxt, sd);
+	if (nowarn)
+		error = __sysfs_add_one(&acxt, sd);
+	else
+		error = sysfs_add_one(&acxt, sd);
 	sysfs_addrm_finish(&acxt);
 
 	if (error)

@@ -79,6 +83,12 @@ int sysfs_create_link(struct kobject * k
 	return error;
 }
 
+int sysfs_create_link(struct kobject * kobj, struct kobject * target,
+		      const char * name)
+{
+	return sysfs_create_link(kobj, target, name, 0);
+}
+
 /**
  *	sysfs_remove_link - remove symlink in object's directory.
  *	@kobj:	object we're acting for.
fs/sysfs/sysfs.h +1 -0
index ce4e15f..1d459f5 100644
@@ -108,6 +108,7 @@ void sysfs_put_active_two(struct sysfs_d
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
include/linux/sysfs.h +2 -0
index 7858eac..3ef8ae7 100644
@@ -101,6 +101,8 @@ void sysfs_remove_bin_file(struct kobjec
 
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
+int __sysfs_create_link(struct kobject *kobj, struct kobject *target,
+			const char *name, int nowarn);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
 int __must_check sysfs_create_group(struct kobject *kobj,
maintainer
#11 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

From: Cornelia Huck <[email protected]>
Date: Thu, 15 May 2008 11:31:31 +0200

Quoted 22 lines
Quoted 19 lines · David Miller

From: Cornelia Huck <[email protected]>
Date: Thu, 15 May 2008 09:52:46 +0200

Quoted 15 lines

Review

Quoted 11 lines

int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) {

(quoted diff) +1 -5
-	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
-		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
-		WARN_ON(1);
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;
-	}
 
 	sd->s_parent = sysfs_get(acxt->parent_sd);

...but this will cause many useful warnings to disappear.

What to do here?

Quoted 7 lines
Quoted 5 lines

Make a __sysfs_add_one() that doesn't warn. Make
sysfs_add_one() be a wrapper around __sysfs_add_one() that warns.

Change networking to call __sysfs_add_one().

Like this (not even compile tested)?

No, the idea is that the __sysfs_add_one() wouldn't need a parameter at all. sysfs_add_one() just needs to check for -EEXIST from __sysfs_add_one() to decide whether to warn or not.

#12 lore raw

Re: [PATCH 2/3] sysfs: remove error messages for -EEXIST case

Quoted 3 lines · David Miller

No, the idea is that the __sysfs_add_one() wouldn't need a parameter at all. sysfs_add_one() just needs to check for -EEXIST from __sysfs_add_one() to decide whether to warn or not.

But that is what the patch does? Only the upper layers need to pass a flag around.

#13 lore raw

PATCH 1/1 driver core: Suppress sysfs warnings for device_rename().

OK, here is an actually-compiled patch with proper description and s-o-b. Comments?

-----

driver core: Suppress sysfs warnings for device_rename().

Renaming network devices to an already existing name is not something we want sysfs to print a scary warning for, since the callers can deal with this correctly. So let's introduce sysfs_create_link_nowarn() which gets rid of the common warning.

Signed-off-by Cornelia Huck <[email protected]>
---
 drivers/base/core.c   |   17 ++++++++---------
 fs/sysfs/dir.c        |   37 +++++++++++++++++++++++++++++++++++--
 fs/sysfs/symlink.c    |   41 +++++++++++++++++++++++++++++++++--------
 fs/sysfs/sysfs.h      |    1 +
 include/linux/sysfs.h |   10 ++++++++++
 5 files changed, 87 insertions(+), 19 deletions(-)
drivers/base/core.c +8 -9
Index: linux-2.6/drivers/base/core.c
===================================================================
@@ -1287,8 +1287,9 @@ int device_rename(struct device *dev, ch
 	if (old_class_name) {
 		new_class_name = make_class_name(dev->class->name, &dev->kobj);
 		if (new_class_name) {
-			error = sysfs_create_link(&dev->parent->kobj,
-						  &dev->kobj, new_class_name);
+			error = sysfs_create_link_nowarn(&dev->parent->kobj,
+							 &dev->kobj,
+							 new_class_name);
 			if (error)
 				goto out;
 			sysfs_remove_link(&dev->parent->kobj, old_class_name);

@@ -1296,13 +1297,11 @@ int device_rename(struct device *dev, ch
 	}
 #else
 	if (dev->class) {
-		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
-		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
-					  dev->bus_id);
-		if (error) {
-			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
-				__func__, error);
-		}
+		error = sysfs_create_link_nowarn(&dev->class->subsys.kobj,
+						 &dev->kobj, dev->bus_id);
+		if (!error)
+			sysfs_remove_link(&dev->class->subsys.kobj,
+					  old_device_name);
 	}
 #endif
fs/sysfs/dir.c +35 -2
Index: linux-2.6/fs/sysfs/dir.c
===================================================================
@@ -398,7 +398,7 @@ void sysfs_addrm_start(struct sysfs_addr
 }
 
 /**
- *	sysfs_add_one - add sysfs_dirent to parent
+ *	__sysfs_add_one - add sysfs_dirent to parent without warning
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be added
  *

@@ -417,7 +417,7 @@ void sysfs_addrm_start(struct sysfs_addr
  *	0 on success, -EEXIST if entry with the given name already
  *	exists.
  */
-int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
 	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name)) {
 		printk(KERN_WARNING "sysfs: duplicate filename '%s' "

@@ -439,6 +439,39 @@ int sysfs_add_one(struct sysfs_addrm_cxt
 }
 
 /**
+ *	sysfs_add_one - add sysfs_dirent to parent
+ *	@acxt: addrm context to use
+ *	@sd: sysfs_dirent to be added
+ *
+ *	Get @acxt->parent_sd and set sd->s_parent to it and increment
+ *	nlink of parent inode if @sd is a directory and link into the
+ *	children list of the parent.
+ *
+ *	This function should be called between calls to
+ *	sysfs_addrm_start() and sysfs_addrm_finish() and should be
+ *	passed the same @acxt as passed to sysfs_addrm_start().
+ *
+ *	LOCKING:
+ *	Determined by sysfs_addrm_start().
+ *
+ *	RETURNS:
+ *	0 on success, -EEXIST if entry with the given name already
+ *	exists.
+ */
+int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+{
+	int ret;
+
+	ret = __sysfs_add_one(acxt, sd);
+	if (ret == -EEXIST) {
+		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
+		       "can not be created\n", sd->s_name);
+		WARN_ON(1);
+	}
+	return ret;
+}
+
+/**
  *	sysfs_remove_one - remove sysfs_dirent from parent
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be removed
fs/sysfs/symlink.c +33 -8

59 lines not on this page · raw message

fs/sysfs/sysfs.h +1 -0
Index: linux-2.6/fs/sysfs/sysfs.h
===================================================================
@@ -107,6 +107,7 @@ struct sysfs_dirent *sysfs_get_active_tw
 void sysfs_put_active_two(struct sysfs_dirent *sd);
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
include/linux/sysfs.h +10 -0
Index: linux-2.6/include/linux/sysfs.h
===================================================================
@@ -103,6 +103,9 @@ void sysfs_remove_bin_file(struct kobjec
 
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
+int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
+					  struct kobject *target,
+					  const char *name);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
 int __must_check sysfs_create_group(struct kobject *kobj,

@@ -182,6 +185,13 @@ static inline int sysfs_create_link(stru
 	return 0;
 }
 
+static inline int sysfs_create_link_nowarn(struct kobject *kobj,
+					   struct kobject *target,
+					   const char *name)
+{
+	return 0;
+}
+
 static inline void sysfs_remove_link(struct kobject *kobj, const char *name)
 {
 }
author
#14 lore raw

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

Quoted 5 lines · On Tue, 20 May 2008 12:59:13 +0200 Cornelia Huck <[email protected]>…

OK, here is an actually-compiled patch with proper description and s-o-b. Comments?

-----

Quoted 9 lines

driver core: Suppress sysfs warnings for device_rename().

Renaming network devices to an already existing name is not something we want sysfs to print a scary warning for, since the callers can deal with this correctly. So let's introduce sysfs_create_link_nowarn() which gets rid of the common warning.

Signed-off-by Cornelia Huck <[email protected]>

This is still getting to be overkill. I prefer that the warnings always are removed.

#15 lore raw

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

Quoted 15 lines · Stephen Hemminger
Quoted 12 lines · On Tue, 20 May 2008 12:59:13 +0200 Cornelia Huck <[email protected]>…

OK, here is an actually-compiled patch with proper description and s-o-b. Comments?

-----

driver core: Suppress sysfs warnings for device_rename().

Renaming network devices to an already existing name is not something we want sysfs to print a scary warning for, since the callers can deal with this correctly. So let's introduce sysfs_create_link_nowarn() which gets rid of the common warning.

Quoted 5 lines
Quoted 2 lines
Signed-off-by Cornelia Huck <[email protected]>

This is still getting to be overkill. I prefer that the warnings always are removed.

No, I do not. They have found a lot of real bugs that have been going unnoticed for quite some time, and some new ones (like the current mess in the pci hotplug subsystem where two different drivers are controlling the same pci hotplug slots and not realizing it at all.)

So having the warning gone for rename() is fine, but I still want it there for new files that are being added to the system.

thanks,

greg k-h

Acked-by Greg Kroah-Hartman on v1 1/1
#16 lore raw

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

In reply to Cornelia Huck
Quoted 2 lines · Cornelia Huck

OK, here is an actually-compiled patch with proper description and s-o-b. Comments?

Looks good to me, feel free to add an:

Acked-by Greg Kroah-Hartman <[email protected]>

to it.

David, are you going to take this through your tree?

thanks,

greg k-h

Acked-by Greg Kroah-Hartman
#17 lore raw

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

Quoted 10 lines · Greg KH
Quoted 2 lines · Cornelia Huck

OK, here is an actually-compiled patch with proper description and s-o-b. Comments?

Looks good to me, feel free to add an:

Acked-by Greg Kroah-Hartman <[email protected]>

to it.

David, are you going to take this through your tree?

Here it is again, respun against today's git:

driver core: Suppress sysfs warnings for device_rename().

Renaming network devices to an already existing name is not something we want sysfs to print a scary warning for, since the callers can deal with this correctly. So let's introduce sysfs_create_link_nowarn() which gets rid of the common warning.

Acked-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>
---
 drivers/base/core.c   |    9 +++++----
 fs/sysfs/dir.c        |   37 +++++++++++++++++++++++++++++++++++--
 fs/sysfs/symlink.c    |   41 +++++++++++++++++++++++++++++++++--------
 fs/sysfs/sysfs.h      |    1 +
 include/linux/sysfs.h |   10 ++++++++++
 5 files changed, 84 insertions(+), 14 deletions(-)
drivers/base/core.c +5 -4
@@ -1282,8 +1282,9 @@ int device_rename(struct device *dev, ch
 	if (old_class_name) {
 		new_class_name = make_class_name(dev->class->name, &dev->kobj);
 		if (new_class_name) {
-			error = sysfs_create_link(&dev->parent->kobj,
-						  &dev->kobj, new_class_name);
+			error = sysfs_create_link_nowarn(&dev->parent->kobj,
+							 &dev->kobj,
+							 new_class_name);
 			if (error)
 				goto out;
 			sysfs_remove_link(&dev->parent->kobj, old_class_name);

@@ -1291,8 +1292,8 @@ int device_rename(struct device *dev, ch
 	}
 #else
 	if (dev->class) {
-		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
-					  dev->bus_id);
+		error = sysfs_create_link_nowarn(&dev->class->subsys.kobj,
+						 &dev->kobj, dev->bus_id);
 		if (error)
 			goto out;
 		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
fs/sysfs/dir.c +35 -2
@@ -398,7 +398,7 @@ void sysfs_addrm_start(struct sysfs_addr
 }
 
 /**
- *	sysfs_add_one - add sysfs_dirent to parent
+ *	__sysfs_add_one - add sysfs_dirent to parent without warning
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be added
  *

@@ -417,7 +417,7 @@ void sysfs_addrm_start(struct sysfs_addr
  *	0 on success, -EEXIST if entry with the given name already
  *	exists.
  */
-int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
 	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;

@@ -435,6 +435,39 @@ int sysfs_add_one(struct sysfs_addrm_cxt
 }
 
 /**
+ *	sysfs_add_one - add sysfs_dirent to parent
+ *	@acxt: addrm context to use
+ *	@sd: sysfs_dirent to be added
+ *
+ *	Get @acxt->parent_sd and set sd->s_parent to it and increment
+ *	nlink of parent inode if @sd is a directory and link into the
+ *	children list of the parent.
+ *
+ *	This function should be called between calls to
+ *	sysfs_addrm_start() and sysfs_addrm_finish() and should be
+ *	passed the same @acxt as passed to sysfs_addrm_start().
+ *
+ *	LOCKING:
+ *	Determined by sysfs_addrm_start().
+ *
+ *	RETURNS:
+ *	0 on success, -EEXIST if entry with the given name already
+ *	exists.
+ */
+int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+{
+	int ret;
+
+	ret = __sysfs_add_one(acxt, sd);
+	if (ret == -EEXIST) {
+		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
+		       "can not be created\n", sd->s_name);
+		WARN_ON(1);
+	}
+	return ret;
+}
+
+/**
  *	sysfs_remove_one - remove sysfs_dirent from parent
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be removed
fs/sysfs/symlink.c +33 -8

59 lines not on this page · raw message

fs/sysfs/sysfs.h +1 -0
@@ -107,6 +107,7 @@ struct sysfs_dirent *sysfs_get_active_tw
 void sysfs_put_active_two(struct sysfs_dirent *sd);
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
include/linux/sysfs.h +10 -0
@@ -101,6 +101,9 @@ void sysfs_remove_bin_file(struct kobjec
 
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
+int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
+					  struct kobject *target,
+					  const char *name);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
 int __must_check sysfs_create_group(struct kobject *kobj,

@@ -180,6 +183,13 @@ static inline int sysfs_create_link(stru
 	return 0;
 }
 
+static inline int sysfs_create_link_nowarn(struct kobject *kobj,
+					   struct kobject *target,
+					   const char *name)
+{
+	return 0;
+}
+
 static inline void sysfs_remove_link(struct kobject *kobj, const char *name)
 {
 }
#18 lore raw

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

On Wed, 21 May 2008 10:05:56 +0200,
Cornelia Huck <[email protected]> wrote:

Just digging through my backlog: Is there any further interest in this patch?

Quoted 220 lines
Quoted 10 lines · Greg KH
Quoted 2 lines · Cornelia Huck

OK, here is an actually-compiled patch with proper description and s-o-b. Comments?

Looks good to me, feel free to add an:

Acked-by Greg Kroah-Hartman <[email protected]>

to it.

David, are you going to take this through your tree?

Here it is again, respun against today's git:

driver core: Suppress sysfs warnings for device_rename().

Renaming network devices to an already existing name is not something we want sysfs to print a scary warning for, since the callers can deal with this correctly. So let's introduce sysfs_create_link_nowarn() which gets rid of the common warning.

Acked-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>
---
 drivers/base/core.c   |    9 +++++----
 fs/sysfs/dir.c        |   37 +++++++++++++++++++++++++++++++++++--
 fs/sysfs/symlink.c    |   41 +++++++++++++++++++++++++++++++++--------
 fs/sysfs/sysfs.h      |    1 +
 include/linux/sysfs.h |   10 ++++++++++
 5 files changed, 84 insertions(+), 14 deletions(-)
drivers/base/core.c +5 -4
@@ -1282,8 +1282,9 @@ int device_rename(struct device *dev, ch
 	if (old_class_name) {
 		new_class_name = make_class_name(dev->class->name, &dev->kobj);
 		if (new_class_name) {
-			error = sysfs_create_link(&dev->parent->kobj,
-						  &dev->kobj, new_class_name);
+			error = sysfs_create_link_nowarn(&dev->parent->kobj,
+							 &dev->kobj,
+							 new_class_name);
 			if (error)
 				goto out;
 			sysfs_remove_link(&dev->parent->kobj, old_class_name);

@@ -1291,8 +1292,8 @@ int device_rename(struct device *dev, ch
 	}
 #else
 	if (dev->class) {
-		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
-					  dev->bus_id);
+		error = sysfs_create_link_nowarn(&dev->class->subsys.kobj,
+						 &dev->kobj, dev->bus_id);
 		if (error)
 			goto out;
 		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
fs/sysfs/dir.c +35 -2
@@ -398,7 +398,7 @@ void sysfs_addrm_start(struct sysfs_addr
 }
 
 /**
- *	sysfs_add_one - add sysfs_dirent to parent
+ *	__sysfs_add_one - add sysfs_dirent to parent without warning
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be added
  *

@@ -417,7 +417,7 @@ void sysfs_addrm_start(struct sysfs_addr
  *	0 on success, -EEXIST if entry with the given name already
  *	exists.
  */
-int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
 	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
 		return -EEXIST;

@@ -435,6 +435,39 @@ int sysfs_add_one(struct sysfs_addrm_cxt
 }
 
 /**
+ *	sysfs_add_one - add sysfs_dirent to parent
+ *	@acxt: addrm context to use
+ *	@sd: sysfs_dirent to be added
+ *
+ *	Get @acxt->parent_sd and set sd->s_parent to it and increment
+ *	nlink of parent inode if @sd is a directory and link into the
+ *	children list of the parent.
+ *
+ *	This function should be called between calls to
+ *	sysfs_addrm_start() and sysfs_addrm_finish() and should be
+ *	passed the same @acxt as passed to sysfs_addrm_start().
+ *
+ *	LOCKING:
+ *	Determined by sysfs_addrm_start().
+ *
+ *	RETURNS:
+ *	0 on success, -EEXIST if entry with the given name already
+ *	exists.
+ */
+int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+{
+	int ret;
+
+	ret = __sysfs_add_one(acxt, sd);
+	if (ret == -EEXIST) {
+		printk(KERN_WARNING "sysfs: duplicate filename '%s' "
+		       "can not be created\n", sd->s_name);
+		WARN_ON(1);
+	}
+	return ret;
+}
+
+/**
  *	sysfs_remove_one - remove sysfs_dirent from parent
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be removed
fs/sysfs/symlink.c +33 -8
@@ -19,13 +19,8 @@
 
 #include "sysfs.h"
 
-/**
- *	sysfs_create_link - create symlink between two objects.
- *	@kobj:	object whose directory we're creating the link in.
- *	@target:	object we're pointing to.
- *	@name:		name of the symlink.
- */
-int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char * name)
+static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
+				const char *name, int warn)
 {
 	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;

@@ -65,7 +60,10 @@ int sysfs_create_link(struct kobject * k
 	target_sd = NULL;	/* reference is now owned by the symlink */
 
 	sysfs_addrm_start(&acxt, parent_sd);
-	error = sysfs_add_one(&acxt, sd);
+	if (warn)
+		error = sysfs_add_one(&acxt, sd);
+	else
+		error = __sysfs_add_one(&acxt, sd);
 	sysfs_addrm_finish(&acxt);
 
 	if (error)

@@ -80,6 +78,33 @@ int sysfs_create_link(struct kobject * k
 }
 
 /**
+ *	sysfs_create_link - create symlink between two objects.
+ *	@kobj:	object whose directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ */
+int sysfs_create_link(struct kobject *kobj, struct kobject *target,
+		      const char *name)
+{
+	return sysfs_do_create_link(kobj, target, name, 1);
+}
+
+/**
+ *	sysfs_create_link_nowarn - create symlink between two objects.
+ *	@kobj:	object whose directory we're creating the link in.
+ *	@target:	object we're pointing to.
+ *	@name:		name of the symlink.
+ *
+ *	This function does the same as sysf_create_link(), but it
+ *	doesn't warn if the link already exists.
+ */
+int sysfs_create_link_nowarn(struct kobject *kobj, struct kobject *target,
+			     const char *name)
+{
+	return sysfs_do_create_link(kobj, target, name, 0);
+}
+
+/**
  *	sysfs_remove_link - remove symlink in object's directory.
  *	@kobj:	object we're acting for.
  *	@name:	name of the symlink to remove.
fs/sysfs/sysfs.h +1 -0
@@ -107,6 +107,7 @@ struct sysfs_dirent *sysfs_get_active_tw
 void sysfs_put_active_two(struct sysfs_dirent *sd);
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
+int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
include/linux/sysfs.h +10 -0
@@ -101,6 +101,9 @@ void sysfs_remove_bin_file(struct kobjec
 
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
+int __must_check sysfs_create_link_nowarn(struct kobject *kobj,
+					  struct kobject *target,
+					  const char *name);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
 int __must_check sysfs_create_group(struct kobject *kobj,

@@ -180,6 +183,13 @@ static inline int sysfs_create_link(stru
 	return 0;
 }
 
+static inline int sysfs_create_link_nowarn(struct kobject *kobj,
+					   struct kobject *target,
+					   const char *name)
+{
+	return 0;
+}
+
 static inline void sysfs_remove_link(struct kobject *kobj, const char *name)
 {
 }
author
#19 lore raw

Re: [PATCH] driver core: Suppress sysfs warnings for device_rename().

Quoted 33 lines · On Tue, 10 Jun 2008 11:09:08 +0200 Cornelia Huck <[email protected]>…

On Wed, 21 May 2008 10:05:56 +0200,
Cornelia Huck <[email protected]> wrote:

Just digging through my backlog: Is there any further interest in this patch?

Quoted 27 lines
Quoted 10 lines · Greg KH
Quoted 2 lines · Cornelia Huck

OK, here is an actually-compiled patch with proper description and s-o-b. Comments?

Looks good to me, feel free to add an:

Acked-by Greg Kroah-Hartman <[email protected]>

to it.

David, are you going to take this through your tree?

Here it is again, respun against today's git:

driver core: Suppress sysfs warnings for device_rename().

Renaming network devices to an already existing name is not something we want sysfs to print a scary warning for, since the callers can deal with this correctly. So let's introduce sysfs_create_link_nowarn() which gets rid of the common warning.

Acked-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>

I you want to put the warnings back this is a reasonable way.

author
#20 lore raw

PATCH 3/3 bonding: handle case of device named bonding_master

In reply to a message not in the archive

If device already exists named bonding_masters, then fail. This is a wierd corner case only a QA group could love.

Signed-off-by Stephen Hemminger <[email protected]>

---
Note: bonding is not yet network namespace aware, it will need lots of work to get this to happen.

drivers/net/bonding/bond_sysfs.c +10 -2
@@ -1437,8 +1437,16 @@ int bond_create_sysfs(void)
 	 * configure multiple bonding devices.
 	 */
 	if (ret == -EEXIST) {
-		netdev_class = NULL;
-		return 0;
+		/* Is someone being kinky and naming a device bonding_master? */
+		if (__dev_get_by_name(&init_net,
+				      class_attr_bonding_masters.attr.name))
+			printk(KERN_ERR
+			       "network device named %s already exists in sysfs",
+			       class_attr_bonding_masters.attr.name);
+		else {
+			netdev_class = NULL;
+			return 0;
+		}
 	}
 
 	return ret;
maintainer
#21 lore raw

Re: [PATCH 3/3] bonding: handle case of device named bonding_master

From: Stephen Hemminger <[email protected]>
Date: Wed, 14 May 2008 18:15:28 -0700

Quoted 4 lines

If device already exists named bonding_masters, then fail. This is a wierd corner case only a QA group could love.

Signed-off-by Stephen Hemminger <[email protected]>

Also applied, thanks a lot Stephen.

author
#22 lore raw

PATCH 1/3 net: handle errors from device_rename

In reply to Stephen Hemminger

device_rename can fail with -EEXIST or -ENOMEM, so handle any problems.

Signed-off-by Stephen Hemminger <[email protected]>
net/core/dev.c +5 -1
@@ -903,7 +903,11 @@ int dev_change_name(struct net_device *d
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
 rollback:
-	device_rename(&dev->dev, dev->name);
+	err = device_rename(&dev->dev, dev->name);
+	if (err) {
+		memcpy(dev->name, oldname, IFNAMSIZ);
+		return err;
+	}
 
 	write_lock_bh(&dev_base_lock);
 	hlist_del(&dev->name_hlist);
#24 lore raw

Re: [PATCH 1/3] net: handle errors from device_rename

In reply to Stephen Hemminger

Stephen Hemminger said the following on 2008-5-15 9:15:

net/core/dev.c int dev_change_name(struct net_device *d · line 908

Quoted 5 lines

rollback:

net/core/dev.c +3 -1
-	device_rename(&dev->dev, dev->name);
+	err = device_rename(&dev->dev, dev->name);
+	if (err) {
+		memcpy(dev->name, oldname, IFNAMSIZ);

                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

no need, dev->name didn't change in device_rename().

Quoted 3 lines
net/core/dev.c +2 -0
+		return err;
+	}
author
#25 lore raw

Re: [PATCH 1/3] net: handle errors from device_rename

Quoted 7 lines · On Thu, 15 May 2008 16:41:40 +0800 Wang Chen <[email protected]> wrote:

Stephen Hemminger said the following on 2008-5-15 9:15:

net/core/dev.c int dev_change_name(struct net_device *d · line 908

Quoted 5 lines

rollback:

net/core/dev.c +3 -1
-	device_rename(&dev->dev, dev->name);
+	err = device_rename(&dev->dev, dev->name);
+	if (err) {
+		memcpy(dev->name, oldname, IFNAMSIZ);

                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 5 lines

no need, dev->name didn't change in device_rename().

net/core/dev.c int dev_change_name(struct net_device *d · line 910

Quoted 3 lines
net/core/dev.c +2 -0
+		return err;
+	}

dev->name has already been changed before the call to device_rename, so it needs to be set back read the whole code context.