staging: gasket: Replace symbolic permissions with octal permissions

PATCH In review Mamta Shukla 3 participants 5 messages Oct 6, 2018 – Oct 10, 2018 Staging

Share & tools

Mamta Shukla

author

2018-10-06 15:17 UTC #0 lore raw

PATCH 1/1 staging: gasket: Replace symbolic permissions with octal permissions

Use octal permissions in place of symbolic permissions to fix checkpatch warning.
WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.

Signed-off-by Mamta Shukla <[email protected]>

---
 drivers/staging/gasket/gasket_sysfs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/gasket/gasket_sysfs.h +2 -2

index f32eaf8..8b563d2 100644
@@ -40,7 +40,7 @@
  */
 #define GASKET_END_OF_ATTR_ARRAY                                               \
 	{                                                                      \
-		.attr = __ATTR(GASKET_ARRAY_END_TOKEN, S_IRUGO, NULL, NULL),   \
+		.attr = __ATTR(GASKET_ARRAY_END_TOKEN, 0444, NULL, NULL),   \
 		.data.attr_type = 0,                                           \
 	}

@@ -75,7 +75,7 @@ struct gasket_sysfs_attribute {
 
 #define GASKET_SYSFS_RO(_name, _show_function, _attr_type)                     \
 	{                                                                      \
-		.attr = __ATTR(_name, S_IRUGO, _show_function, NULL),          \
+		.attr = __ATTR(_name, 0444, _show_function, NULL),          \
 		.data.attr_type = _attr_type                                   \
 	}

Signature · 1 line

1.9.1

Himanshu Jha

Reviewed-by on v1 1/1

2018-10-06 17:52 UTC #1 lore raw

Quoted 6 lines · Mamta Shukla

Use octal permissions in place of symbolic permissions to fix checkpatch warning.
WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
Signed-off-by Mamta Shukla <[email protected]>

Reviewed-by Himanshu Jha <[email protected]>

In the meantime take a look here:
https://lkml.org/lkml/2016/8/2/1945

Quoted 35 lines

---
 drivers/staging/gasket/gasket_sysfs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/gasket/gasket_sysfs.h +2 -2

index f32eaf8..8b563d2 100644
@@ -40,7 +40,7 @@
  */
 #define GASKET_END_OF_ATTR_ARRAY                                               \
 	{                                                                      \
-		.attr = __ATTR(GASKET_ARRAY_END_TOKEN, S_IRUGO, NULL, NULL),   \
+		.attr = __ATTR(GASKET_ARRAY_END_TOKEN, 0444, NULL, NULL),   \
 		.data.attr_type = 0,                                           \
 	}

@@ -75,7 +75,7 @@ struct gasket_sysfs_attribute {
 
 #define GASKET_SYSFS_RO(_name, _show_function, _attr_type)                     \
 	{                                                                      \
-		.attr = __ATTR(_name, S_IRUGO, _show_function, NULL),          \
+		.attr = __ATTR(_name, 0444, _show_function, NULL),          \
 		.data.attr_type = _attr_type                                   \
 	}

Signature · 8 lines

1.9.1

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20181006143316.GA23864%40armorer.
For more options, visit https://groups.google.com/d/optout.

Signature · 4 lines

Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Greg KH

2018-10-09 12:55 UTC #2 lore raw

In reply to Mamta Shukla

Quoted 15 lines · Mamta Shukla

Use octal permissions in place of symbolic permissions to fix checkpatch warning.
WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
Signed-off-by Mamta Shukla <[email protected]>
---
 drivers/staging/gasket/gasket_sysfs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/gasket/gasket_sysfs.h +0 -0
index f32eaf8..8b563d2 100644

drivers/staging/gasket/gasket_sysfs.h line 43

Quoted 5 lines

drivers/staging/gasket/gasket_sysfs.h +1 -1

@@ -40,7 +40,7 @@
  */
 #define GASKET_END_OF_ATTR_ARRAY                                               \
 	{                                                                      \
-		.attr = __ATTR(GASKET_ARRAY_END_TOKEN, S_IRUGO, NULL, NULL),   \
+		.attr = __ATTR(GASKET_ARRAY_END_TOKEN, 0444, NULL, NULL),   \

This is very odd, why is this define even needed?

drivers/staging/gasket/gasket_sysfs.h struct gasket_sysfs_attribute { · line 78

Quoted 9 lines

drivers/staging/gasket/gasket_sysfs.h +1 -1

 		.data.attr_type = 0,                                           \
 	}

@@ -75,7 +75,7 @@ struct gasket_sysfs_attribute {
 
 #define GASKET_SYSFS_RO(_name, _show_function, _attr_type)                     \
 	{                                                                      \
-		.attr = __ATTR(_name, S_IRUGO, _show_function, NULL),          \
+		.attr = __ATTR(_name, 0444, _show_function, NULL),          \

This should use __ATTR_RO() instead, right?

thanks,

greg k-h

Mamta Shukla

author

2018-10-10 13:11 UTC #3 lore raw

Quoted 20 lines · Greg KH

Review
Quoted 11 lines · Mamta Shukla
Use octal permissions in place of symbolic permissions to fix checkpatch warning.
WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
Signed-off-by Mamta Shukla <[email protected]>
---
 drivers/staging/gasket/gasket_sysfs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/gasket/gasket_sysfs.h
drivers/staging/gasket/gasket_sysfs.h line 41
Quoted 6 lines
index f32eaf8..8b563d2 100644
drivers/staging/gasket/gasket_sysfs.h +0 -0
@@ -40,7 +40,7 @@
  */
 #define GASKET_END_OF_ATTR_ARRAY
       \

Quoted 9 lines

drivers/staging/gasket/gasket_sysfs.h line 42
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -0
      {
    \
drivers/staging/gasket/gasket_sysfs.h line 43
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -1
-             .attr = __ATTR(GASKET_ARRAY_END_TOKEN, S_IRUGO, NULL,
NULL), \
drivers/staging/gasket/gasket_sysfs.h line 43
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +1 -0
+             .attr = __ATTR(GASKET_ARRAY_END_TOKEN, 0444, NULL, NULL),
\
Quoted 1 line
This is very odd, why is this define even needed?

This gasket_sysfs.h and gasket_sysfs.c utility allows gasket driver framework to maintain their own set of sysfs entries. This was for terminating
gasket_sysfs_attr array.
Is this understanding correct?

Quoted 6 lines

Quoted 1 line

              .data.attr_type = 0,

Quoted 4 lines

      }
drivers/staging/gasket/gasket_sysfs.h +0 -0
@@ -75,7 +75,7 @@ struct gasket_sysfs_attribute {
 

Quoted 11 lines

drivers/staging/gasket/gasket_sysfs.h struct gasket_sysfs_attribute { · line 76
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -0
 #define GASKET_SYSFS_RO(_name, _show_function, _attr_type)
       \
drivers/staging/gasket/gasket_sysfs.h struct gasket_sysfs_attribute { · line 77
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -0
      {
    \
drivers/staging/gasket/gasket_sysfs.h struct gasket_sysfs_attribute { · line 78
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -1
-             .attr = __ATTR(_name, S_IRUGO, _show_function, NULL),
    \
drivers/staging/gasket/gasket_sysfs.h struct gasket_sysfs_attribute { · line 78
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +1 -0
+             .attr = __ATTR(_name, 0444, _show_function, NULL),
\
Quoted 1 line
This should use __ATTR_RO() instead, right?

Yes, this should be of Read only type attribute.This driver deals with 'raw' sysfs attributes.
Can I write a different patch for this?

Quoted 5 lines

Quoted 4 lines
thanks,
greg k-h

Signature · 1 line

Mamta Shukla.

Greg KH

2018-10-10 13:27 UTC #4 lore raw

Quoted 29 lines · Mamta Shukla

Quoted 27 lines · Greg KH
Review
Quoted 11 lines · Mamta Shukla
Use octal permissions in place of symbolic permissions to fix checkpatch warning.
WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
Signed-off-by Mamta Shukla <[email protected]>
---
 drivers/staging/gasket/gasket_sysfs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/gasket/gasket_sysfs.h
drivers/staging/gasket/gasket_sysfs.h line 41
Quoted 6 lines
index f32eaf8..8b563d2 100644
drivers/staging/gasket/gasket_sysfs.h +0 -0
@@ -40,7 +40,7 @@
  */
 #define GASKET_END_OF_ATTR_ARRAY
       \
drivers/staging/gasket/gasket_sysfs.h line 42
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -0
      {
    \
drivers/staging/gasket/gasket_sysfs.h line 43
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -1
-             .attr = __ATTR(GASKET_ARRAY_END_TOKEN, S_IRUGO, NULL,
NULL), \
drivers/staging/gasket/gasket_sysfs.h line 43
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +1 -0
+             .attr = __ATTR(GASKET_ARRAY_END_TOKEN, 0444, NULL, NULL),
\

Quoted 8 lines

Quoted 2 lines
Quoted 1 line
This is very odd, why is this define even needed?
This gasket_sysfs.h and gasket_sysfs.c utility allows gasket driver framework to maintain their own set of sysfs entries. This was for terminating
gasket_sysfs_attr array.
Is this understanding correct?

That seems correct. But the code should just be using a NULL attribute instead, no other subsystem has stuff like this. So the code should be changed to not need this. And really, the permission here is just pointless anyway...

Quoted 13 lines

Quoted 13 lines
Quoted 1 line
              .data.attr_type = 0,
     \
drivers/staging/gasket/gasket_sysfs.h struct gasket_sysfs_attribute { · line 76
Quoted 5 lines
      }
drivers/staging/gasket/gasket_sysfs.h +0 -0
@@ -75,7 +75,7 @@ struct gasket_sysfs_attribute {
 
 #define GASKET_SYSFS_RO(_name, _show_function, _attr_type)
       \
drivers/staging/gasket/gasket_sysfs.h struct gasket_sysfs_attribute { · line 77
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -0
      {
    \
drivers/staging/gasket/gasket_sysfs.h struct gasket_sysfs_attribute { · line 78
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +0 -1
-             .attr = __ATTR(_name, S_IRUGO, _show_function, NULL),
    \
Quoted 1 line
drivers/staging/gasket/gasket_sysfs.h +1 -0
+             .attr = __ATTR(_name, 0444, _show_function, NULL),

Quoted 8 lines

Quoted 4 lines
  \
Quoted 1 line
This should use __ATTR_RO() instead, right?
Yes, this should be of Read only type attribute.This driver deals with 'raw' sysfs attributes.
Can I write a different patch for this?

Yes, please just redo this patch and use __ATTR_RO() instead of this change.

thanks,

greg k-h