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

[PATCH 0001/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/arm/common/bL_switcher.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c
index 37dc0fe..bd51c35 100644
--- a/arch/arm/common/bL_switcher.c
+++ b/arch/arm/common/bL_switcher.c
@@ -773,7 +773,7 @@ static int bL_switcher_hotplug_callback(struct notifier_block *nfb,
 }
 
 static bool no_bL_switcher;
-core_param(no_bL_switcher, no_bL_switcher, bool, 0644);
+core_param(no_bL_switcher, no_bL_switcher, bool, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 
 static int __init bL_switcher_init(void)
 {
-- 
2.9.2

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

[Russell King - ARM Linux]

On Tue, Aug 02, 2016 at 06:33:22PM +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.

Sending a huge patch series is always a no-no.  Please:

(a) group the patches according to maintainer
(b) send in smaller chunks, especially when starting off a new cleanup

I'm not sure why I received powerpc and ia64 changes in addition to ARM
changes, I've nothing to do with powerpc and ia64.  In any case, I'm
not going to read each individual mail to find out whether the patch
is something that is relevent or not.

> 
> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Signed-off-by: Baole Ni <baolex.ni@intel.com>
> ---
>  arch/arm/common/bL_switcher.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c
> index 37dc0fe..bd51c35 100644
> --- a/arch/arm/common/bL_switcher.c
> +++ b/arch/arm/common/bL_switcher.c
> @@ -773,7 +773,7 @@ static int bL_switcher_hotplug_callback(struct notifier_block *nfb,
>  }
>  
>  static bool no_bL_switcher;
> -core_param(no_bL_switcher, no_bL_switcher, bool, 0644);
> +core_param(no_bL_switcher, no_bL_switcher, bool, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);

This unnecessarily violates the column limit that we have in the coding
style.  It's also a very simplistic conversion from the octal constant
to the definitions.

core_param(no_bL_switcher, no_bL_switcher, bool, S_IRUGO | S_IWUSR);

would be a better way of saying the same thing - see include/linux/stat.h

However, the cleanup of file modes is at best of questionable value.
Octal file modes are something of a Unix standard - see the chmod man
page.  So, I don't see there's even a need to change file modes to
symbolic constants, especially when it means a _lot_ of mail noise.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[One Thousand Gnomes]

On Tue,  2 Aug 2016 18:33:22 +0800
Baole Ni <baolex.ni@intel.com> 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.

If you are going to do this then please generate one patch per subsystem,
and just do a couple of subsystems initially until everyone is happy with
how it is being converted, then send out more subsystem by subsystem.

Alan

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

[Masami Hiramatsu]

On Tue, 2 Aug 2016 14:15:03 +0100
One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:

> On Tue,  2 Aug 2016 18:33:22 +0800
> Baole Ni <baolex.ni@intel.com> 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.
> 
> If you are going to do this then please generate one patch per subsystem,
> and just do a couple of subsystems initially until everyone is happy with
> how it is being converted, then send out more subsystem by subsystem.

+1

Moreover, please DO NOT use same subject and same description for each patch (explain
what you did and why for each one), unless it will be just spamming git-log...

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Pavel Machek]

Hi!

> 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.

> -core_param(no_bL_switcher, no_bL_switcher, bool, 0644);
> +core_param(no_bL_switcher, no_bL_switcher, bool, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);

Everyone knows what 0644 is, but noone can read S_IRUSR | S_IWUSR |
S_IRCRP | S_IROTH (*). Please don't do this.

									Pavel
















































(*) I deliberately included an error there. Did you spot the error?
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Joe Perches]

On Tue, 2016-08-02 at 19:42 +0200, Pavel Machek 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.
> > 
> > -core_param(no_bL_switcher, no_bL_switcher, bool, 0644);
> > +core_param(no_bL_switcher, no_bL_switcher, bool, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);

> Everyone knows what 0644 is, but noone can read S_IRUSR | S_IWUSR |
> S_IRCRP | S_IROTH (*). Please don't do this.

Perhaps this conversion is best done in reverse with
most all of the S_[A-Z]{5,5} uses converted to octal.

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

[Pavel Machek]

On Tue 2016-08-02 10:52:18, Joe Perches wrote:
> On Tue, 2016-08-02 at 19:42 +0200, Pavel Machek 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.
> > > 
> > > -core_param(no_bL_switcher, no_bL_switcher, bool, 0644);
> > > +core_param(no_bL_switcher, no_bL_switcher, bool, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> 
> > Everyone knows what 0644 is, but noone can read S_IRUSR | S_IWUSR |
> > S_IRCRP | S_IROTH (*). Please don't do this.
> 
> Perhaps this conversion is best done in reverse with
> most all of the S_[A-Z]{5,5} uses converted to octal.

I'd prefer that, yes.. But lets discuss that before another 1200 patch
patchbomb...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html