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

Groups > linux.kernel > #1454236 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0967/1285] Replace numeric parameter like 0444 with macro Baole Ni <baolex.ni@intel.com> - 2016-08-02 15:00 +0200
    Re: [PATCH 0967/1285] Replace numeric parameter like 0444 with macro "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-08-02 15:40 +0200
      Re: [PATCH 0967/1285] Replace numeric parameter like 0444 with macro Greg KH <gregkh@linuxfoundation.org> - 2016-08-02 19:00 +0200
    Re: [PATCH 0967/1285] Replace numeric parameter like 0444 with macro "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-08-02 16:30 +0200

#1454236 — [PATCH 0967/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/tty/serial/8250/8250_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 0fbd7c0..9309ec1 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1188,17 +1188,17 @@ module_exit(serial8250_exit);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Generic 8250/16x50 serial driver");
 
-module_param(share_irqs, uint, 0644);
+module_param(share_irqs, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices (unsafe)");
 
-module_param(nr_uarts, uint, 0644);
+module_param(nr_uarts, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
 
-module_param(skip_txen_test, uint, 0644);
+module_param(skip_txen_test, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
 
 #ifdef CONFIG_SERIAL_8250_RSA
-module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
+module_param_array(probe_rsa, ulong, &probe_rsa_count, S_IRUSR | S_IRGRP | S_IROTH);
 MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
 #endif
 MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
-- 
2.9.2

[toc] | [next] | [standalone]

#1454374

And actually S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH  can be simplified
to S_IRUGO | S_IWUSR, not?
I think we need to have and use dedicated special macros for most
common magic octal permissions like 0755 or 0644.

2016-08-02 16:19 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> Hello,
>
> I believe that 0644 is shorter and easier to read and understand than
> the long list of macros like S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
> where it is easier to miss something.
>
> 2016-08-02 15:03 GMT+03:00 Baole Ni <baolex.ni@intel.com>:
>> 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/tty/serial/8250/8250_core.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index 0fbd7c0..9309ec1 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -1188,17 +1188,17 @@ module_exit(serial8250_exit);
>>  MODULE_LICENSE("GPL");
>>  MODULE_DESCRIPTION("Generic 8250/16x50 serial driver");
>>
>> -module_param(share_irqs, uint, 0644);
>> +module_param(share_irqs, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>>  MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices (unsafe)");
>>
>> -module_param(nr_uarts, uint, 0644);
>> +module_param(nr_uarts, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>>  MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
>>
>> -module_param(skip_txen_test, uint, 0644);
>> +module_param(skip_txen_test, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>>  MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
>>
>>  #ifdef CONFIG_SERIAL_8250_RSA
>> -module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
>> +module_param_array(probe_rsa, ulong, &probe_rsa_count, S_IRUSR | S_IRGRP | S_IROTH);
>>  MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
>>  #endif
>>  MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
>> --
>> 2.9.2
>>
>
>
>
> --
> With best regards,
> Matwey V. Kornilov.
> Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
> 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

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

#1455024

On Tue, Aug 02, 2016 at 04:34:20PM +0300, Matwey V. Kornilov wrote:
> And actually S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH  can be simplified
> to S_IRUGO | S_IWUSR, not?
> I think we need to have and use dedicated special macros for most
> common magic octal permissions like 0755 or 0644.

We already do have them, the developer here decided to not use them for
some unknown reason, making all 1285 of these patches wrong :(

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

#1454546

Hello,

I believe that 0644 is shorter and easier to read and understand than
the long list of macros like S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
where it is easier to miss something.

2016-08-02 15:03 GMT+03:00 Baole Ni <baolex.ni@intel.com>:
> 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/tty/serial/8250/8250_core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 0fbd7c0..9309ec1 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1188,17 +1188,17 @@ module_exit(serial8250_exit);
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Generic 8250/16x50 serial driver");
>
> -module_param(share_irqs, uint, 0644);
> +module_param(share_irqs, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>  MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices (unsafe)");
>
> -module_param(nr_uarts, uint, 0644);
> +module_param(nr_uarts, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>  MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
>
> -module_param(skip_txen_test, uint, 0644);
> +module_param(skip_txen_test, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>  MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
>
>  #ifdef CONFIG_SERIAL_8250_RSA
> -module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
> +module_param_array(probe_rsa, ulong, &probe_rsa_count, S_IRUSR | S_IRGRP | S_IROTH);
>  MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
>  #endif
>  MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
> --
> 2.9.2
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web