linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC] sysfs.txt: add note on available attribute macros
@ 2019-02-14 11:21 Nicholas Mc Guire
  2019-02-14 12:44 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 2+ messages in thread
From: Nicholas Mc Guire @ 2019-02-14 11:21 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Greg Kroah-Hartman, linux-doc, linux-kernel, Nicholas Mc Guire

The common cases of attributes should probably be using the __ATTR_XXX
macros to make code more concise and readable but the current sysfs.txt
does not point developers to those convenience macros. Further there is
no note in sysfs.txt currently explaining why trying to set a sysfs file
to mode 0666 will fail.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Note quite sure if this is the right place to place the note on mode 0666 
but it should be somewhere as any attempt to set 0666 will be reverted 
to 0664.

 Documentation/filesystems/sysfs.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt
index 41411b0..c246142 100644
--- a/Documentation/filesystems/sysfs.txt
+++ b/Documentation/filesystems/sysfs.txt
@@ -116,6 +116,22 @@ static struct device_attribute dev_attr_foo = {
 	.store = store_foo,
 };
 
+Note as stated in include/linux/kernel.h "OTHER_WRITABLE?  Generally
+considered a bad idea." so trying to set a sysfs file writable for
+everyone will fail reverting to mode 0664.
+
+For this common cases sysfs.h provides convenience macros to
+make defining attributes easier as well as making code more
+concise and readable. The above case could be shortened to:
+
+static struct device_attribute dev_attr_foo = __ATTR_RW(foo);
+
+the list of helpers available is:
+__ATTR_RO(name): same as above with mode 0444
+__ATTR_WO(name): same as above with mode 0200
+__ATTR_RO_MODE(name, mode): allowing to pass in a specific mode
+__ATTR_RW(name): setting mode to 0644
+__ATTR_NULL: which sets the name to NULL
 
 Subsystem-Specific Callbacks
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH RFC] sysfs.txt: add note on available attribute macros
  2019-02-14 11:21 [PATCH RFC] sysfs.txt: add note on available attribute macros Nicholas Mc Guire
@ 2019-02-14 12:44 ` Greg Kroah-Hartman
  0 siblings, 0 replies; 2+ messages in thread
From: Greg Kroah-Hartman @ 2019-02-14 12:44 UTC (permalink / raw)
  To: Nicholas Mc Guire; +Cc: Jonathan Corbet, linux-doc, linux-kernel

On Thu, Feb 14, 2019 at 12:21:50PM +0100, Nicholas Mc Guire wrote:
> The common cases of attributes should probably be using the __ATTR_XXX
> macros to make code more concise and readable but the current sysfs.txt
> does not point developers to those convenience macros. Further there is
> no note in sysfs.txt currently explaining why trying to set a sysfs file
> to mode 0666 will fail.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> Note quite sure if this is the right place to place the note on mode 0666 
> but it should be somewhere as any attempt to set 0666 will be reverted 
> to 0664.
> 
>  Documentation/filesystems/sysfs.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt
> index 41411b0..c246142 100644
> --- a/Documentation/filesystems/sysfs.txt
> +++ b/Documentation/filesystems/sysfs.txt
> @@ -116,6 +116,22 @@ static struct device_attribute dev_attr_foo = {
>  	.store = store_foo,
>  };
>  
> +Note as stated in include/linux/kernel.h "OTHER_WRITABLE?  Generally
> +considered a bad idea." so trying to set a sysfs file writable for
> +everyone will fail reverting to mode 0664.
> +
> +For this common cases sysfs.h provides convenience macros to
> +make defining attributes easier as well as making code more
> +concise and readable. The above case could be shortened to:
> +
> +static struct device_attribute dev_attr_foo = __ATTR_RW(foo);
> +
> +the list of helpers available is:
> +__ATTR_RO(name): same as above with mode 0444
> +__ATTR_WO(name): same as above with mode 0200
> +__ATTR_RO_MODE(name, mode): allowing to pass in a specific mode
> +__ATTR_RW(name): setting mode to 0644
> +__ATTR_NULL: which sets the name to NULL

__ATTR_NULL is used to end a list of attributes, no one should actually
try to use it :)

And really, all the current users of it should be converted to use the
groups api instead, as it should not be needed.

__ATTR_RO_MODE() needs a bit more description as well.

So the idea of documenting these is great, just needs a bit of tweaking.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2019-02-14 12:44 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-14 11:21 [PATCH RFC] sysfs.txt: add note on available attribute macros Nicholas Mc Guire
2019-02-14 12:44 ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).