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

[PATCH 0967/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/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

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

[Matwey V. Kornilov]

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

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

[Matwey V. Kornilov]

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

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

[Greg KH]

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 :(