Re: [PATCH] powerpc: Use octal numbers for file permissions

[Cyril Bur]

On Tue, 2017-01-17 at 20:52 +1100, Michael Ellerman wrote:
> Cyril Bur  writes:
> 
> > On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote:
> > > Symbolic macros are unintuitive and hard to read, whereas octal constants
> > > are much easier to interpret.  Replace macros for the basic permission
> > > flags (user/group/other read/write/execute) with numeric constants
> > > instead, across the whole powerpc tree.
> > > 
> > > Introducing a significant number of changes across the tree for no runtime
> > > benefit isn't exactly desirable, but so long as these macros are still
> > > used in the tree people will keep sending patches that add them.  Not only
> > > are they hard to parse at a glance, there are multiple ways of coming to
> > > the same value (as you can see with 0444 and 0644 in this patch) which
> > > hurts readability.
> > > 
> > > Signed-off-by: Russell Currey 
> > 
> > Reviewed-by: Cyril Bur 
> 
> Did you really really review every single change?
> 

Yes. I just went through it again and still didn't find any mistakes.

> Because if you did then I don't have to, and that would be *great* :)
> 

My pleasure :)

Interestingly enough, reviewing this patch taught me to quickly parse
the symbolics, which, for me now makes this patch less important haha.
I'm still in favour! No matter how fast you get at the symolics the
octal is still faster, also there's only one way to write the octal
permissions!


> cheers

Re: [PATCH] powerpc: Use octal numbers for file permissions

[Oliver O'Halloran]

It has been pointed out that this actually occured in 2017. My apologies.

On 17/01/2017 9:50 PM, "Oliver O'Halloran"  wrote:

> "It's possible I missed one, but I did genuinely review all of it"
>
> Cyril Bur, 2016
> In a hobart pub, specifically The Winston
>
> On 17/01/2017 8:53 PM, "Michael Ellerman"  wrote:
>
>> Cyril Bur  writes:
>>
>> > On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote:
>> >> Symbolic macros are unintuitive and hard to read, whereas octal
>> constants
>> >> are much easier to interpret.  Replace macros for the basic permission
>> >> flags (user/group/other read/write/execute) with numeric constants
>> >> instead, across the whole powerpc tree.
>> >>
>> >> Introducing a significant number of changes across the tree for no
>> runtime
>> >> benefit isn't exactly desirable, but so long as these macros are still
>> >> used in the tree people will keep sending patches that add them.  Not
>> only
>> >> are they hard to parse at a glance, there are multiple ways of coming
>> to
>> >> the same value (as you can see with 0444 and 0644 in this patch) which
>> >> hurts readability.
>> >>
>> >> Signed-off-by: Russell Currey 
>> >
>> > Reviewed-by: Cyril Bur 
>>
>> Did you really really review every single change?
>>
>> Because if you did then I don't have to, and that would be *great* :)
>>
>> cheers
>>
>

Re: [PATCH] powerpc: Use octal numbers for file permissions

[Oliver O'Halloran]

"It's possible I missed one, but I did genuinely review all of it"

Cyril Bur, 2016
In a hobart pub, specifically The Winston

On 17/01/2017 8:53 PM, "Michael Ellerman"  wrote:

> Cyril Bur  writes:
>
> > On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote:
> >> Symbolic macros are unintuitive and hard to read, whereas octal
> constants
> >> are much easier to interpret.  Replace macros for the basic permission
> >> flags (user/group/other read/write/execute) with numeric constants
> >> instead, across the whole powerpc tree.
> >>
> >> Introducing a significant number of changes across the tree for no
> runtime
> >> benefit isn't exactly desirable, but so long as these macros are still
> >> used in the tree people will keep sending patches that add them.  Not
> only
> >> are they hard to parse at a glance, there are multiple ways of coming to
> >> the same value (as you can see with 0444 and 0644 in this patch) which
> >> hurts readability.
> >>
> >> Signed-off-by: Russell Currey 
> >
> > Reviewed-by: Cyril Bur 
>
> Did you really really review every single change?
>
> Because if you did then I don't have to, and that would be *great* :)
>
> cheers
>

Re: [PATCH] powerpc: Use octal numbers for file permissions

[Michael Ellerman]

Cyril Bur  writes:

> On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote:
>> Symbolic macros are unintuitive and hard to read, whereas octal constants
>> are much easier to interpret.  Replace macros for the basic permission
>> flags (user/group/other read/write/execute) with numeric constants
>> instead, across the whole powerpc tree.
>> 
>> Introducing a significant number of changes across the tree for no runtime
>> benefit isn't exactly desirable, but so long as these macros are still
>> used in the tree people will keep sending patches that add them.  Not only
>> are they hard to parse at a glance, there are multiple ways of coming to
>> the same value (as you can see with 0444 and 0644 in this patch) which
>> hurts readability.
>> 
>> Signed-off-by: Russell Currey 
>
> Reviewed-by: Cyril Bur 

Did you really really review every single change?

Because if you did then I don't have to, and that would be *great* :)

cheers

Re: [PATCH] powerpc: Use octal numbers for file permissions

[Balbir Singh]

On Mon, Jan 16, 2017 at 11:21:52AM +1100, Russell Currey wrote:
> On Fri, 2017-01-13 at 13:41 +0530, Balbir Singh wrote:
> > On Thu, Jan 12, 2017 at 02:54:13PM +1100, Russell Currey wrote:
> > > Symbolic macros are unintuitive and hard to read, whereas octal constants
> > > are much easier to interpret.  Replace macros for the basic permission
> > > flags (user/group/other read/write/execute) with numeric constants
> > > instead, across the whole powerpc tree.
> > > 
> > 
> > I know Linus said otherwise, but I wonder if the churn is worth it.
> > At user mode (do man 2 chmod), these constants are used frequently,
> > even with chmod the command we use chmod a+r equivalents or chmod
> > u+r. My big concern with numbers is how do you know you did not
> > turn on the sticky bit for a file? Can you imagine if someone used
> > 0x644 or 0x444 would we catch it?
> 
> I would certainly expect something like that would be caught.
>

OK.. Lets hope so.
 
> > 
> > Not resisting, but thinking if the churn and what follows might be
> > OK.
> 
> So long as the constants are still in the tree people will still send patches
> with them (which continues to happen even though there's a checkpatch 
> warning). 
> Constants have the issue that the same value can be written multiple ways 
> (which
> is misleading) - some of the files I touched come about the same set of
> permissions different ways or even mix octal values and macros within the same
> file.

I don't think anyone prevents 0444 | 0200 from being sent. It's just that
associativity rules allow for composing things differently.

> 
> I think using octal values for rwx (and sticking to macros for things like the
> sticky bit) is on the side of simplicity and consistency.
>

Fair enough, maintainer gets to decide :)

Balbir Singh. 

Re: [PATCH] powerpc: Use octal numbers for file permissions

[Russell Currey]

On Fri, 2017-01-13 at 13:41 +0530, Balbir Singh wrote:
> On Thu, Jan 12, 2017 at 02:54:13PM +1100, Russell Currey wrote:
> > Symbolic macros are unintuitive and hard to read, whereas octal constants
> > are much easier to interpret.  Replace macros for the basic permission
> > flags (user/group/other read/write/execute) with numeric constants
> > instead, across the whole powerpc tree.
> > 
> 
> I know Linus said otherwise, but I wonder if the churn is worth it.
> At user mode (do man 2 chmod), these constants are used frequently,
> even with chmod the command we use chmod a+r equivalents or chmod
> u+r. My big concern with numbers is how do you know you did not
> turn on the sticky bit for a file? Can you imagine if someone used
> 0x644 or 0x444 would we catch it?

I would certainly expect something like that would be caught.

> 
> Not resisting, but thinking if the churn and what follows might be
> OK.

So long as the constants are still in the tree people will still send patches
with them (which continues to happen even though there's a checkpatch warning). 
Constants have the issue that the same value can be written multiple ways (which
is misleading) - some of the files I touched come about the same set of
permissions different ways or even mix octal values and macros within the same
file.

I think using octal values for rwx (and sticking to macros for things like the
sticky bit) is on the side of simplicity and consistency.

> 
> Balbir Singh.

Re: [PATCH] powerpc: Use octal numbers for file permissions

[Balbir Singh]

On Thu, Jan 12, 2017 at 02:54:13PM +1100, Russell Currey wrote:
> Symbolic macros are unintuitive and hard to read, whereas octal constants
> are much easier to interpret.  Replace macros for the basic permission
> flags (user/group/other read/write/execute) with numeric constants
> instead, across the whole powerpc tree.
>

I know Linus said otherwise, but I wonder if the churn is worth it.
At user mode (do man 2 chmod), these constants are used frequently,
even with chmod the command we use chmod a+r equivalents or chmod
u+r. My big concern with numbers is how do you know you did not
turn on the sticky bit for a file? Can you imagine if someone used
0x644 or 0x444 would we catch it?

Not resisting, but thinking if the churn and what follows might be
OK.

Balbir Singh.

Re: [PATCH] powerpc: Use octal numbers for file permissions

[Cyril Bur]

On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote:
> Symbolic macros are unintuitive and hard to read, whereas octal constants
> are much easier to interpret.  Replace macros for the basic permission
> flags (user/group/other read/write/execute) with numeric constants
> instead, across the whole powerpc tree.
> 
> Introducing a significant number of changes across the tree for no runtime
> benefit isn't exactly desirable, but so long as these macros are still
> used in the tree people will keep sending patches that add them.  Not only
> are they hard to parse at a glance, there are multiple ways of coming to
> the same value (as you can see with 0444 and 0644 in this patch) which
> hurts readability.
> 
> Signed-off-by: Russell Currey 

Reviewed-by: Cyril Bur 

> ---
> I wondered what a "S_IRUGO" was and subsequently found the following:
>   https://lwn.net/Articles/696229/
> so I figured making numeric constants standard across the tree was a good
> idea instead of the mix we currently have.
> 
> For new patches that come in, checkpatch warns when using something like
> S_IRUGO and tells you to use something like 0444 instead.

I wonder if both these points shouldn't actually be in the commit
message?

> ---
>  arch/powerpc/kernel/eeh_sysfs.c|  2 +-
>  arch/powerpc/kernel/iommu.c|  2 +-
>  arch/powerpc/kernel/proc_powerpc.c |  2 +-
>  arch/powerpc/kernel/rtas-proc.c| 14 +++---
>  arch/powerpc/kernel/rtas_flash.c   |  2 +-
>  arch/powerpc/kernel/rtasd.c|  2 +-
>  arch/powerpc/kernel/traps.c|  4 ++--
>  arch/powerpc/kvm/book3s_hv.c   | 10 --
>  arch/powerpc/kvm/book3s_xics.c |  2 +-
>  arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c |  2 +-
>  arch/powerpc/platforms/cell/spufs/inode.c  |  4 ++--
>  arch/powerpc/platforms/powernv/opal-dump.c |  4 ++--
>  arch/powerpc/platforms/powernv/opal-elog.c |  4 ++--
>  arch/powerpc/platforms/powernv/opal-sysparam.c |  6 +++---
>  arch/powerpc/platforms/pseries/cmm.c   | 16 
>  arch/powerpc/platforms/pseries/dlpar.c |  2 +-
>  arch/powerpc/platforms/pseries/hvCall_inst.c   |  2 +-
>  arch/powerpc/platforms/pseries/ibmebus.c   |  4 ++--
>  arch/powerpc/platforms/pseries/lparcfg.c   |  4 ++--
>  arch/powerpc/platforms/pseries/mobility.c  |  4 ++--
>  arch/powerpc/platforms/pseries/reconfig.c  |  2 +-
>  arch/powerpc/platforms/pseries/scanlog.c   |  2 +-
>  arch/powerpc/platforms/pseries/suspend.c   |  3 +--
>  arch/powerpc/platforms/pseries/vio.c   |  8 
>  arch/powerpc/sysdev/axonram.c  |  2 +-
>  arch/powerpc/sysdev/mv64x60_pci.c  |  2 +-
>  26 files changed, 54 insertions(+), 57 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index 1ceecdda810b..f6d2d5b66907 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -48,7 +48,7 @@ static ssize_t eeh_show_##_name(struct device *dev,  \
> \
>   return sprintf(buf, _format "\n", edev->_memb);   \
>  }\
> -static DEVICE_ATTR(_name, S_IRUGO, eeh_show_##_name, NULL);
> +static DEVICE_ATTR(_name, 0444, eeh_show_##_name, NULL);
>  
>  EEH_SHOW_ATTR(eeh_mode,mode,"0x%x");
>  EEH_SHOW_ATTR(eeh_config_addr, config_addr, "0x%x");
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index 5f202a566ec5..7ef279a0888e 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -127,7 +127,7 @@ static ssize_t fail_iommu_store(struct device *dev,
>   return count;
>  }
>  
> -static DEVICE_ATTR(fail_iommu, S_IRUGO|S_IWUSR, fail_iommu_show,
> +static DEVICE_ATTR(fail_iommu, 0644, fail_iommu_show,
>  fail_iommu_store);
>  
>  static int fail_iommu_bus_notify(struct notifier_block *nb,
> diff --git a/arch/powerpc/kernel/proc_powerpc.c 
> b/arch/powerpc/kernel/proc_powerpc.c
> index 56548bf6231f..9bfbd800d32f 100644
> --- a/arch/powerpc/kernel/proc_powerpc.c
> +++ b/arch/powerpc/kernel/proc_powerpc.c
> @@ -63,7 +63,7 @@ static int __init proc_ppc64_init(void)
>  {
>   struct proc_dir_entry *pde;
>  
> - pde = proc_create_data("powerpc/systemcfg", S_IFREG|S_IRUGO, NULL,
> + pde = proc_create_data("powerpc/systemcfg", S_IFREG | 0444, NULL,
>  _map_fops, vdso_data);
>   if (!pde)
>   return 1;
> diff --git a/arch/powerpc/kernel/rtas-proc.c b/arch/powerpc/kernel/rtas-proc.c
> index df56dfc4b681..bb5e8cbcc553 100644
> --- a/arch/powerpc/kernel/rtas-proc.c
> +++ b/arch/powerpc/kernel/rtas-proc.c
> @@ -260,19 +260,19 @@ static int __init proc_rtas_init(void)
>   if 

[PATCH] powerpc: Use octal numbers for file permissions

[Russell Currey]

Symbolic macros are unintuitive and hard to read, whereas octal constants
are much easier to interpret.  Replace macros for the basic permission
flags (user/group/other read/write/execute) with numeric constants
instead, across the whole powerpc tree.

Introducing a significant number of changes across the tree for no runtime
benefit isn't exactly desirable, but so long as these macros are still
used in the tree people will keep sending patches that add them.  Not only
are they hard to parse at a glance, there are multiple ways of coming to
the same value (as you can see with 0444 and 0644 in this patch) which
hurts readability.

Signed-off-by: Russell Currey 
---
I wondered what a "S_IRUGO" was and subsequently found the following:
https://lwn.net/Articles/696229/
so I figured making numeric constants standard across the tree was a good
idea instead of the mix we currently have.

For new patches that come in, checkpatch warns when using something like
S_IRUGO and tells you to use something like 0444 instead.
---
 arch/powerpc/kernel/eeh_sysfs.c|  2 +-
 arch/powerpc/kernel/iommu.c|  2 +-
 arch/powerpc/kernel/proc_powerpc.c |  2 +-
 arch/powerpc/kernel/rtas-proc.c| 14 +++---
 arch/powerpc/kernel/rtas_flash.c   |  2 +-
 arch/powerpc/kernel/rtasd.c|  2 +-
 arch/powerpc/kernel/traps.c|  4 ++--
 arch/powerpc/kvm/book3s_hv.c   | 10 --
 arch/powerpc/kvm/book3s_xics.c |  2 +-
 arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c |  2 +-
 arch/powerpc/platforms/cell/spufs/inode.c  |  4 ++--
 arch/powerpc/platforms/powernv/opal-dump.c |  4 ++--
 arch/powerpc/platforms/powernv/opal-elog.c |  4 ++--
 arch/powerpc/platforms/powernv/opal-sysparam.c |  6 +++---
 arch/powerpc/platforms/pseries/cmm.c   | 16 
 arch/powerpc/platforms/pseries/dlpar.c |  2 +-
 arch/powerpc/platforms/pseries/hvCall_inst.c   |  2 +-
 arch/powerpc/platforms/pseries/ibmebus.c   |  4 ++--
 arch/powerpc/platforms/pseries/lparcfg.c   |  4 ++--
 arch/powerpc/platforms/pseries/mobility.c  |  4 ++--
 arch/powerpc/platforms/pseries/reconfig.c  |  2 +-
 arch/powerpc/platforms/pseries/scanlog.c   |  2 +-
 arch/powerpc/platforms/pseries/suspend.c   |  3 +--
 arch/powerpc/platforms/pseries/vio.c   |  8 
 arch/powerpc/sysdev/axonram.c  |  2 +-
 arch/powerpc/sysdev/mv64x60_pci.c  |  2 +-
 26 files changed, 54 insertions(+), 57 deletions(-)

diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
index 1ceecdda810b..f6d2d5b66907 100644
--- a/arch/powerpc/kernel/eeh_sysfs.c
+++ b/arch/powerpc/kernel/eeh_sysfs.c
@@ -48,7 +48,7 @@ static ssize_t eeh_show_##_name(struct device *dev,  \
  \
return sprintf(buf, _format "\n", edev->_memb);   \
 }\
-static DEVICE_ATTR(_name, S_IRUGO, eeh_show_##_name, NULL);
+static DEVICE_ATTR(_name, 0444, eeh_show_##_name, NULL);
 
 EEH_SHOW_ATTR(eeh_mode,mode,"0x%x");
 EEH_SHOW_ATTR(eeh_config_addr, config_addr, "0x%x");
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 5f202a566ec5..7ef279a0888e 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -127,7 +127,7 @@ static ssize_t fail_iommu_store(struct device *dev,
return count;
 }
 
-static DEVICE_ATTR(fail_iommu, S_IRUGO|S_IWUSR, fail_iommu_show,
+static DEVICE_ATTR(fail_iommu, 0644, fail_iommu_show,
   fail_iommu_store);
 
 static int fail_iommu_bus_notify(struct notifier_block *nb,
diff --git a/arch/powerpc/kernel/proc_powerpc.c 
b/arch/powerpc/kernel/proc_powerpc.c
index 56548bf6231f..9bfbd800d32f 100644
--- a/arch/powerpc/kernel/proc_powerpc.c
+++ b/arch/powerpc/kernel/proc_powerpc.c
@@ -63,7 +63,7 @@ static int __init proc_ppc64_init(void)
 {
struct proc_dir_entry *pde;
 
-   pde = proc_create_data("powerpc/systemcfg", S_IFREG|S_IRUGO, NULL,
+   pde = proc_create_data("powerpc/systemcfg", S_IFREG | 0444, NULL,
   _map_fops, vdso_data);
if (!pde)
return 1;
diff --git a/arch/powerpc/kernel/rtas-proc.c b/arch/powerpc/kernel/rtas-proc.c
index df56dfc4b681..bb5e8cbcc553 100644
--- a/arch/powerpc/kernel/rtas-proc.c
+++ b/arch/powerpc/kernel/rtas-proc.c
@@ -260,19 +260,19 @@ static int __init proc_rtas_init(void)
if (rtas_node == NULL)
return -ENODEV;
 
-   proc_create("powerpc/rtas/progress", S_IRUGO|S_IWUSR, NULL,
+   proc_create("powerpc/rtas/progress", 0644, NULL,
_rtas_progress_operations);
-   proc_create("powerpc/rtas/clock", S_IRUGO|S_IWUSR, NULL,
+   proc_create("powerpc/rtas/clock", 0644, NULL,