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

[Greg KH]

On Wed, Feb 11, 2009 at 07:56:55PM -0700, Alex Chiang wrote:
> * Greg KH <gregkh@xxxxxxx>:
> > 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@xxxxxx>
> > > ---
> > > 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@xxxxxx>
> 
> 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@xxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/