2021-09-04 07:43:34

by Utkarsh Verma

[permalink] [raw]
Subject: [PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message

Add a new message type SYMBOLIC_PERMS under the 'Permissions'
subsection. Octal permission bits are easier to read and understand
instead of their symbolic macro names.

Suggested-by: Lukas Bulwahn <[email protected]>
Signed-off-by: Utkarsh Verma <[email protected]>
---
Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..01105e9c89de 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -957,6 +957,17 @@ Permissions
Permission bits should use 4 digit octal permissions (like 0700 or 0444).
Avoid using any other base like decimal.

+ **SYMBOLIC_PERMS**
+ Permission bits in the octal form are more readable and easier to
+ understand than their symbolic counterparts because many command-line
+ tools use this notation only. Experienced kernel developers have been using
+ this traditional Unix permission bits for decades and so they find it
+ easier to understand the octal notation than the symbolic macros.
+ Also, it is harder to read S_IWUSR|S_IRUGO than 0644, which obscures the
+ developer's intent rather than clarifying it.
+
+ See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
+

Spacing and Brackets
--------------------
--
2.25.1


2021-09-04 08:00:44

by Lukas Bulwahn

[permalink] [raw]
Subject: Re: [PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message

On Sat, Sep 4, 2021 at 9:30 AM Utkarsh Verma <[email protected]> wrote:
>
> Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> subsection. Octal permission bits are easier to read and understand
> instead of their symbolic macro names.
>
> Suggested-by: Lukas Bulwahn <[email protected]>
> Signed-off-by: Utkarsh Verma <[email protected]>
> ---
> Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..01105e9c89de 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -957,6 +957,17 @@ Permissions
> Permission bits should use 4 digit octal permissions (like 0700 or 0444).
> Avoid using any other base like decimal.
>
> + **SYMBOLIC_PERMS**
> + Permission bits in the octal form are more readable and easier to
> + understand than their symbolic counterparts because many command-line
> + tools use this notation only. Experienced kernel developers have been using
> + this traditional Unix permission bits for decades and so they find it
> + easier to understand the octal notation than the symbolic macros.
> + Also, it is harder to read S_IWUSR|S_IRUGO than 0644, which obscures the
> + developer's intent rather than clarifying it.

Just a quick stylistic nit:

s/Also, it is harder to read /For example, it is harder to read/

Other than that:

Acked-by: Lukas Bulwahn <[email protected]>
Reviewed-by: Lukas Bulwahn <[email protected]>

Feel free to send a quick v2 for my nitpicking, and apply the tags
from this email.

Lukas

> +
> + See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
> +
>
> Spacing and Brackets
> --------------------
> --
> 2.25.1
>

2021-09-04 08:45:52

by Utkarsh Verma

[permalink] [raw]
Subject: [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message

Add a new message type SYMBOLIC_PERMS under the 'Permissions'
subsection. Octal permission bits are easier to read and understand
instead of their symbolic macro names.

Suggested-by: Lukas Bulwahn <[email protected]>
Signed-off-by: Utkarsh Verma <[email protected]>
Acked-by: Lukas Bulwahn <[email protected]>
Reviewed-by: Lukas Bulwahn <[email protected]>
---
Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..41037594ec24 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -957,6 +957,17 @@ Permissions
Permission bits should use 4 digit octal permissions (like 0700 or 0444).
Avoid using any other base like decimal.

+ **SYMBOLIC_PERMS**
+ Permission bits in the octal form are more readable and easier to
+ understand than their symbolic counterparts because many command-line
+ tools use this notation only. Experienced kernel developers have been using
+ this traditional Unix permission bits for decades and so they find it
+ easier to understand the octal notation than the symbolic macros.
+ For example, it is harder to read S_IWUSR|S_IRUGO than 0644, which
+ obscures the developer's intent rather than clarifying it.
+
+ See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
+

Spacing and Brackets
--------------------
--
2.25.1

2021-09-09 17:05:06

by Dwaipayan Ray

[permalink] [raw]
Subject: Re: [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message

On Sat, Sep 4, 2021 at 1:53 PM Utkarsh Verma <[email protected]> wrote:
>
> Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> subsection. Octal permission bits are easier to read and understand
> instead of their symbolic macro names.
>
> Suggested-by: Lukas Bulwahn <[email protected]>
> Signed-off-by: Utkarsh Verma <[email protected]>
> Acked-by: Lukas Bulwahn <[email protected]>
> Reviewed-by: Lukas Bulwahn <[email protected]>
> ---
> Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..41037594ec24 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -957,6 +957,17 @@ Permissions
> Permission bits should use 4 digit octal permissions (like 0700 or 0444).
> Avoid using any other base like decimal.
>
> + **SYMBOLIC_PERMS**
> + Permission bits in the octal form are more readable and easier to
> + understand than their symbolic counterparts because many command-line
> + tools use this notation only. Experienced kernel developers have been using

Let's remove "only".

> + this traditional Unix permission bits for decades and so they find it

Maybe you meant "these" here.

With these changes made,
Acked-by: Dwaipayan Ray <[email protected]>

> + easier to understand the octal notation than the symbolic macros.
> + For example, it is harder to read S_IWUSR|S_IRUGO than 0644, which
> + obscures the developer's intent rather than clarifying it.
> +
> + See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
> +
>
> Spacing and Brackets
> --------------------
> --
> 2.25.1
>

2021-09-14 21:12:52

by Jonathan Corbet

[permalink] [raw]
Subject: Re: [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message

Dwaipayan Ray <[email protected]> writes:

> On Sat, Sep 4, 2021 at 1:53 PM Utkarsh Verma <[email protected]> wrote:
>>
>> Add a new message type SYMBOLIC_PERMS under the 'Permissions'
>> subsection. Octal permission bits are easier to read and understand
>> instead of their symbolic macro names.
>>
>> Suggested-by: Lukas Bulwahn <[email protected]>
>> Signed-off-by: Utkarsh Verma <[email protected]>
>> Acked-by: Lukas Bulwahn <[email protected]>
>> Reviewed-by: Lukas Bulwahn <[email protected]>
>> ---
>> Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
>> index f0956e9ea2d8..41037594ec24 100644
>> --- a/Documentation/dev-tools/checkpatch.rst
>> +++ b/Documentation/dev-tools/checkpatch.rst
>> @@ -957,6 +957,17 @@ Permissions
>> Permission bits should use 4 digit octal permissions (like 0700 or 0444).
>> Avoid using any other base like decimal.
>>
>> + **SYMBOLIC_PERMS**
>> + Permission bits in the octal form are more readable and easier to
>> + understand than their symbolic counterparts because many command-line
>> + tools use this notation only. Experienced kernel developers have been using
>
> Let's remove "only".
>
>> + this traditional Unix permission bits for decades and so they find it
>
> Maybe you meant "these" here.
>
> With these changes made,
> Acked-by: Dwaipayan Ray <[email protected]>

I took the liberty of apply the patch with those changes made.

Thanks,

jon

2021-09-14 22:30:44

by Dwaipayan Ray

[permalink] [raw]
Subject: Re: [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message

On Wed, Sep 15, 2021 at 2:40 AM Jonathan Corbet <[email protected]> wrote:
>
> Dwaipayan Ray <[email protected]> writes:
>
> > On Sat, Sep 4, 2021 at 1:53 PM Utkarsh Verma <[email protected]> wrote:
> >>
> >> Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> >> subsection. Octal permission bits are easier to read and understand
> >> instead of their symbolic macro names.
> >>
> >> Suggested-by: Lukas Bulwahn <[email protected]>
> >> Signed-off-by: Utkarsh Verma <[email protected]>
> >> Acked-by: Lukas Bulwahn <[email protected]>
> >> Reviewed-by: Lukas Bulwahn <[email protected]>
> >> ---
> >> Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
> >> 1 file changed, 11 insertions(+)
> >>
> >> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> >> index f0956e9ea2d8..41037594ec24 100644
> >> --- a/Documentation/dev-tools/checkpatch.rst
> >> +++ b/Documentation/dev-tools/checkpatch.rst
> >> @@ -957,6 +957,17 @@ Permissions
> >> Permission bits should use 4 digit octal permissions (like 0700 or 0444).
> >> Avoid using any other base like decimal.
> >>
> >> + **SYMBOLIC_PERMS**
> >> + Permission bits in the octal form are more readable and easier to
> >> + understand than their symbolic counterparts because many command-line
> >> + tools use this notation only. Experienced kernel developers have been using
> >
> > Let's remove "only".
> >
> >> + this traditional Unix permission bits for decades and so they find it
> >
> > Maybe you meant "these" here.
> >
> > With these changes made,
> > Acked-by: Dwaipayan Ray <[email protected]>
>
> I took the liberty of apply the patch with those changes made.
>

Thanks Jonathan.

Utkarsh, you can start working on your next patches after submitting, you don't
have to wait for the existing patches to be first accepted. They will follow
the same review -> changes -> review cycle until they are good for
acceptance.

Like lukas said, try preparing a batch of say 3 to 5 rules and let's
review it and get it in.

Thanks,
Dwaipayan.