[Cbe-oss-dev] [patch 10/13] powerpc: sysfs fix compiler warning

[Greg KH]

On Thu, Jul 19, 2007 at 10:02:29AM +1000, Michael Ellerman wrote:
> On Thu, 2007-07-19 at 00:32 +0200, Arnd Bergmann wrote:
> > On Wednesday 18 July 2007, Andrew Morton wrote:
> > > >       for_each_possible_cpu(cpu) {
> > > >               sysdev = get_cpu_sysdev(cpu);
> > > > -             sysfs_create_group(&sysdev->kobj, attrs);
> > > > +             error = sysfs_create_group(&sysdev->kobj, attrs);
> > > > +
> > > > +             if (error) {
> > > > +                     for_each_possible_cpu(cpu) {
> > > > +                             sysdev = get_cpu_sysdev(cpu);
> > > > +                             sysfs_remove_group(&sysdev->kobj, attrs);
> > > > +                     }
> > > > +                     break;
> > > > +             }
> > > >       }
> > > >  
> > > >       mutex_unlock(&cpu_mutex);
> > > > -     return 0;
> > > > +     return error;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(cpu_add_sysdev_attr_group);
> > > 
> > > This will attempt to remove groups which were never added.  I don't know
> > > what will happen then.
> 
> It might BUG if the directory lookup fails, otherwise it will happily do
> nothing AFAICT.
> 
> > I looked at the code and it doesn't look too good. Let's drop this patch for
> > now. Christian, please submit a new patch when you've found a solution.
> 
> 
> Given that we __must_check the create, it'd be nice if the sysfs API
> gave us a bit of help, how about:
> 
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 52eed2a..cba7a2c 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -65,15 +65,16 @@ int sysfs_create_group(struct kobject * kobj,
>  	return error;
>  }
>  
> -void sysfs_remove_group(struct kobject * kobj, 
> -			const struct attribute_group * grp)
> +static int __sysfs_remove_group(struct kobject * kobj, 
> +			 const struct attribute_group * grp)
>  {
>  	struct dentry * dir;
>  
>  	if (grp->name) {
>  		dir = lookup_one_len_kern(grp->name, kobj->dentry,
>  				strlen(grp->name));
> -		BUG_ON(IS_ERR(dir));
> +		if (IS_ERR(dir))
> +			return PTR_ERR(dir);
>  	}
>  	else
>  		dir = dget(kobj->dentry);
> @@ -83,8 +84,22 @@ void sysfs_remove_group(struct kobject * kobj,
>  		sysfs_remove_subdir(dir);
>  	/* release the ref. taken in this routine */
>  	dput(dir);
> +
> +	return 0;
> +}
> +
> +void sysfs_remove_group(struct kobject * kobj, 
> +			const struct attribute_group * grp)
> +{
> +	BUG_ON(__sysfs_remove_group(kobj, grp));
>  }
> +EXPORT_SYMBOL_GPL(sysfs_remove_group);
>  
> +void sysfs_remove_group_please(struct kobject * kobj, 
> +			const struct attribute_group * grp)
> +{

Heh, this is a joke, right?

Funny, you have to ask nicely in order for the code to do what you
want...

:)

I don't see the problem with the original code that you are creating
this patch for.  Can you explain please?

thanks,

greg k-h