diff mbox

[v2,1/3] sysfs: Support is_visible() on binary attributes

Message ID 1442234049-18637-2-git-send-email-emilio.lopez@collabora.co.uk
State New
Headers show

Commit Message

Emilio López Sept. 14, 2015, 12:34 p.m. UTC
According to the sysfs header file:

    "The returned value will replace static permissions defined in
     struct attribute or struct bin_attribute."

but this isn't the case, as is_visible is only called on struct attribute
only. This patch introduces a new is_bin_visible() function to implement
the same functionality for binary attributes, and updates documentation
accordingly.

Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---

Changes from v1:
 - Don't overload is_visible, introduce is_bin_visible instead as
   discussed on the list.

 fs/sysfs/group.c      | 17 +++++++++++++++--
 include/linux/sysfs.h | 18 ++++++++++++++----
 2 files changed, 29 insertions(+), 6 deletions(-)

Comments

Guenter Roeck Sept. 14, 2015, 3:33 p.m. UTC | #1
On 09/14/2015 05:34 AM, Emilio López wrote:
> According to the sysfs header file:
>
>      "The returned value will replace static permissions defined in
>       struct attribute or struct bin_attribute."
>
> but this isn't the case, as is_visible is only called on struct attribute
> only. This patch introduces a new is_bin_visible() function to implement
> the same functionality for binary attributes, and updates documentation
> accordingly.
>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>

Nitpick below, but otherwise looks ok to me.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>
> Changes from v1:
>   - Don't overload is_visible, introduce is_bin_visible instead as
>     discussed on the list.
>
>   fs/sysfs/group.c      | 17 +++++++++++++++--
>   include/linux/sysfs.h | 18 ++++++++++++++----
>   2 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..51b56e6 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -73,13 +73,26 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>   	}
>
>   	if (grp->bin_attrs) {
> -		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> +		for (i = 0, bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> +			umode_t mode = (*bin_attr)->attr.mode;
> +
>   			if (update)
>   				kernfs_remove_by_name(parent,
>   						(*bin_attr)->attr.name);
> +			if (grp->is_bin_visible) {
> +				mode = grp->is_bin_visible(kobj, *bin_attr, i);
> +				if (!mode)
> +					continue;
> +			}
> +
> +			WARN(mode & ~(SYSFS_PREALLOC | 0664),
> +			     "Attribute %s: Invalid permissions 0%o\n",
> +			     (*bin_attr)->attr.name, mode);
> +
> +			mode &= SYSFS_PREALLOC | 0664;

Strictly speaking, the mode validation for binary attributes is new and logically
separate. Should it be mentioned in the commit log, or even be a separate patch ?

>   			error = sysfs_add_file_mode_ns(parent,
>   					&(*bin_attr)->attr, true,
> -					(*bin_attr)->attr.mode, NULL);
> +					mode, NULL);
>   			if (error)
>   				break;
>   		}
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 9f65758..2f66050 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -64,10 +64,18 @@ do {							\
>    *		a new subdirectory with this name.
>    * @is_visible:	Optional: Function to return permissions associated with an
>    *		attribute of the group. Will be called repeatedly for each
> - *		attribute in the group. Only read/write permissions as well as
> - *		SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
> - *		not visible. The returned value will replace static permissions
> - *		defined in struct attribute or struct bin_attribute.
> + *		non-binary attribute in the group. Only read/write
> + *		permissions as well as SYSFS_PREALLOC are accepted. Must
> + *		return 0 if an attribute is not visible. The returned value
> + *		will replace static permissions defined in struct attribute.
> + * @is_bin_visible:
> + *		Optional: Function to return permissions associated with a
> + *		binary attribute of the group. Will be called repeatedly
> + *		for each binary attribute in the group. Only read/write
> + *		permissions as well as SYSFS_PREALLOC are accepted. Must
> + *		return 0 if a binary attribute is not visible. The returned
> + *		value will replace static permissions defined in
> + *		struct bin_attribute.
>    * @attrs:	Pointer to NULL terminated list of attributes.
>    * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
>    *		Either attrs or bin_attrs or both must be provided.
> @@ -76,6 +84,8 @@ struct attribute_group {
>   	const char		*name;
>   	umode_t			(*is_visible)(struct kobject *,
>   					      struct attribute *, int);
> +	umode_t			(*is_bin_visible)(struct kobject *,
> +						  struct bin_attribute *, int);
>   	struct attribute	**attrs;
>   	struct bin_attribute	**bin_attrs;
>   };
>
diff mbox

Patch

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 39a0199..51b56e6 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -73,13 +73,26 @@  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 	}
 
 	if (grp->bin_attrs) {
-		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+		for (i = 0, bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
+			umode_t mode = (*bin_attr)->attr.mode;
+
 			if (update)
 				kernfs_remove_by_name(parent,
 						(*bin_attr)->attr.name);
+			if (grp->is_bin_visible) {
+				mode = grp->is_bin_visible(kobj, *bin_attr, i);
+				if (!mode)
+					continue;
+			}
+
+			WARN(mode & ~(SYSFS_PREALLOC | 0664),
+			     "Attribute %s: Invalid permissions 0%o\n",
+			     (*bin_attr)->attr.name, mode);
+
+			mode &= SYSFS_PREALLOC | 0664;
 			error = sysfs_add_file_mode_ns(parent,
 					&(*bin_attr)->attr, true,
-					(*bin_attr)->attr.mode, NULL);
+					mode, NULL);
 			if (error)
 				break;
 		}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9f65758..2f66050 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -64,10 +64,18 @@  do {							\
  *		a new subdirectory with this name.
  * @is_visible:	Optional: Function to return permissions associated with an
  *		attribute of the group. Will be called repeatedly for each
- *		attribute in the group. Only read/write permissions as well as
- *		SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
- *		not visible. The returned value will replace static permissions
- *		defined in struct attribute or struct bin_attribute.
+ *		non-binary attribute in the group. Only read/write
+ *		permissions as well as SYSFS_PREALLOC are accepted. Must
+ *		return 0 if an attribute is not visible. The returned value
+ *		will replace static permissions defined in struct attribute.
+ * @is_bin_visible:
+ *		Optional: Function to return permissions associated with a
+ *		binary attribute of the group. Will be called repeatedly
+ *		for each binary attribute in the group. Only read/write
+ *		permissions as well as SYSFS_PREALLOC are accepted. Must
+ *		return 0 if a binary attribute is not visible. The returned
+ *		value will replace static permissions defined in
+ *		struct bin_attribute.
  * @attrs:	Pointer to NULL terminated list of attributes.
  * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
  *		Either attrs or bin_attrs or both must be provided.
@@ -76,6 +84,8 @@  struct attribute_group {
 	const char		*name;
 	umode_t			(*is_visible)(struct kobject *,
 					      struct attribute *, int);
+	umode_t			(*is_bin_visible)(struct kobject *,
+						  struct bin_attribute *, int);
 	struct attribute	**attrs;
 	struct bin_attribute	**bin_attrs;
 };