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

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0984/1285] Replace numeric parameter like 0444 with macro
@ 2016-08-02 12:05 Baole Ni
  2016-08-02 13:54 ` Felipe Balbi
  0 siblings, 1 reply; 6+ messages in thread
From: Baole Ni @ 2016-08-02 12:05 UTC (permalink / raw)
  To: gregkh, m.chehab, m.szyprowski, kyungmin.park, k.kozlowski
  Cc: linux-usb, linux-kernel, felipe.balbi, peter.chen, mina86,
	deepa.kernel, mathias.nyman, stern, chuansheng.liu, baolex.ni,
	oneukum

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/usb/misc/usbtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 6b978f0..5e81dc3 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -15,7 +15,7 @@
 /*-------------------------------------------------------------------------*/
 
 static int override_alt = -1;
-module_param_named(alt, override_alt, int, 0644);
+module_param_named(alt, override_alt, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 MODULE_PARM_DESC(alt, ">= 0 to override altsetting selection");
 static void complicated_callback(struct urb *urb);
 
-- 
2.9.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 0984/1285] Replace numeric parameter like 0444 with macro
  2016-08-02 12:05 [PATCH 0984/1285] Replace numeric parameter like 0444 with macro Baole Ni
@ 2016-08-02 13:54 ` Felipe Balbi
  2016-08-02 14:03   ` Marcel Holtmann
                     ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Felipe Balbi @ 2016-08-02 13:54 UTC (permalink / raw)
  To: Baole Ni, gregkh, m.chehab, m.szyprowski, kyungmin.park,
	k.kozlowski
  Cc: linux-usb, linux-kernel, peter.chen, mina86, deepa.kernel,
	mathias.nyman, stern, chuansheng.liu, baolex.ni, oneukum

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

Baole Ni <baolex.ni@intel.com> writes:

> 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/usb/misc/usbtest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> index 6b978f0..5e81dc3 100644
> --- a/drivers/usb/misc/usbtest.c
> +++ b/drivers/usb/misc/usbtest.c
> @@ -15,7 +15,7 @@
>  /*-------------------------------------------------------------------------*/
>  
>  static int override_alt = -1;
> -module_param_named(alt, override_alt, int, 0644);
> +module_param_named(alt, override_alt, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);

line too long. You need to run this series through scripts/checkpatch.pl

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0984/1285] Replace numeric parameter like 0444 with macro
  2016-08-02 13:54 ` Felipe Balbi
@ 2016-08-02 14:03   ` Marcel Holtmann
  2016-08-02 14:04   ` Marcel Holtmann
  2016-08-03  8:30   ` Oliver Neukum
  2 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2016-08-02 14:03 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Baole Ni, Greg Kroah-Hartman, m.chehab, Marek Szyprowski,
	kyungmin.park, k.kozlowski, linux-usb, linux-kernel, peter.chen,
	mina86, deepa.kernel, mathias.nyman, stern, chuansheng.liu,
	oneukum

Hi Felipe,

>> 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/usb/misc/usbtest.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
>> index 6b978f0..5e81dc3 100644
>> --- a/drivers/usb/misc/usbtest.c
>> +++ b/drivers/usb/misc/usbtest.c
>> @@ -15,7 +15,7 @@
>> /*-------------------------------------------------------------------------*/
>> 
>> static int override_alt = -1;
>> -module_param_named(alt, override_alt, int, 0644);
>> +module_param_named(alt, override_alt, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> 
> line too long. You need to run this series through scripts/checkpatch.pl

please don't give them any ideas. Next thing you know and another 1285 patch bomb is coming our way.

Regards

Marcel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0984/1285] Replace numeric parameter like 0444 with macro
  2016-08-02 13:54 ` Felipe Balbi
  2016-08-02 14:03   ` Marcel Holtmann
@ 2016-08-02 14:04   ` Marcel Holtmann
  2016-08-03  8:30   ` Oliver Neukum
  2 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2016-08-02 14:04 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Baole Ni, Greg Kroah-Hartman, m.chehab, Marek Szyprowski,
	kyungmin.park, k.kozlowski, linux-usb, linux-kernel, peter.chen,
	mina86, deepa.kernel, mathias.nyman, stern, chuansheng.liu,
	oneukum

Hi Felipe,

>> 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/usb/misc/usbtest.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
>> index 6b978f0..5e81dc3 100644
>> --- a/drivers/usb/misc/usbtest.c
>> +++ b/drivers/usb/misc/usbtest.c
>> @@ -15,7 +15,7 @@
>> /*-------------------------------------------------------------------------*/
>> 
>> static int override_alt = -1;
>> -module_param_named(alt, override_alt, int, 0644);
>> +module_param_named(alt, override_alt, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> 
> line too long. You need to run this series through scripts/checkpatch.pl

please don't give them any ideas. Next thing you know and another 1285 patch bomb is coming our way.

Regards

Marcel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0984/1285] Replace numeric parameter like 0444 with macro
  2016-08-02 13:54 ` Felipe Balbi
  2016-08-02 14:03   ` Marcel Holtmann
  2016-08-02 14:04   ` Marcel Holtmann
@ 2016-08-03  8:30   ` Oliver Neukum
  2016-08-03 11:15     ` Michal Nazarewicz
  2 siblings, 1 reply; 6+ messages in thread
From: Oliver Neukum @ 2016-08-03  8:30 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: gregkh, k.kozlowski, kyungmin.park, m.chehab, m.szyprowski,
	peter.chen, deepa.kernel, baolex.ni, chuansheng.liu,
	mathias.nyman, mina86, stern, linux-kernel, linux-usb

On Tue, 2016-08-02 at 16:54 +0300, Felipe Balbi wrote:
> Baole Ni <baolex.ni@intel.com> writes:
> 
> > 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/usb/misc/usbtest.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> > index 6b978f0..5e81dc3 100644
> > --- a/drivers/usb/misc/usbtest.c
> > +++ b/drivers/usb/misc/usbtest.c
> > @@ -15,7 +15,7 @@
> >  /*-------------------------------------------------------------------------*/
> >  
> >  static int override_alt = -1;
> > -module_param_named(alt, override_alt, int, 0644);
> > +module_param_named(alt, override_alt, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> 
> line too long. You need to run this series through scripts/checkpatch.pl
> 

Before we think about that, the basic question whether

S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH

is clearer and easier to read than

0644

must be decided. I would saz no, it is not.

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0984/1285] Replace numeric parameter like 0444 with macro
  2016-08-03  8:30   ` Oliver Neukum
@ 2016-08-03 11:15     ` Michal Nazarewicz
  0 siblings, 0 replies; 6+ messages in thread
From: Michal Nazarewicz @ 2016-08-03 11:15 UTC (permalink / raw)
  To: Oliver Neukum, Felipe Balbi
  Cc: gregkh, k.kozlowski, kyungmin.park, m.chehab, m.szyprowski,
	peter.chen, deepa.kernel, baolex.ni, chuansheng.liu,
	mathias.nyman, stern, linux-kernel, linux-usb

On Wed, Aug 03 2016, Oliver Neukum wrote:
> Before we think about that, the basic question whether
>
> S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
>
> is clearer and easier to read than
>
> 0644
>
> must be decided. I would saz no, it is not.

I was about to write the same thing.

I dislike magic numbers just like the next guy, but this replaces
a compact representation of the permissions with a long string of hard
to read, awkwardly abbreviated strings.

On personal note, I can never remember whether ‘u’ means user and ‘o’
means other or ‘u’ means users and ‘o’ means ‘owner’.  In cited case
this is somehow averted because both USR and OTH are present, but what
does ‘S_IRWXU’ mean is a mystery to me.

To my mind, the macros make sense only when testing for particular bit
being set.  Something like:

	if (mode & S_IRUSR && check_if_user_can_read())
		success;

could be argued as better than ‘mode & 0400’ but even than the awkward
abbreviation doesn’t help.  Again, ‘PERM_USER_READABLE’ would be much
better (also for the reason mentioned above).

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-08-03 12:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-02 12:05 [PATCH 0984/1285] Replace numeric parameter like 0444 with macro Baole Ni
2016-08-02 13:54 ` Felipe Balbi
2016-08-02 14:03   ` Marcel Holtmann
2016-08-02 14:04   ` Marcel Holtmann
2016-08-03  8:30   ` Oliver Neukum
2016-08-03 11:15     ` Michal Nazarewicz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox