linux-kernel - Re: [PATCH 0236/1285] Replace numeric parameter like 0444 with macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160802155452.31b922e9@endymion>
Date:	Tue, 2 Aug 2016 15:54:52 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Baole Ni <baolex.ni@...el.com>
Cc:	linux@...ck-us.net, airlied@...ux.ie, kgene@...nel.org,
	k.kozlowski@...sung.com, dougthompson@...ssion.com, bp@...en8.de,
	linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
	chuansheng.liu@...el.com, aduggan@...aptics.com,
	dianders@...omium.org
Subject: Re: [PATCH 0236/1285] Replace numeric parameter like 0444 with
 macro

Hi Baole,

On Tue,  2 Aug 2016 18:52:08 +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.

1285 patches? One patch per file? With Cc to a dozens of people? For a
very minor and highly discussable cleanup? Please stop this madness
immediately.

First of all, do a few patches and see what feedback you get. Then
you'll now if your patches are welcome, and maybe you can send more.
Then ask subsystem maintainers who are interested whether they prefer
one patch per driver, or one patch for the whole subsystem.

Personally I don't see any value in replacing a number I understand by
4 constants with cryptic names. If there's a bug in 0644 I'll see it
immediately. If there's a typo in S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
there are good chances I'll never notice.

So nack from me. This is neither more robust nor more readable. If it
was just me we'd kill all these useless macros right away.

> Signed-off-by: Chuansheng Liu <chuansheng.liu@...el.com>
> Signed-off-by: Baole Ni <baolex.ni@...el.com>
> ---
>  drivers/hwmon/acpi_power_meter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 579bdf9..11460f7 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -1008,7 +1008,7 @@ MODULE_AUTHOR("Darrick J. Wong <darrick.wong@...cle.com>");
>  MODULE_DESCRIPTION("ACPI 4.0 power meter driver");
>  MODULE_LICENSE("GPL");
>  
> -module_param(force_cap_on, bool, 0644);
> +module_param(force_cap_on, bool, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>  MODULE_PARM_DESC(force_cap_on, "Enable power cap even it is unsafe to do so.");
>  
>  module_init(acpi_power_meter_init);

-- 
Jean Delvare
SUSE L3 Support