[PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Alex Chiang]

Give a better clue about where we might be creating duplicate
files by displaying the parent's name as well.

Signed-off-by: Alex Chiang <achiang@hp.com>
---
It would be nice to get a full path, but simply printing out the
parent is cheap and more useful than what we have now.

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 82d3b79..5828d1d 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -459,7 +459,8 @@ int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 
 	ret = __sysfs_add_one(acxt, sd);
 	WARN(ret == -EEXIST, KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
+		       "can not be created in %s\n",
+		       sd->s_name, acxt->parent_sd->s_name);
 	return ret;
 }
 

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Greg KH]

On Wed, Feb 11, 2009 at 01:26:01PM -0700, Alex Chiang wrote:
> Give a better clue about where we might be creating duplicate
> files by displaying the parent's name as well.
> 
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
> It would be nice to get a full path, but simply printing out the
> parent is cheap and more useful than what we have now.

What happens if you don't have a parent?  will this oops if you are
creating a duplicate device in the root of the sysfs tree?

thanks,

greg k-h

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Alex Chiang]

* Greg KH <gregkh@suse.de>:
> On Wed, Feb 11, 2009 at 01:26:01PM -0700, Alex Chiang wrote:
> > Give a better clue about where we might be creating duplicate
> > files by displaying the parent's name as well.
> > 
> > Signed-off-by: Alex Chiang <achiang@hp.com>
> > ---
> > It would be nice to get a full path, but simply printing out the
> > parent is cheap and more useful than what we have now.
> 
> What happens if you don't have a parent?  will this oops if you are
> creating a duplicate device in the root of the sysfs tree?

We won't oops because if you attempt to create a device in the
root of the sysfs tree with a NULL parent, then we say that your
parent is sysfs_root:

int sysfs_create_dir(struct kobject * kobj)
{
	...
        if (kobj->parent)
		parent_sd = kobj->parent->sd;
	else
		parent_sd = &sysfs_root;
	...

But I do notice that we're not giving sysfs_root a name, so if
you do hit the WARN for a duplicate entry in sysfs_root, you get
a blank string for the location of the duplicate.

How about this instead?

From: Alex Chiang <achiang@hp.com>

sysfs: give sysfs_root a proper name; display parent in duplicate entry WARN

Name sysfs_root "/".

Make sysfs_add_one tell you _where_ you're attempting to create a
duplicate file to help debug.

Giving sysfs_root a proper name covers the case when trying to
create multiple entries with the same name in the root of the
sysfs tree.

Signed-off-by: Alex Chiang <achiang@hp.com>
---
Still would be nicer to get a full path.

I think that in the context of a sysfs WARN, claiming that you're
trying to create a duplicate file in "/" really means "/sys" and
shouldn't be confusing. Sample output:

sysfs: duplicate filename 'alex' can not be created in /

This maybe wants to be 2 patches, and I can split it up that way
if you prefer.

---

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 82d3b79..e153bd7 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -459,7 +459,8 @@ int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 
 	ret = __sysfs_add_one(acxt, sd);
 	WARN(ret == -EEXIST, KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
+		       "cannot be created in %s\n",
+		       sd->s_name, acxt->parent_sd->s_name);
 	return ret;
 }
 
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index ab343e3..7661b8d 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -33,7 +33,7 @@ static const struct super_operations sysfs_ops = {
 };
 
 struct sysfs_dirent sysfs_root = {
-	.s_name		= "",
+	.s_name		= "/",
 	.s_count	= ATOMIC_INIT(1),
 	.s_flags	= SYSFS_DIR,
 	.s_mode		= S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Greg KH]

On Wed, Feb 11, 2009 at 07:56:55PM -0700, Alex Chiang wrote:
> * Greg KH <gregkh@suse.de>:
> > On Wed, Feb 11, 2009 at 01:26:01PM -0700, Alex Chiang wrote:
> > > Give a better clue about where we might be creating duplicate
> > > files by displaying the parent's name as well.
> > > 
> > > Signed-off-by: Alex Chiang <achiang@hp.com>
> > > ---
> > > It would be nice to get a full path, but simply printing out the
> > > parent is cheap and more useful than what we have now.
> > 
> > What happens if you don't have a parent?  will this oops if you are
> > creating a duplicate device in the root of the sysfs tree?
> 
> We won't oops because if you attempt to create a device in the
> root of the sysfs tree with a NULL parent, then we say that your
> parent is sysfs_root:
> 
> int sysfs_create_dir(struct kobject * kobj)
> {
> 	...
>         if (kobj->parent)
> 		parent_sd = kobj->parent->sd;
> 	else
> 		parent_sd = &sysfs_root;
> 	...
> 
> But I do notice that we're not giving sysfs_root a name, so if
> you do hit the WARN for a duplicate entry in sysfs_root, you get
> a blank string for the location of the duplicate.
> 
> How about this instead?
> 
> From: Alex Chiang <achiang@hp.com>
> 
> sysfs: give sysfs_root a proper name; display parent in duplicate entry WARN
> 
> Name sysfs_root "/".
> 
> Make sysfs_add_one tell you _where_ you're attempting to create a
> duplicate file to help debug.
> 
> Giving sysfs_root a proper name covers the case when trying to
> create multiple entries with the same name in the root of the
> sysfs tree.
> 
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
> Still would be nicer to get a full path.

It would, and you _almost_ might be able to get it, but it might take
some work.  We do store the last sysfs file accessed, and can compute
the path of that as we have a struct file * to play with.  You could
walk the tree of devices backwards to reconstruct the full path to
emulate this if you want to.

> I think that in the context of a sysfs WARN, claiming that you're
> trying to create a duplicate file in "/" really means "/sys" and
> shouldn't be confusing. Sample output:

While you and I mount sysfs at /sys/ and it is the "expected" place for
it to be, we shouldn't just assume that is where it is in the
filesystem, especially as I don't where the container people are placing
sysfs.

> sysfs: duplicate filename 'alex' can not be created in /
> 
> This maybe wants to be 2 patches, and I can split it up that way
> if you prefer.

Sure, that would probably be best.

thanks,

greg k-h

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Alex Chiang]

* Greg KH <gregkh@suse.de>:
> On Wed, Feb 11, 2009 at 07:56:55PM -0700, Alex Chiang wrote:
> > * Greg KH <gregkh@suse.de>:
> > > On Wed, Feb 11, 2009 at 01:26:01PM -0700, Alex Chiang wrote:
> > > > Give a better clue about where we might be creating duplicate
> > > > files by displaying the parent's name as well.
> > > > 
> > > > Signed-off-by: Alex Chiang <achiang@hp.com>
> > > > ---
> > > > It would be nice to get a full path, but simply printing out the
> > > > parent is cheap and more useful than what we have now.
> > > 
> > > What happens if you don't have a parent?  will this oops if you are
> > > creating a duplicate device in the root of the sysfs tree?
> > 
> > We won't oops because if you attempt to create a device in the
> > root of the sysfs tree with a NULL parent, then we say that your
> > parent is sysfs_root:
> > 
> > int sysfs_create_dir(struct kobject * kobj)
> > {
> > 	...
> >         if (kobj->parent)
> > 		parent_sd = kobj->parent->sd;
> > 	else
> > 		parent_sd = &sysfs_root;
> > 	...
> > 
> > But I do notice that we're not giving sysfs_root a name, so if
> > you do hit the WARN for a duplicate entry in sysfs_root, you get
> > a blank string for the location of the duplicate.
> > 
> > How about this instead?
> > 
> > From: Alex Chiang <achiang@hp.com>
> > 
> > sysfs: give sysfs_root a proper name; display parent in duplicate entry WARN
> > 
> > Name sysfs_root "/".
> > 
> > Make sysfs_add_one tell you _where_ you're attempting to create a
> > duplicate file to help debug.
> > 
> > Giving sysfs_root a proper name covers the case when trying to
> > create multiple entries with the same name in the root of the
> > sysfs tree.
> > 
> > Signed-off-by: Alex Chiang <achiang@hp.com>
> > ---
> > Still would be nicer to get a full path.
> 
> It would, and you _almost_ might be able to get it, but it might take
> some work.  We do store the last sysfs file accessed, and can compute
> the path of that as we have a struct file * to play with.  You could
> walk the tree of devices backwards to reconstruct the full path to
> emulate this if you want to.
> 
> > I think that in the context of a sysfs WARN, claiming that you're
> > trying to create a duplicate file in "/" really means "/sys" and
> > shouldn't be confusing. Sample output:
> 
> While you and I mount sysfs at /sys/ and it is the "expected" place for
> it to be, we shouldn't just assume that is where it is in the
> filesystem, especially as I don't where the container people are placing
> sysfs.
> 
> > sysfs: duplicate filename 'alex' can not be created in /
> > 
> > This maybe wants to be 2 patches, and I can split it up that way
> > if you prefer.
> 
> Sure, that would probably be best.

Here's v3.

Seems like 1 patch makes sense because the logical change is that
sysfs_add_one() now displays the full sysfs path to a duplicate
entry. The fact that it relies on a helper function is an
implementation detail. ;)

I tested it by hacking up a kernel to create duplicate entries in
both sysfs_root, as well as creating duplicate entries under a
random sysfs directory.

Examples:

-----------[ cut here ]------------
WARNING: at fs/sysfs/dir.c:488 sysfs_add_one+0xe0/0x100()
sysfs: cannot create duplicate filename '/alex'

------------[ cut here ]------------
WARNING: at fs/sysfs/dir.c:488 sysfs_add_one+0xe0/0x100()
Hardware name: server rx6600                   
sysfs: cannot create duplicate filename '/bus/pci/slots/5/alex'

From: Alex Chiang <achiang@hp.com>

sysfs: sysfs_add_one WARNs with full path to duplicate filename

As a debugging aid, it can be useful to know the full path to a
duplicate file being created in sysfs.

We now will display warnings such as:

	sysfs: cannot create duplicate filename '/foo'

when attempting to create multiple files named 'foo' in the sysfs
root, or:

	sysfs: cannot create duplicate filename '/bus/pci/slots/5/foo'

when attempting to create multiple files named 'foo' under a
given directory in sysfs.

The path displayed is always a relative path to sysfs_root. The
leading '/' in the path name refers to the sysfs_root mount
point, and should not be confused with the "real" '/'.

Thanks to Alex Williamson for essentially writing sysfs_pathname.

Cc: Alex Williamson <alex.williamson@hp.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---
The strcat(strcat()) is a little ugly, but concise and accurate.

Without the memset, I get all sorts of stack garbage when the
path is finally printed out.

 dir.c |   32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 82d3b79..73d34a0 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -434,6 +434,27 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 }
 
 /**
+ *	sysfs_pathname - return full path to sysfs dirent
+ *	@sd: sysfs_dirent whose path we want
+ *	@path: caller allocated buffer
+ *
+ *	Gives the name "/" to the sysfs_root entry; any path returned
+ *	is relative to wherever sysfs is mounted.
+ *
+ *	XXX: does no error checking on @path; assumes caller did something
+ *	     sane, like char path[PATH_MAX]
+ */
+static char *sysfs_pathname(struct sysfs_dirent *sd, char *path)
+{
+	if (sd->s_parent) {
+		sysfs_pathname(sd->s_parent, path);
+		strcat(path, "/");
+	}
+	strcat(path, sd->s_name);
+	return path;
+}
+
+/**
  *	sysfs_add_one - add sysfs_dirent to parent
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be added
@@ -458,8 +479,15 @@ int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 	int ret;
 
 	ret = __sysfs_add_one(acxt, sd);
-	WARN(ret == -EEXIST, KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
+	if (ret == -EEXIST) {
+		char path[PATH_MAX];
+		memset(path, 0, PATH_MAX);
+		WARN(1, KERN_WARNING
+		     "sysfs: cannot create duplicate filename '%s'\n",
+		     strcat(strcat(sysfs_pathname(acxt->parent_sd, path), "/"),
+			    sd->s_name));
+	}
+
 	return ret;
 }
 

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Kay Sievers]

On Thu, Feb 12, 2009 at 08:02, Alex Chiang <achiang@hp.com> wrote:

> sysfs: sysfs_add_one WARNs with full path to duplicate filename
>
> As a debugging aid, it can be useful to know the full path to a
> duplicate file being created in sysfs.

>        ret = __sysfs_add_one(acxt, sd);
> -       WARN(ret == -EEXIST, KERN_WARNING "sysfs: duplicate filename '%s' "
> -                      "can not be created\n", sd->s_name);
> +       if (ret == -EEXIST) {
> +               char path[PATH_MAX];

Isn't that 4k on the stack, and a bit large to put there?

Thanks,
Kay

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Greg KH]

On Thu, Feb 12, 2009 at 10:05:23AM +0100, Kay Sievers wrote:
> On Thu, Feb 12, 2009 at 08:02, Alex Chiang <achiang@hp.com> wrote:
> 
> > sysfs: sysfs_add_one WARNs with full path to duplicate filename
> >
> > As a debugging aid, it can be useful to know the full path to a
> > duplicate file being created in sysfs.
> 
> >        ret = __sysfs_add_one(acxt, sd);
> > -       WARN(ret == -EEXIST, KERN_WARNING "sysfs: duplicate filename '%s' "
> > -                      "can not be created\n", sd->s_name);
> > +       if (ret == -EEXIST) {
> > +               char path[PATH_MAX];
> 
> Isn't that 4k on the stack, and a bit large to put there?

I agree, that's not ok.  Alex, care to redo this?

thanks,

greg k-h

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Alex Chiang]

* Greg KH <gregkh@suse.de>:
> On Thu, Feb 12, 2009 at 10:05:23AM +0100, Kay Sievers wrote:
> > On Thu, Feb 12, 2009 at 08:02, Alex Chiang <achiang@hp.com> wrote:
> > 
> > > sysfs: sysfs_add_one WARNs with full path to duplicate filename
> > >
> > > As a debugging aid, it can be useful to know the full path to a
> > > duplicate file being created in sysfs.
> > 
> > >        ret = __sysfs_add_one(acxt, sd);
> > > -       WARN(ret == -EEXIST, KERN_WARNING "sysfs: duplicate filename '%s' "
> > > -                      "can not be created\n", sd->s_name);
> > > +       if (ret == -EEXIST) {
> > > +               char path[PATH_MAX];
> > 
> > Isn't that 4k on the stack, and a bit large to put there?
> 
> I agree, that's not ok.  Alex, care to redo this?

Here's v4. Adding Arjan to cc because I realized this may cause
problems for kerneloops.org.

If we're displaying a full path now to something like:

	/sys/bus/pci/devices/.../foo

Then the "..." will highly likely be different on every machine
(even though the programming error is still the same).

Is this patch going to cause major heartburn?

Maybe it's more trouble than it's worth... :-/

Thanks.

/ac

From: Alex Chiang <achiang@hp.com>

sysfs: sysfs_add_one WARNs with full path to duplicate filename

As a debugging aid, it can be useful to know the full path to a
duplicate file being created in sysfs.

We now will display warnings such as:

	sysfs: cannot create duplicate filename '/foo'

when attempting to create multiple files named 'foo' in the sysfs
root, or:

	sysfs: cannot create duplicate filename '/bus/pci/slots/5/foo'

when attempting to create multiple files named 'foo' under a
given directory in sysfs.

The path displayed is always a relative path to sysfs_root. The
leading '/' in the path name refers to the sysfs_root mount
point, and should not be confused with the "real" '/'.

Thanks to Alex Williamson for essentially writing sysfs_pathname.

Cc: Alex Williamson <alex.williamson@hp.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---
 dir.c |   31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)
---
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 82d3b79..ed33361 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -434,6 +434,26 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 }
 
 /**
+ *	sysfs_pathname - return full path to sysfs dirent
+ *	@sd: sysfs_dirent whose path we want
+ *	@path: caller allocated buffer
+ *
+ *	Gives the name "/" to the sysfs_root entry; any path returned
+ *	is relative to wherever sysfs is mounted.
+ *
+ *	XXX: does no error checking on @path size
+ */
+static char *sysfs_pathname(struct sysfs_dirent *sd, char *path)
+{
+	if (sd->s_parent) {
+		sysfs_pathname(sd->s_parent, path);
+		strcat(path, "/");
+	}
+	strcat(path, sd->s_name);
+	return path;
+}
+
+/**
  *	sysfs_add_one - add sysfs_dirent to parent
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be added
@@ -458,8 +478,15 @@ int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 	int ret;
 
 	ret = __sysfs_add_one(acxt, sd);
-	WARN(ret == -EEXIST, KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
+	if (ret == -EEXIST) {
+		char *path = kzalloc(PATH_MAX, GFP_KERNEL);
+		WARN(1, KERN_WARNING
+		     "sysfs: cannot create duplicate filename '%s'\n",
+		     strcat(strcat(sysfs_pathname(acxt->parent_sd, path), "/"),
+			    sd->s_name));
+		kfree(path);
+	}
+
 	return ret;
 }
 

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Greg KH]

On Thu, Feb 12, 2009 at 10:38:35AM -0700, Alex Chiang wrote:
> +	if (ret == -EEXIST) {
> +		char *path = kzalloc(PATH_MAX, GFP_KERNEL);

You forgot to check the return value of kzalloc().

Another try?

8th time's a charm?  :)

thanks,

greg k-h

Re: [PATCH] sysfs: sysfs_add_one tells you _where_ the duplicate file is

[Alex Chiang]

* Greg KH <gregkh@suse.de>:
> On Thu, Feb 12, 2009 at 10:38:35AM -0700, Alex Chiang wrote:
> > +	if (ret == -EEXIST) {
> > +		char *path = kzalloc(PATH_MAX, GFP_KERNEL);
> 
> You forgot to check the return value of kzalloc().
> 
> Another try?
> 
> 8th time's a charm?  :)

Yeah, you're too fast for me. I thought about that just as I
walked away from my desk. ;)

What are we on, v6?

From: Alex Chiang <achiang@hp.com>

sysfs: sysfs_add_one WARNs with full path to duplicate filename

As a debugging aid, it can be useful to know the full path to a
duplicate file being created in sysfs.

We now will display warnings such as:

	sysfs: cannot create duplicate filename '/foo'

when attempting to create multiple files named 'foo' in the sysfs
root, or:

	sysfs: cannot create duplicate filename '/bus/pci/slots/5/foo'

when attempting to create multiple files named 'foo' under a
given directory in sysfs.

The path displayed is always a relative path to sysfs_root. The
leading '/' in the path name refers to the sysfs_root mount
point, and should not be confused with the "real" '/'.

Thanks to Alex Williamson for essentially writing sysfs_pathname.

Cc: Alex Williamson <alex.williamson@hp.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---
 dir.c |   32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)
---
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 82d3b79..f13d852 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -434,6 +434,26 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 }
 
 /**
+ *	sysfs_pathname - return full path to sysfs dirent
+ *	@sd: sysfs_dirent whose path we want
+ *	@path: caller allocated buffer
+ *
+ *	Gives the name "/" to the sysfs_root entry; any path returned
+ *	is relative to wherever sysfs is mounted.
+ *
+ *	XXX: does no error checking on @path size
+ */
+static char *sysfs_pathname(struct sysfs_dirent *sd, char *path)
+{
+	if (sd->s_parent) {
+		sysfs_pathname(sd->s_parent, path);
+		strcat(path, "/");
+	}
+	strcat(path, sd->s_name);
+	return path;
+}
+
+/**
  *	sysfs_add_one - add sysfs_dirent to parent
  *	@acxt: addrm context to use
  *	@sd: sysfs_dirent to be added
@@ -458,8 +478,16 @@ int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 	int ret;
 
 	ret = __sysfs_add_one(acxt, sd);
-	WARN(ret == -EEXIST, KERN_WARNING "sysfs: duplicate filename '%s' "
-		       "can not be created\n", sd->s_name);
+	if (ret == -EEXIST) {
+		char *path = kzalloc(PATH_MAX, GFP_KERNEL);
+		WARN(1, KERN_WARNING
+		     "sysfs: cannot create duplicate filename '%s'\n",
+		     (path == NULL) ? sd->s_name :
+		     strcat(strcat(sysfs_pathname(acxt->parent_sd, path), "/"),
+		            sd->s_name));
+		kfree(path);
+	}
+
 	return ret;
 }