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

Groups > linux.kernel > #1573612 > unrolled thread

Re: Staging: speakup - syle fix permissions to octal

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Staging: speakup - syle fix permissions to octal Joe Perches <joe@perches.com> - 2017-02-04 08:30 +0100
    Re: Staging: speakup - syle fix permissions to octal Guenter Roeck <linux@roeck-us.net> - 2017-02-04 15:30 +0100
      Re: Staging: speakup - syle fix permissions to octal Julia Lawall <julia.lawall@lip6.fr> - 2017-02-04 15:30 +0100
        Re: Staging: speakup - syle fix permissions to octal Guenter Roeck <linux@roeck-us.net> - 2017-02-04 16:20 +0100
          Re: Staging: speakup - syle fix permissions to octal Julia Lawall <julia.lawall@lip6.fr> - 2017-02-04 16:30 +0100
    Re: Staging: speakup - syle fix permissions to octal Julia Lawall <julia.lawall@lip6.fr> - 2017-02-04 19:20 +0100
      Re: Staging: speakup - syle fix permissions to octal Joe Perches <joe@perches.com> - 2017-02-04 19:40 +0100
        Re: Staging: speakup - syle fix permissions to octal Julia Lawall <julia.lawall@lip6.fr> - 2017-02-04 20:30 +0100
          Re: Staging: speakup - syle fix permissions to octal Joe Perches <joe@perches.com> - 2017-02-04 20:40 +0100

#1573612 — Re: Staging: speakup - syle fix permissions to octal

(adding Julia Lawall)

On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
> On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
> > A style fix across whole driver.
> > changed permissions to octal style, found using checkpatch
> > 
> > Signed-off-by: Derek Robson <robsonde@gmail.com>
> 
> FWIW, I think changes like this are best done using coccinelle.

I think checkpatch does it reasonably well.

Julia?  Can coccinelle do this?

I believe cocinelle doesn't handle the substitution
and octal addition very well when multiple flags
are used.

> That ensures that the results can be reproduced and are well defined.
> As it is, someone will have to check each line of your patches to ensure
> that the conversion is correct.
> 
> It would also ensure (hopefully) that we don't end up with constructs
> such as 
> 
> > -#define USER_R (S_IFREG|S_IRUGO)
> > -#define USER_W (S_IFREG|S_IWUGO)
> > +#define USER_R (S_IFREG|0444)
> > +#define USER_W (S_IFREG|0666)
> 
> which really defeat the purpose of the whole exercise.

Why do you think mixing file specific attributes
with octal permissions is a bad thing?

$ git log -1 f90774e1fd2700d
commit f90774e1fd2700de4a6e0d62866d34a26c544bd0
Author: Joe Perches <joe@perches.com>
Date:   Tue Oct 11 13:51:47 2016 -0700

    checkpatch: look for symbolic permissions and suggest octal instead
    
    S_<FOO> uses should be avoided where octal is more intelligible.
    
    Linus didst say:
    
    : It's *much* easier to parse and understand the octal numbers, while the
    : symbolic macro names are just random line noise and hard as hell to
    : understand.  You really have to think about it.
    :
    : So we should rather go the other way: convert existing bad symbolic
    : permission bit macro use to just use the octal numbers.
    :
    : The symbolic names are good for the *other* bits (ie sticky bit, and the
    : inode mode _type_ numbers etc), but for the permission bits, the symbolic
    : names are just insane crap.  Nobody sane should ever use them.  Not in the
    : kernel, not in user space.
    (http://lkml.kernel.org/r/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com)
    
    Link: http://lkml.kernel.org/r/7232ef011d05a92f4caa86a5e9830d87966a2eaf.1470180926.git.joe@perches.com
    Signed-off-by: Joe Perches <joe@perches.com>
    Cc: Linus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

[toc] | [next] | [standalone]

#1573674

On 02/03/2017 11:27 PM, Joe Perches wrote:
> (adding Julia Lawall)
>
> On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
>> On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
>>> A style fix across whole driver.
>>> changed permissions to octal style, found using checkpatch
>>>
>>> Signed-off-by: Derek Robson <robsonde@gmail.com>
>>
>> FWIW, I think changes like this are best done using coccinelle.
>
> I think checkpatch does it reasonably well.
>
> Julia?  Can coccinelle do this?
>
> I believe cocinelle doesn't handle the substitution
> and octal addition very well when multiple flags
> are used.
>

Why not ? Seems to be quite simple. One just has to list all the variants
being used in the rule.

>> That ensures that the results can be reproduced and are well defined.
>> As it is, someone will have to check each line of your patches to ensure
>> that the conversion is correct.
>>
>> It would also ensure (hopefully) that we don't end up with constructs
>> such as
>>
>>> -#define USER_R (S_IFREG|S_IRUGO)
>>> -#define USER_W (S_IFREG|S_IWUGO)
>>> +#define USER_R (S_IFREG|0444)
>>> +#define USER_W (S_IFREG|0666)
>>
>> which really defeat the purpose of the whole exercise.
>
> Why do you think mixing file specific attributes
> with octal permissions is a bad thing?
>

Just an assumption. My bad. Ultimately, what I think doesn't really
matter, though - because what I think is that the whole "use octals"
is a bad idea to start with.

Guenter

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

#1573675

On Sat, 4 Feb 2017, Guenter Roeck wrote:

> On 02/03/2017 11:27 PM, Joe Perches wrote:
> > (adding Julia Lawall)
> >
> > On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
> > > On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
> > > > A style fix across whole driver.
> > > > changed permissions to octal style, found using checkpatch
> > > >
> > > > Signed-off-by: Derek Robson <robsonde@gmail.com>
> > >
> > > FWIW, I think changes like this are best done using coccinelle.
> >
> > I think checkpatch does it reasonably well.
> >
> > Julia?  Can coccinelle do this?
> >
> > I believe cocinelle doesn't handle the substitution
> > and octal addition very well when multiple flags
> > are used.
> >
>
> Why not ? Seems to be quite simple. One just has to list all the variants
> being used in the rule.
>
> > > That ensures that the results can be reproduced and are well defined.
> > > As it is, someone will have to check each line of your patches to ensure
> > > that the conversion is correct.
> > >
> > > It would also ensure (hopefully) that we don't end up with constructs
> > > such as
> > >
> > > > -#define USER_R (S_IFREG|S_IRUGO)
> > > > -#define USER_W (S_IFREG|S_IWUGO)
> > > > +#define USER_R (S_IFREG|0444)
> > > > +#define USER_W (S_IFREG|0666)
> > >
> > > which really defeat the purpose of the whole exercise.
> >
> > Why do you think mixing file specific attributes
> > with octal permissions is a bad thing?
> >
>
> Just an assumption. My bad. Ultimately, what I think doesn't really
> matter, though - because what I think is that the whole "use octals"
> is a bad idea to start with.

I don't think I have received yet the message that this is referring to.
But I don't see a problem for Coccinelle a priori.  If there are things
that need to be added together, as long as they are explicit constants,
that can be done in python or ocaml.

julia

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

#1573683

On 02/04/2017 06:29 AM, Julia Lawall wrote:
>
>
> On Sat, 4 Feb 2017, Guenter Roeck wrote:
>
>> On 02/03/2017 11:27 PM, Joe Perches wrote:
>>> (adding Julia Lawall)
>>>
>>> On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
>>>> On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
>>>>> A style fix across whole driver.
>>>>> changed permissions to octal style, found using checkpatch
>>>>>
>>>>> Signed-off-by: Derek Robson <robsonde@gmail.com>
>>>>
>>>> FWIW, I think changes like this are best done using coccinelle.
>>>
>>> I think checkpatch does it reasonably well.
>>>
>>> Julia?  Can coccinelle do this?
>>>
>>> I believe cocinelle doesn't handle the substitution
>>> and octal addition very well when multiple flags
>>> are used.
>>>
>>
>> Why not ? Seems to be quite simple. One just has to list all the variants
>> being used in the rule.
>>
>>>> That ensures that the results can be reproduced and are well defined.
>>>> As it is, someone will have to check each line of your patches to ensure
>>>> that the conversion is correct.
>>>>
>>>> It would also ensure (hopefully) that we don't end up with constructs
>>>> such as
>>>>
>>>>> -#define USER_R (S_IFREG|S_IRUGO)
>>>>> -#define USER_W (S_IFREG|S_IWUGO)
>>>>> +#define USER_R (S_IFREG|0444)
>>>>> +#define USER_W (S_IFREG|0666)
>>>>
>>>> which really defeat the purpose of the whole exercise.
>>>
>>> Why do you think mixing file specific attributes
>>> with octal permissions is a bad thing?
>>>
>>
>> Just an assumption. My bad. Ultimately, what I think doesn't really
>> matter, though - because what I think is that the whole "use octals"
>> is a bad idea to start with.
>
> I don't think I have received yet the message that this is referring to.
> But I don't see a problem for Coccinelle a priori.  If there are things
> that need to be added together, as long as they are explicit constants,
> that can be done in python or ocaml.
>

Something like

@@
@@

(
- S_IFREG | S_IRUGO | S_IWUGO
+ S_IFREG | 0666
|
- S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
+ 0644
|
- S_IRUGO|S_IWUSR
+ 0644
|
- S_IWUSR|S_IRUGO
+ 0644
|
- S_IRUGO|S_IWUGO
+ 0666
|
- S_IWUGO|S_IRUGO
+ 0666
|
- S_IRUGO
+ 0444
|
- S_IWUGO
+ 0222
|
- S_IWUSR
+ 0200
)

Odd is that the S_IFREG rule seems to be needed to catch "S_IFREG | S_IRUGO | S_IWUGO",
but probably I am missing something as usual ;-).

Guenter

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

#1573684

On Sat, 4 Feb 2017, Guenter Roeck wrote:

> On 02/04/2017 06:29 AM, Julia Lawall wrote:
> >
> >
> > On Sat, 4 Feb 2017, Guenter Roeck wrote:
> >
> > > On 02/03/2017 11:27 PM, Joe Perches wrote:
> > > > (adding Julia Lawall)
> > > >
> > > > On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
> > > > > On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
> > > > > > A style fix across whole driver.
> > > > > > changed permissions to octal style, found using checkpatch
> > > > > >
> > > > > > Signed-off-by: Derek Robson <robsonde@gmail.com>
> > > > >
> > > > > FWIW, I think changes like this are best done using coccinelle.
> > > >
> > > > I think checkpatch does it reasonably well.
> > > >
> > > > Julia?  Can coccinelle do this?
> > > >
> > > > I believe cocinelle doesn't handle the substitution
> > > > and octal addition very well when multiple flags
> > > > are used.
> > > >
> > >
> > > Why not ? Seems to be quite simple. One just has to list all the variants
> > > being used in the rule.
> > >
> > > > > That ensures that the results can be reproduced and are well defined.
> > > > > As it is, someone will have to check each line of your patches to
> > > > > ensure
> > > > > that the conversion is correct.
> > > > >
> > > > > It would also ensure (hopefully) that we don't end up with constructs
> > > > > such as
> > > > >
> > > > > > -#define USER_R (S_IFREG|S_IRUGO)
> > > > > > -#define USER_W (S_IFREG|S_IWUGO)
> > > > > > +#define USER_R (S_IFREG|0444)
> > > > > > +#define USER_W (S_IFREG|0666)
> > > > >
> > > > > which really defeat the purpose of the whole exercise.
> > > >
> > > > Why do you think mixing file specific attributes
> > > > with octal permissions is a bad thing?
> > > >
> > >
> > > Just an assumption. My bad. Ultimately, what I think doesn't really
> > > matter, though - because what I think is that the whole "use octals"
> > > is a bad idea to start with.
> >
> > I don't think I have received yet the message that this is referring to.
> > But I don't see a problem for Coccinelle a priori.  If there are things
> > that need to be added together, as long as they are explicit constants,
> > that can be done in python or ocaml.
> >
>
> Something like
>
> @@
> @@
>
> (
> - S_IFREG | S_IRUGO | S_IWUGO
> + S_IFREG | 0666
> |
> - S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
> + 0644
> |
> - S_IRUGO|S_IWUSR
> + 0644
> |
> - S_IWUSR|S_IRUGO
> + 0644
> |
> - S_IRUGO|S_IWUGO
> + 0666
> |
> - S_IWUGO|S_IRUGO
> + 0666
> |
> - S_IRUGO
> + 0444
> |
> - S_IWUGO
> + 0222
> |
> - S_IWUSR
> + 0200
> )
>
> Odd is that the S_IFREG rule seems to be needed to catch "S_IFREG | S_IRUGO |
> S_IWUGO",
> but probably I am missing something as usual ;-).

The associativity of | is not what you hope for.

julia

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

#1573727

On Fri, 3 Feb 2017, Joe Perches wrote:

> (adding Julia Lawall)
>
> On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
> > On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
> > > A style fix across whole driver.
> > > changed permissions to octal style, found using checkpatch
> > >
> > > Signed-off-by: Derek Robson <robsonde@gmail.com>
> >
> > FWIW, I think changes like this are best done using coccinelle.
>
> I think checkpatch does it reasonably well.
>
> Julia?  Can coccinelle do this?
>
> I believe cocinelle doesn't handle the substitution
> and octal addition very well when multiple flags
> are used.

OK, finally received.  I think that Guenter's solution is a good one,
because the rule-writer knows better than Coccinelle what people find to
be intelligible.

julia

>
> > That ensures that the results can be reproduced and are well defined.
> > As it is, someone will have to check each line of your patches to ensure
> > that the conversion is correct.
> >
> > It would also ensure (hopefully) that we don't end up with constructs
> > such as
> >
> > > -#define USER_R (S_IFREG|S_IRUGO)
> > > -#define USER_W (S_IFREG|S_IWUGO)
> > > +#define USER_R (S_IFREG|0444)
> > > +#define USER_W (S_IFREG|0666)
> >
> > which really defeat the purpose of the whole exercise.
>
> Why do you think mixing file specific attributes
> with octal permissions is a bad thing?
>
> $ git log -1 f90774e1fd2700d
> commit f90774e1fd2700de4a6e0d62866d34a26c544bd0
> Author: Joe Perches <joe@perches.com>
> Date:   Tue Oct 11 13:51:47 2016 -0700
>
>     checkpatch: look for symbolic permissions and suggest octal instead
>
>     S_<FOO> uses should be avoided where octal is more intelligible.
>
>     Linus didst say:
>
>     : It's *much* easier to parse and understand the octal numbers, while the
>     : symbolic macro names are just random line noise and hard as hell to
>     : understand.  You really have to think about it.
>     :
>     : So we should rather go the other way: convert existing bad symbolic
>     : permission bit macro use to just use the octal numbers.
>     :
>     : The symbolic names are good for the *other* bits (ie sticky bit, and the
>     : inode mode _type_ numbers etc), but for the permission bits, the symbolic
>     : names are just insane crap.  Nobody sane should ever use them.  Not in the
>     : kernel, not in user space.
>     (http://lkml.kernel.org/r/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com)
>
>     Link: http://lkml.kernel.org/r/7232ef011d05a92f4caa86a5e9830d87966a2eaf.1470180926.git.joe@perches.com
>     Signed-off-by: Joe Perches <joe@perches.com>
>     Cc: Linus Torvalds <torvalds@linux-foundation.org>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>

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

#1573731

On Sat, 2017-02-04 at 19:10 +0100, Julia Lawall wrote:
> 
> On Fri, 3 Feb 2017, Joe Perches wrote:
> 
> > (adding Julia Lawall)
> > 
> > On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
> > > On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
> > > > A style fix across whole driver.
> > > > changed permissions to octal style, found using checkpatch
> > > > 
> > > > Signed-off-by: Derek Robson <robsonde@gmail.com>
> > > 
> > > FWIW, I think changes like this are best done using coccinelle.
> > 
> > I think checkpatch does it reasonably well.
> > 
> > Julia?  Can coccinelle do this?
> > 
> > I believe cocinelle doesn't handle the substitution
> > and octal addition very well when multiple flags
> > are used.
> 
> OK, finally received.  I think that Guenter's solution is a good one,
> because the rule-writer knows better than Coccinelle what people find to
> be intelligible.

If it's written in python, I don't see how that's
particularly different than being written in perl,
but coccinelle could certainly handle expressions
on multiple lines of these or'd constants better.

Are there any than span 2 or more lines?
I didn't see one with a casual grep.

Does coccinelle handle arbitrary ordering of these
constants in a form like Guenter suggests or does
each possible ordered sequence need to be written?

just fyi:

"S_IRWXU" => 0700
"S_IRUSR" => 0400
"S_IWUSR" => 0200
"S_IXUSR" => 0100
"S_IRWXG" => 0070
"S_IRGRP" => 0040
"S_IWGRP" => 0020
"S_IXGRP" => 0010
"S_IRWXO" => 0007
"S_IROTH" => 0004
"S_IWOTH" => 0002
"S_IXOTH" => 0001
"S_IRWXUGO" => 0777
"S_IRUGO" => 0444
"S_IWUGO" => 0222
"S_IXUGO" => 0111

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

#1573737

On Sat, 4 Feb 2017, Joe Perches wrote:

> On Sat, 2017-02-04 at 19:10 +0100, Julia Lawall wrote:
> >
> > On Fri, 3 Feb 2017, Joe Perches wrote:
> >
> > > (adding Julia Lawall)
> > >
> > > On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
> > > > On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
> > > > > A style fix across whole driver.
> > > > > changed permissions to octal style, found using checkpatch
> > > > >
> > > > > Signed-off-by: Derek Robson <robsonde@gmail.com>
> > > >
> > > > FWIW, I think changes like this are best done using coccinelle.
> > >
> > > I think checkpatch does it reasonably well.
> > >
> > > Julia?  Can coccinelle do this?
> > >
> > > I believe cocinelle doesn't handle the substitution
> > > and octal addition very well when multiple flags
> > > are used.
> >
> > OK, finally received.  I think that Guenter's solution is a good one,
> > because the rule-writer knows better than Coccinelle what people find to
> > be intelligible.
>
> If it's written in python, I don't see how that's
> particularly different than being written in perl,
> but coccinelle could certainly handle expressions
> on multiple lines of these or'd constants better.

Well, the python would only be needed if there was a need for actual
calculations.  Should S_IRGRP | S_IWGRP become 0060?

>
> Are there any than span 2 or more lines?
> I didn't see one with a casual grep.
>
> Does coccinelle handle arbitrary ordering of these
> constants in a form like Guenter suggests or does
> each possible ordered sequence need to be written?

There is the following isomorphism:

X | Y => Y | X

But that only works when the associativity is right.

A | B | C is (A | B) | C, so the B and C won't exchange with each other.
One can also write eg

A | ... | B

which gives more fexibility.  I'm not sure to what extent that works when
there are changes, though.

julia

> just fyi:
>
> "S_IRWXU" => 0700
> "S_IRUSR" => 0400
> "S_IWUSR" => 0200
> "S_IXUSR" => 0100
> "S_IRWXG" => 0070
> "S_IRGRP" => 0040
> "S_IWGRP" => 0020
> "S_IXGRP" => 0010
> "S_IRWXO" => 0007
> "S_IROTH" => 0004
> "S_IWOTH" => 0002
> "S_IXOTH" => 0001
> "S_IRWXUGO" => 0777
> "S_IRUGO" => 0444
> "S_IWUGO" => 0222
> "S_IXUGO" => 0111

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

#1573738

On Sat, 2017-02-04 at 20:24 +0100, Julia Lawall wrote:
> 
> On Sat, 4 Feb 2017, Joe Perches wrote:
> 
> > On Sat, 2017-02-04 at 19:10 +0100, Julia Lawall wrote:
> > > 
> > > On Fri, 3 Feb 2017, Joe Perches wrote:
> > > 
> > > > (adding Julia Lawall)
> > > > 
> > > > On Fri, 2017-02-03 at 20:44 -0800, Guenter Roeck wrote:
> > > > > On Sat, Jan 28, 2017 at 07:05:09PM +1300, Derek Robson wrote:
> > > > > > A style fix across whole driver.
> > > > > > changed permissions to octal style, found using checkpatch
> > > > > > 
> > > > > > Signed-off-by: Derek Robson <robsonde@gmail.com>
> > > > > 
> > > > > FWIW, I think changes like this are best done using coccinelle.
> > > > 
> > > > I think checkpatch does it reasonably well.
> > > > 
> > > > Julia?  Can coccinelle do this?
> > > > 
> > > > I believe cocinelle doesn't handle the substitution
> > > > and octal addition very well when multiple flags
> > > > are used.
> > > 
> > > OK, finally received.  I think that Guenter's solution is a good one,
> > > because the rule-writer knows better than Coccinelle what people find to
> > > be intelligible.
> > 
> > If it's written in python, I don't see how that's
> > particularly different than being written in perl,
> > but coccinelle could certainly handle expressions
> > on multiple lines of these or'd constants better.
> 
> Well, the python would only be needed if there was a need for actual
> calculations.  Should S_IRGRP | S_IWGRP become 0060?

Yes.

> > Are there any than span 2 or more lines?
> > I didn't see one with a casual grep.
> > 
> > Does coccinelle handle arbitrary ordering of these
> > constants in a form like Guenter suggests or does
> > each possible ordered sequence need to be written?
> 
> There is the following isomorphism:
> 
> X | Y => Y | X
> 
> But that only works when the associativity is right.
> 
> A | B | C is (A | B) | C, so the B and C won't exchange with each other.
> One can also write eg
> 
> A | ... | B
> 
> which gives more fexibility.  I'm not sure to what extent that works when
> there are changes, though.

The perl code doesn't care about the ordering
but it doesn't handle the A | ... | B case.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web