Re: [PATCH net-next v3 3/9] sysfs: add sysfs_group{s}_change_owner()

[Christian Brauner]

On Thu, Feb 20, 2020 at 12:15:50PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2020 at 05:29:37PM +0100, Christian Brauner wrote:
> > Add helpers to change the owner of sysfs groups.
> > This function will be used to correctly account for kobject ownership
> > changes, e.g. when moving network devices between network namespaces.
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > ---
> > /* v2 */
> > -  Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>:
> >    - Add comment how ownership of sysfs object is changed.
> > 
> > /* v3 */
> > -  Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>:
> >    - Add explicit uid/gid parameters.
> > - Christian Brauner <christian.brauner@xxxxxxxxxx>:
> >   - Collapse groups ownership helper patches into a single patch.
> > ---
> >  fs/sysfs/group.c      | 117 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h |  20 ++++++++
> >  2 files changed, 137 insertions(+)
> > 
> > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > index c4ab045926b7..bae562d3cba1 100644
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/dcache.h>
> >  #include <linux/namei.h>
> >  #include <linux/err.h>
> > +#include <linux/fs.h>
> >  #include "sysfs.h"
> >  
> >  
> > @@ -457,3 +458,119 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
> >  	return PTR_ERR_OR_ZERO(link);
> >  }
> >  EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
> > +
> > +static int sysfs_group_attrs_change_owner(struct kernfs_node *grp_kn,
> > +					  const struct attribute_group *grp,
> > +					  struct iattr *newattrs)
> > +{
> > +	struct kernfs_node *kn;
> > +	int error;
> > +
> > +	if (grp->attrs) {
> > +		struct attribute *const *attr;
> > +
> > +		for (attr = grp->attrs; *attr; attr++) {
> > +			kn = kernfs_find_and_get(grp_kn, (*attr)->name);
> > +			if (!kn)
> > +				return -ENOENT;
> > +
> > +			error = kernfs_setattr(kn, newattrs);
> > +			kernfs_put(kn);
> > +			if (error)
> > +				return error;
> > +		}
> > +	}
> > +
> > +	if (grp->bin_attrs) {
> > +		struct bin_attribute *const *bin_attr;
> > +
> > +		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> > +			kn = kernfs_find_and_get(grp_kn, (*bin_attr)->attr.name);
> > +			if (!kn)
> > +				return -ENOENT;
> > +
> > +			error = kernfs_setattr(kn, newattrs);
> > +			kernfs_put(kn);
> > +			if (error)
> > +				return error;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * sysfs_group_change_owner - change owner of an attribute group.
> > + * @kobj:	The kobject containing the group.
> > + * @grp:	The attribute group.
> > + * @kuid:	new owner's kuid
> > + * @kgid:	new owner's kgid
> > + *
> > + * Returns 0 on success or error code on failure.
> 
> This is fine to document, just funny it's the only one documented about
> the return value so far in this series.

I stuck to the documentation style common to the file. Most of the
functions in fs/syfs/file.c did not mention return codes
sysfs_remove_bin_file(), sysfs_create_bin_file(),
sysfs_remove_file_from_group() etc. But I'll document all in this series
with return codes now.

Thanks!
Christian