[PATCH 0029/1285] Replace numeric parameter like 0444 with macro

[PATCH 0029/1285] Replace numeric parameter like 0444 with macro

[Baole Ni]

I find that the developers often just specified the numeric value
when calling a macro which is defined with a parameter for access permission.
As we know, these numeric value for access permission have had the corresponding macro,
and that using macro can improve the robustness and readability of the code,
thus, I suggest replacing the numeric parameter with the macro.

Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
Signed-off-by: Baole Ni <baolex.ni@intel.com>
---
 arch/tile/kernel/sysfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c
index 825867c..cef3937 100644
--- a/arch/tile/kernel/sysfs.c
+++ b/arch/tile/kernel/sysfs.c
@@ -38,7 +38,7 @@ static ssize_t chip_width_show(struct device *dev,
 {
 	return sprintf(page, "%u\n", smp_width);
 }
-static DEVICE_ATTR(chip_width, 0444, chip_width_show, NULL);
+static DEVICE_ATTR(chip_width, S_IRUSR | S_IRGRP | S_IROTH, chip_width_show, NULL);
 
 static ssize_t chip_height_show(struct device *dev,
 				struct device_attribute *attr,
@@ -46,7 +46,7 @@ static ssize_t chip_height_show(struct device *dev,
 {
 	return sprintf(page, "%u\n", smp_height);
 }
-static DEVICE_ATTR(chip_height, 0444, chip_height_show, NULL);
+static DEVICE_ATTR(chip_height, S_IRUSR | S_IRGRP | S_IROTH, chip_height_show, NULL);
 
 static ssize_t chip_serial_show(struct device *dev,
 				struct device_attribute *attr,
@@ -54,7 +54,7 @@ static ssize_t chip_serial_show(struct device *dev,
 {
 	return get_hv_confstr(page, HV_CONFSTR_CHIP_SERIAL_NUM);
 }
-static DEVICE_ATTR(chip_serial, 0444, chip_serial_show, NULL);
+static DEVICE_ATTR(chip_serial, S_IRUSR | S_IRGRP | S_IROTH, chip_serial_show, NULL);
 
 static ssize_t chip_revision_show(struct device *dev,
 				  struct device_attribute *attr,
@@ -62,7 +62,7 @@ static ssize_t chip_revision_show(struct device *dev,
 {
 	return get_hv_confstr(page, HV_CONFSTR_CHIP_REV);
 }
-static DEVICE_ATTR(chip_revision, 0444, chip_revision_show, NULL);
+static DEVICE_ATTR(chip_revision, S_IRUSR | S_IRGRP | S_IROTH, chip_revision_show, NULL);
 
 
 static ssize_t type_show(struct device *dev,
@@ -71,7 +71,7 @@ static ssize_t type_show(struct device *dev,
 {
 	return sprintf(page, "tilera\n");
 }
-static DEVICE_ATTR(type, 0444, type_show, NULL);
+static DEVICE_ATTR(type, S_IRUSR | S_IRGRP | S_IROTH, type_show, NULL);
 
 #define HV_CONF_ATTR(name, conf)					\
 	static ssize_t name ## _show(struct device *dev,		\
@@ -184,7 +184,7 @@ static ssize_t hv_stats_store(struct device *dev,
 	return n < 0 ? n : count;
 }
 
-static DEVICE_ATTR(hv_stats, 0644, hv_stats_show, hv_stats_store);
+static DEVICE_ATTR(hv_stats, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH, hv_stats_show, hv_stats_store);
 
 static int hv_stats_device_add(struct device *dev, struct subsys_interface *sif)
 {
-- 
2.9.2

Re: [PATCH 0029/1285] Replace numeric parameter like 0444 with macro

[Greg KH]

On Tue, Aug 02, 2016 at 06:35:37PM +0800, Baole Ni wrote:
> I find that the developers often just specified the numeric value
> when calling a macro which is defined with a parameter for access permission.
> As we know, these numeric value for access permission have had the corresponding macro,
> and that using macro can improve the robustness and readability of the code,
> thus, I suggest replacing the numeric parameter with the macro.
> 
> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Signed-off-by: Baole Ni <baolex.ni@intel.com>
> ---
>  arch/tile/kernel/sysfs.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c
> index 825867c..cef3937 100644
> --- a/arch/tile/kernel/sysfs.c
> +++ b/arch/tile/kernel/sysfs.c
> @@ -38,7 +38,7 @@ static ssize_t chip_width_show(struct device *dev,
>  {
>  	return sprintf(page, "%u\n", smp_width);
>  }
> -static DEVICE_ATTR(chip_width, 0444, chip_width_show, NULL);
> +static DEVICE_ATTR(chip_width, S_IRUSR | S_IRGRP | S_IROTH, chip_width_show, NULL);

You do know about S_IRUGO, right?  Why not use that?

Same goes for your other replacements, use the correct #define, not a
mix of values | together.

And you are sending hundreds of patches with the same identical subject
line, that's not ok, you need to make it unique for every patch.
Usually that is done with the subsystem and driver name, see examples of
this in the kernel changelogs.

Please fix up and try again, in a much smaller series, before trying to
do this across the whole kernel tree.

good luck!

greg k-h

Re: [PATCH 0029/1285] Replace numeric parameter like 0444 with macro

[Rich Felker]

On Tue, Aug 02, 2016 at 02:16:17PM +0200, Greg KH wrote:
> On Tue, Aug 02, 2016 at 06:35:37PM +0800, Baole Ni wrote:
> > I find that the developers often just specified the numeric value
> > when calling a macro which is defined with a parameter for access permission.
> > As we know, these numeric value for access permission have had the corresponding macro,
> > and that using macro can improve the robustness and readability of the code,
> > thus, I suggest replacing the numeric parameter with the macro.
> > 
> > Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> > Signed-off-by: Baole Ni <baolex.ni@intel.com>
> > ---
> >  arch/tile/kernel/sysfs.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c
> > index 825867c..cef3937 100644
> > --- a/arch/tile/kernel/sysfs.c
> > +++ b/arch/tile/kernel/sysfs.c
> > @@ -38,7 +38,7 @@ static ssize_t chip_width_show(struct device *dev,
> >  {
> >  	return sprintf(page, "%u\n", smp_width);
> >  }
> > -static DEVICE_ATTR(chip_width, 0444, chip_width_show, NULL);
> > +static DEVICE_ATTR(chip_width, S_IRUSR | S_IRGRP | S_IROTH, chip_width_show, NULL);
> 
> You do know about S_IRUGO, right?  Why not use that?
> 
> Same goes for your other replacements, use the correct #define, not a
> mix of values | together.
> 
> And you are sending hundreds of patches with the same identical subject
> line, that's not ok, you need to make it unique for every patch.
> Usually that is done with the subsystem and driver name, see examples of
> this in the kernel changelogs.
> 
> Please fix up and try again, in a much smaller series, before trying to
> do this across the whole kernel tree.
> 
> good luck!

While I won't object to the parts I'm maintainer for if there's a
general consensus this is an improvement, I don't think it's an
improvement, even with S_IRUGO. It's a significant decrease in
readability. Everybody who works on this code knows immediately and
intuitively what 0444 means. I have to actually stop and think about
what S_IRUGO means.

>From my perspective, these macros were a mistake from a time when
standardization efforts wrongly thought of file permission mode values
as "not a public interface". POSIX since corrected that (in the 2008
edition, from what I can tell); the specific values everybody expects
are mandated.

Rich

Re: [PATCH 0029/1285] Replace numeric parameter like 0444 with macro

[Linus Torvalds]

On Tue, Aug 2, 2016 at 8:16 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>  }
>> -static DEVICE_ATTR(chip_width, 0444, chip_width_show, NULL);
>> +static DEVICE_ATTR(chip_width, S_IRUSR | S_IRGRP | S_IROTH, chip_width_show, NULL);

I detest these patches.

It's actually *MUCH* easer to see that "oh, everybody gets to read"
when you see something like 0444, and seeing 0600 is a very clear
"owner only".

I contrast, something like S_IRUSR | S_IRGRP | S_IROTH you actually
have to _read_ to figure it out.

I seriously believe that when it comes to UNIX chmod masks, the octal
representation is much better.

> You do know about S_IRUGO, right?  Why not use that?

Actually, that one is even worse. And part of it is exactly that there
are nineteen billion of these "random letter combinations" that are
visually almost entirely indistinguishable from each other, so part of
the problem with the symbolic names is exactly that you have to read
them, and you have to read them *CAREFULLY*.

In contrast, 0444 is completely unambiguous, and doesn't take a lot of
effort to parse.

Honestly, the whole "use symbolic names instead of numbers" only makes
sense if the symbolic names are easier to parse or understand. In the
case of the unix permission masks, that simply isn't the case. Never
has been. Those names are an abomination, and should just die.

So I don't want to see _any_ of these patches being applied,
personally. Much less being bombarded with hundreds of patches with
the same subject line and contents.

                   Linus