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 <shemminger@vyatta.com>

--- a/drivers/base/core.c	2008-05-14 17:56:36.000000000 -0700
+++ b/drivers/base/core.c	2008-05-14 17:56:40.000000000 -0700
@@ -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
 
--- a/fs/sysfs/dir.c	2008-05-14 17:56:36.000000000 -0700
+++ b/fs/sysfs/dir.c	2008-05-14 17:56:40.000000000 -0700
@@ -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);
 
--

[ message continues ]

From: Stephen Hemminger <shemminger@vyatta.com>

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.
--

[ message continues ]

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 <gregkh@suse.de>
to them in that case.

thanks,

greg k-h
--

[ message continues ]

From: Greg KH <greg@kroah.com>

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

[ message continues ]

From: Stephen Hemminger <shemminger@vyatta.com>

Applied.
--

[ message continues ]

On Wed, 14 May 2008 18:16:03 -0700,


...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.
--

[ message continues ]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

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.

--

[ message continues ]

On Thu, 15 May 2008 01:01:39 -0700 (PDT),

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.

diff --git a/drivers/base/core.c b/drivers/base/core.c
index be288b5..a9058f4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -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);
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index a1c3a1f..b7914a0 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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 ...
[ message continues ]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

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.

--

[ message continues ]

On Thu, 15 May 2008 03:00:54 -0700 (PDT),

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

[ message continues ]

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 <cornelia.huck@de.ibm.com>

---
 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(-)

Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ 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
 
Index: ...
[ message continues ]

On Tue, 20 May 2008 12:59:13 +0200

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

[ message continues ]

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
--

[ message continues ]

Looks good to me, feel free to add an:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

to it.

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

thanks,

greg k-h
--

[ message continues ]

On Tue, 20 May 2008 15:52:44 -0700,

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 <gregkh@suse.de>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 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(-)

--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -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);
--- linux-2.6.orig/fs/sysfs/dir.c
+++ 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
 ...
[ message continues ]

On Tue, 10 Jun 2008 11:09:08 +0200

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

[ message continues ]