Re: [PATCH 5/6] sysfs: add support for binary attributes in groups

[Guenter Roeck <linux@roeck-us.net>]

On Wed, Jul 10, 2013 at 01:05:13PM -0700, Greg Kroah-Hartman wrote:
> groups should be able to support binary attributes, just like it
> supports "normal" attributes.  This lets us only handle one type of
> structure, groups, throughout the driver core and subsystems, making
> binary attributes a "full fledged" part of the driver model, and not
> something just "tacked on".
> 
> Reported-by: Oliver Schinagl <oliver+list@schinagl.nl>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/sysfs/group.c      | 18 ++++++++++++++++--
>  include/linux/sysfs.h |  4 ++--
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index aec3d5c..d8d8a8f 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -20,16 +20,19 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
>  			 const struct attribute_group *grp)
>  {
>  	struct attribute *const* attr;
> -	int i;
> +	struct bin_attribute *const* bin_attr;

checkpatch doesn't like the above and similar declarations below.

Yes, I know, nitpick, just saying ;).

>  
> -	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> +	for (attr = grp->attrs; *attr; attr++)
>  		sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
> +	for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
> +		sysfs_remove_bin_file(kobj, *bin_attr);
>  }
>  
>  static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
>  			const struct attribute_group *grp, int update)
>  {
>  	struct attribute *const* attr;
> +	struct bin_attribute *const* bin_attr;
>  	int error = 0, i;
>  
>  	for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> @@ -52,6 +55,17 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
>  	}
>  	if (error)
>  		remove_files(dir_sd, kobj, grp);

You might want to abort here if there was an error.

> +
> +	for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {

What happens here if grp->bin_attrs is NULL ? Doesn't that cause a NULL
reference when accessing *bin_attr ?

[ The problem didn't exist before since attr was supposed to be non-NULL.
  It may now be a problem if only bin_attr is provided, ie if there are
  only binary attribute files.
  Or maybe I am just tired and there is no problem... ]

> +		if (update)
> +			sysfs_remove_bin_file(kobj, *bin_attr);
> +		error = sysfs_create_bin_file(kobj, *bin_attr);
> +		if (error)
> +			break;
> +	}
> +	if (error)
> +		remove_files(dir_sd, kobj, grp);
> +
>  	return error;
>  }
>  
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index d50a96b..2c3b6a3 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -21,6 +21,7 @@
>  
>  struct kobject;
>  struct module;
> +struct bin_attribute;
>  enum kobj_ns_type;
>  
>  struct attribute {
> @@ -59,10 +60,9 @@ struct attribute_group {
>  	umode_t			(*is_visible)(struct kobject *,
>  					      struct attribute *, int);
>  	struct attribute	**attrs;
> +	struct bin_attribute	**bin_attrs;
>  };
>  
> -
> -
>  /**
>   * Use these macros to make defining attributes easier. See include/linux/device.h
>   * for examples..
> -- 
> 1.8.3.rc0.20.gb99dd2e
> 
>