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

[PATCH 0063/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>
---
 drivers/ata/pata_legacy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
index bce2a8c..1dc0beb 100644
--- a/drivers/ata/pata_legacy.c
+++ b/drivers/ata/pata_legacy.c
@@ -76,7 +76,7 @@
 #define NR_HOST 6
 
 static int all;
-module_param(all, int, 0444);
+module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH);
 MODULE_PARM_DESC(all, "Grab all legacy port devices, even if PCI(0=off, 1=on)");
 
 enum controller {
-- 
2.9.2

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

[Sergei Shtylyov]

Hello.

On 8/2/2016 1:37 PM, 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>
> ---
>  drivers/ata/pata_legacy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
> index bce2a8c..1dc0beb 100644
> --- a/drivers/ata/pata_legacy.c
> +++ b/drivers/ata/pata_legacy.c
> @@ -76,7 +76,7 @@
>  #define NR_HOST 6
>
>  static int all;
> -module_param(all, int, 0444);
> +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH);

    There's S_IRUGO for this case, no?

MBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 8/2/2016 2:30 PM, Sergei Shtylyov 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>
>> ---
>>  drivers/ata/pata_legacy.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
>> index bce2a8c..1dc0beb 100644
>> --- a/drivers/ata/pata_legacy.c
>> +++ b/drivers/ata/pata_legacy.c
>> @@ -76,7 +76,7 @@
>>  #define NR_HOST 6
>>
>>  static int all;
>> -module_param(all, int, 0444);
>> +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH);
>
>    There's S_IRUGO for this case, no?

    Sending 1285 patches with the same subject also was a bad idea. You need a 
subsystem/driver prefix in order to somehow differ them.

MBR, Sergei

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

[Steven Rostedt]

On Tue, Aug 02, 2016 at 03:32:57PM +0300, Sergei Shtylyov wrote:
> > >  static int all;
> > > -module_param(all, int, 0444);
> > > +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH);
> > 
> >    There's S_IRUGO for this case, no?

Sure, and honestly, I understand what 0444 is better than seeing:

  S_IRUSR | S_IRGRP | SIROTH

Heck, 0444 is more understandable to me than S_IRUGO, because honestly, those
macros are just as cryptic as 0444 is. Working with Unix/Linux systems since
1991, I understand the octo numbers very well. And I'm sure most other people
do to. Any file that I'm Cc'd on here will get an automatic NAK from me.

> 
>    Sending 1285 patches with the same subject also was a bad idea. You need
> a subsystem/driver prefix in order to somehow differ them.

Yes, it's a very good way to be added to everyone's /dev/null folder too. Each
subsystem should have one patch that covers all its files. Not a patch per
file!

What? Is Intel now give extra bonuses for commit numbers?

Sorry, but I'm a little grumpy when my phone starts popping like a popcorn
machine while I'm having my breakfast because of these silly emails.

-- Steve

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

[Marcel Holtmann]

Hi Steven,

>>>> static int all;
>>>> -module_param(all, int, 0444);
>>>> +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH);
>>> 
>>>   There's S_IRUGO for this case, no?
> 
> Sure, and honestly, I understand what 0444 is better than seeing:
> 
>  S_IRUSR | S_IRGRP | SIROTH
> 
> Heck, 0444 is more understandable to me than S_IRUGO, because honestly, those
> macros are just as cryptic as 0444 is. Working with Unix/Linux systems since
> 1991, I understand the octo numbers very well. And I'm sure most other people
> do to. Any file that I'm Cc'd on here will get an automatic NAK from me.

you are not helping to reduce the 1285 patch bomb. Your automatic replies make it worse now ;)

Regards

Marcel

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

[Tejun Heo]

Hello,

On Tue, Aug 02, 2016 at 06:37:59PM +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>

I don't see the point.  This actually makes code harder to read.
Sure, in general, we want to avoid using magic raw numbers but the
octal permission numbers are pretty much canonical.  These conversions
are pure noises.

Nacked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[David Miller]

From: Tejun Heo <tj@kernel.org>
Date: Tue, 2 Aug 2016 14:27:25 -0400

> the octal permission numbers are pretty much canonical

+1