[PATCH]sysfs: Don't emit a warning when sysfs_rename_link() fails.

[PATCH]sysfs: Don't emit a warning when sysfs_rename_link() fails.

[Cornelia Huck]

Hi Greg, hi Eric,

the recent sysfs tagged directory changes switched device_rename() to
sysfs_rename_link() - which is a good thing but AFAICS re-introduces
the scary warnings when a netdevice is renamed to something that
already exists (which I tried to fix with
36ce6dad6e3cb3f050ed41e0beac0070d2062b25).

The following patch switches sysfs_rename_link() to non-warning symlink
creation again. It is on top of the current driver core series.

sysfs: Don't emit a warning when sysfs_rename_link() fails.

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

sysfs_rename_link() will be printing a scary looking warning
if a link of the same name already exists. Callers of
device_rename() don't want to see this warning since they
already handle the failure themselves, so let's use the not
warning variant of sysfs_do_create_link().

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 fs/sysfs/symlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/fs/sysfs/symlink.c
===================================================================
--- linux-2.6.orig/fs/sysfs/symlink.c
+++ linux-2.6/fs/sysfs/symlink.c
@@ -135,7 +135,7 @@ int sysfs_rename_link(struct kobject *ko
 			const char *old, const char *new)
 {
 	sysfs_delete_link(kobj, targ, old);
-	return sysfs_create_link(kobj, targ, new);
+	return sysfs_do_create_link(kobj, targ, new, 0);
 }
 
 static int sysfs_get_target_path(struct sysfs_dirent *parent_sd,

Re: [PATCH]sysfs: Don't emit a warning when sysfs_rename_link() fails.

[Eric W. Biederman]

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

> Hi Greg, hi Eric,
>
> the recent sysfs tagged directory changes switched device_rename() to
> sysfs_rename_link() - which is a good thing but AFAICS re-introduces
> the scary warnings when a netdevice is renamed to something that
> already exists (which I tried to fix with
> 36ce6dad6e3cb3f050ed41e0beac0070d2062b25).

A netdevice can not be renamed to something that already exists, correctly
and still emit warnings.  Either it is a noop rename in which case
the fact that we delete the link before creating it will avoid warnings.
Or we are actually using a conflicting name.  In which case it is a
real and valid problem.  The netdev layer especially since the
networking layer already has validated that the rename is valid
before calling device_rename.

> The following patch switches sysfs_rename_link() to non-warning symlink
> creation again. It is on top of the current driver core series.

We don't need this.  Using the non-warning symlink creation is unnecessary.
Using non-warning symlink creation hides real errors.

In practice any errors that show up will be errors in sysfs, because
the network subsystem validates everything before calling us.

Eric

Re: [PATCH]sysfs: Don't emit a warning when sysfs_rename_link() fails.

[Cornelia Huck]

On Wed, 17 Sep 2008 11:55:14 -0700,
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> 
> > Hi Greg, hi Eric,
> >
> > the recent sysfs tagged directory changes switched device_rename() to
> > sysfs_rename_link() - which is a good thing but AFAICS re-introduces
> > the scary warnings when a netdevice is renamed to something that
> > already exists (which I tried to fix with
> > 36ce6dad6e3cb3f050ed41e0beac0070d2062b25).
> 
> A netdevice can not be renamed to something that already exists, correctly
> and still emit warnings.  Either it is a noop rename in which case
> the fact that we delete the link before creating it will avoid warnings.

Sure, that case is fine.

> Or we are actually using a conflicting name.  In which case it is a
> real and valid problem. 

Which may be in userspace as well. In the past the networking folks
were unhappy about the sysfs warning, claiming that failures should
rather be handled in their layer.

> The netdev layer especially since the
> networking layer already has validated that the rename is valid
> before calling device_rename.

Does it? Or do I just don't get it because of lack of coffee?

> 
> > The following patch switches sysfs_rename_link() to non-warning symlink
> > creation again. It is on top of the current driver core series.
> 
> We don't need this.  Using the non-warning symlink creation is unnecessary.
> Using non-warning symlink creation hides real errors.

For most cases, yes.

> 
> In practice any errors that show up will be errors in sysfs, because
> the network subsystem validates everything before calling us.

I was under the impression that no checks are done if the rename is
triggered via ioctl. And it makes more sense to have the caller of the
ioctl get an error than to spit a sysfs warning.

Re: [PATCH]sysfs: Don't emit a warning when sysfs_rename_link() fails.

[Eric W. Biederman]

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

> On Wed, 17 Sep 2008 11:55:14 -0700,
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>> Cornelia Huck <cornelia.huck@de.ibm.com> writes:


>> Or we are actually using a conflicting name.  In which case it is a
>> real and valid problem. 
>
> Which may be in userspace as well. In the past the networking folks
> were unhappy about the sysfs warning, claiming that failures should
> rather be handled in their layer.

Because the failures have always been handled in the networking
stack.

The only case that was ever warned about by sysfs were noop
renames.

So no we will not catch any userspace bugs with this code.

>> The netdev layer especially since the
>> networking layer already has validated that the rename is valid
>> before calling device_rename.
>
> Does it? Or do I just don't get it because of lack of coffee?

In the dev_change_name we either do dev_alloc_name
which generates a unique device name or __dev_get_by_name
which looks to see if we are already using that name.

The code in cfg80211_dev_rename is a trivial probe so I
don't think that is subtle.

>> > The following patch switches sysfs_rename_link() to non-warning symlink
>> > creation again. It is on top of the current driver core series.
>> 
>> We don't need this.  Using the non-warning symlink creation is unnecessary.
>> Using non-warning symlink creation hides real errors.
>
> For most cases, yes.

In these cases especially.

>> In practice any errors that show up will be errors in sysfs, because
>> the network subsystem validates everything before calling us.
>
> I was under the impression that no checks are done if the rename is
> triggered via ioctl. And it makes more sense to have the caller of the
> ioctl get an error than to spit a sysfs warning.

Currently there are exactly 2 callers of device_rename.
net/core/dev.c:dev_change_name 
net/wireless/core.c:cfg80211_dev_rename

Both of which verify that the rename is valid before
they call device_rename.  In fact that they have to because
the kobject layer does not have enough information to verify
that the rename is valid, and sysfs is not necessarily compiled in.

Eric

Re: [PATCH]sysfs: Don't emit a warning when sysfs_rename_link() fails.

[Cornelia Huck]

On Thu, 18 Sep 2008 03:00:31 -0700,
ebiederm@xmission.com (Eric W. Biederman) wrote:

> In the dev_change_name we either do dev_alloc_name
> which generates a unique device name or __dev_get_by_name
> which looks to see if we are already using that name.

Duh, I overlooked that, sorry about the noise.

(OTOH, we should get of sysfs_do_create_link() then, since it's an
unneed indirection).

Re: [PATCH]sysfs: Don't emit a warning when sysfs_rename_link() fails.

[Eric W. Biederman]

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

> On Thu, 18 Sep 2008 03:00:31 -0700,
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>> In the dev_change_name we either do dev_alloc_name
>> which generates a unique device name or __dev_get_by_name
>> which looks to see if we are already using that name.
>
> Duh, I overlooked that, sorry about the noise.

No problem.

> (OTOH, we should get of sysfs_do_create_link() then, since it's an
> unneed indirection).

No objection from me.

Eric

[PATCH] sysfs: Remove sysfs_do_create_link().

[Cornelia Huck]

Since sysfs_create_link_nowarn() is gone, sysfs_do_create_link()
is an unneeded indirection: Remove it.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 fs/sysfs/symlink.c |   27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

--- linux-2.6.orig/fs/sysfs/symlink.c
+++ linux-2.6/fs/sysfs/symlink.c
@@ -19,8 +19,14 @@
 
 #include "sysfs.h"
 
-static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
-				const char *name, int warn)
+/**
+ *	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)
 {
 	struct sysfs_dirent *parent_sd = NULL;
 	struct sysfs_dirent *target_sd = NULL;
@@ -60,10 +66,7 @@ static int sysfs_do_create_link(struct k
 	target_sd = NULL;	/* reference is now owned by the symlink */
 
 	sysfs_addrm_start(&acxt, parent_sd);
-	if (warn)
-		error = sysfs_add_one(&acxt, sd);
-	else
-		error = __sysfs_add_one(&acxt, sd);
+	error = sysfs_add_one(&acxt, sd);
 	sysfs_addrm_finish(&acxt);
 
 	if (error)
@@ -78,18 +81,6 @@ static int sysfs_do_create_link(struct 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_delete_link - remove symlink in object's directory.
  *	@kobj:	object we're acting for.
  *	@targ:	object we're pointing to.

Re: [PATCH] sysfs: Remove sysfs_do_create_link().

[Greg KH]

On Thu, Sep 18, 2008 at 02:28:06PM +0200, Cornelia Huck wrote:
> Since sysfs_create_link_nowarn() is gone, sysfs_do_create_link()
> is an unneeded indirection: Remove it.

Is this still needed, now that I've dropped Eric's patchset?

thanks,

greg k-h

Re: [PATCH] sysfs: Remove sysfs_do_create_link().

[Cornelia Huck]

On Mon, 6 Oct 2008 21:19:33 -0700,
Greg KH <greg@kroah.com> wrote:

> On Thu, Sep 18, 2008 at 02:28:06PM +0200, Cornelia Huck wrote:
> > Since sysfs_create_link_nowarn() is gone, sysfs_do_create_link()
> > is an unneeded indirection: Remove it.
> 
> Is this still needed, now that I've dropped Eric's patchset?

No, this was just a cleanup on top.