Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1453763 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0063/1285] Replace numeric parameter like 0444 with macro Baole Ni <baolex.ni@intel.com> - 2016-08-02 13:20 +0200
    Re: [PATCH 0063/1285] Replace numeric parameter like 0444 with macro Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-08-02 13:40 +0200
      Re: [PATCH 0063/1285] Replace numeric parameter like 0444 with macro Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-08-02 14:40 +0200
        Re: [PATCH 0063/1285] Replace numeric parameter like 0444 with macro Steven Rostedt <rostedt@goodmis.org> - 2016-08-02 15:30 +0200
          Re: [PATCH 0063/1285] Replace numeric parameter like 0444 with macro Marcel Holtmann <marcel@holtmann.org> - 2016-08-02 16:00 +0200
    Re: [PATCH 0063/1285] Replace numeric parameter like 0444 with  macro David Miller <davem@davemloft.net> - 2016-08-02 20:40 +0200
    Re: [PATCH 0063/1285] Replace numeric parameter like 0444 with macro Tejun Heo <tj@kernel.org> - 2016-08-02 20:40 +0200

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

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

[toc] | [next] | [standalone]

#1453829

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

[toc] | [prev] | [next] | [standalone]

#1454125

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

[toc] | [prev] | [next] | [standalone]

#1454334

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

[toc] | [prev] | [next] | [standalone]

#1454460

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

[toc] | [prev] | [next] | [standalone]

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

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

> the octal permission numbers are pretty much canonical

+1

[toc] | [prev] | [next] | [standalone]

#1455364

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web