All of lore.kernel.org
 help / color / mirror / Atom feed
* [TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions
@ 2016-11-22  3:03 Walt Feasel
  2016-11-26 11:05 ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Walt Feasel @ 2016-11-22  3:03 UTC (permalink / raw)
  To: kernelnewbies

Make suggested checkpatch modifications for
WARNING: Symbolic permissions 'S_IWUSR | S_IRUGO' are not preferred.
Consider using octal permissions '0644'.
WARNING: Symbolic permissions 'S_IRUGO' are not preferred.
Consider using octal permissions '0444'.

Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
---
I am new to making trivial patches and do not make some
for a few type of warnings.
This is one of them as I am not fully certain that it is
as easy as this.
The replacing of 'S_IWUSR | S_IRUGO' with '0644' seems
simple enough.
However the adding of '_RW' to '__ATTR' to make '__ATTR_RW'
I saw in a reply to a patch and am not sure that it would
be relevant in this case.
I also made a previous patch adding spaces around '|' and
want to know if just replacing 'S_IWUSR|S_IRUGO' with
'0644' in one shot would be acceptable since my being new
and not fixing just one type of warning per patch.
Seems straight forward but I have spammed other peoples
email and the mailing list enough with improper patches.

 drivers/staging/speakup/speakup_acntpc.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/speakup/speakup_acntpc.c b/drivers/staging/speakup/speakup_acntpc.c
index 27f812e..51281be 100644
--- a/drivers/staging/speakup/speakup_acntpc.c
+++ b/drivers/staging/speakup/speakup_acntpc.c
@@ -56,28 +56,28 @@ static struct var_t vars[] = {
 /* These attributes will appear in /sys/accessibility/speakup/acntpc. */
 
 static struct kobj_attribute caps_start_attribute =
-	__ATTR(caps_start, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(caps_start, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute caps_stop_attribute =
-	__ATTR(caps_stop, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(caps_stop, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute pitch_attribute =
-	__ATTR(pitch, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(pitch, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute rate_attribute =
-	__ATTR(rate, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(rate, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute tone_attribute =
-	__ATTR(tone, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(tone, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute vol_attribute =
-	__ATTR(vol, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(vol, 0644, spk_var_show, spk_var_store);
 
 static struct kobj_attribute delay_time_attribute =
-	__ATTR(delay_time, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(delay_time, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute direct_attribute =
-	__ATTR(direct, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(direct, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute full_time_attribute =
-	__ATTR(full_time, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(full_time, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute jiffy_delta_attribute =
-	__ATTR(jiffy_delta, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(jiffy_delta, 0644, spk_var_show, spk_var_store);
 static struct kobj_attribute trigger_time_attribute =
-	__ATTR(trigger_time, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
+	__ATTR_RW(trigger_time, 0644, spk_var_show, spk_var_store);
 
 /*
  * Create a group of attributes so that we can create and destroy them all
@@ -306,8 +306,8 @@ static void accent_release(void)
 	speakup_info.port_tts = 0;
 }
 
-module_param_named(port, port_forced, int, S_IRUGO);
-module_param_named(start, synth_acntpc.startup, short, S_IRUGO);
+module_param_named(port, port_forced, int, 0444);
+module_param_named(start, synth_acntpc.startup, short, 0444);
 
 MODULE_PARM_DESC(port, "Set the port for the synthesizer (override probing).");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions
  2016-11-22  3:03 [TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions Walt Feasel
@ 2016-11-26 11:05 ` Greg KH
  2016-11-26 17:10   ` Walt Feasel
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2016-11-26 11:05 UTC (permalink / raw)
  To: kernelnewbies

On Mon, Nov 21, 2016 at 10:03:00PM -0500, Walt Feasel wrote:
> Make suggested checkpatch modifications for
> WARNING: Symbolic permissions 'S_IWUSR | S_IRUGO' are not preferred.
> Consider using octal permissions '0644'.
> WARNING: Symbolic permissions 'S_IRUGO' are not preferred.
> Consider using octal permissions '0444'.
> 
> Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
> ---
> I am new to making trivial patches and do not make some
> for a few type of warnings.
> This is one of them as I am not fully certain that it is
> as easy as this.
> The replacing of 'S_IWUSR | S_IRUGO' with '0644' seems
> simple enough.
> However the adding of '_RW' to '__ATTR' to make '__ATTR_RW'
> I saw in a reply to a patch and am not sure that it would
> be relevant in this case.
> I also made a previous patch adding spaces around '|' and
> want to know if just replacing 'S_IWUSR|S_IRUGO' with
> '0644' in one shot would be acceptable since my being new
> and not fixing just one type of warning per patch.
> Seems straight forward but I have spammed other peoples
> email and the mailing list enough with improper patches.
> 
>  drivers/staging/speakup/speakup_acntpc.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/speakup/speakup_acntpc.c b/drivers/staging/speakup/speakup_acntpc.c
> index 27f812e..51281be 100644
> --- a/drivers/staging/speakup/speakup_acntpc.c
> +++ b/drivers/staging/speakup/speakup_acntpc.c
> @@ -56,28 +56,28 @@ static struct var_t vars[] = {
>  /* These attributes will appear in /sys/accessibility/speakup/acntpc. */
>  
>  static struct kobj_attribute caps_start_attribute =
> -	__ATTR(caps_start, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
> +	__ATTR_RW(caps_start, 0644, spk_var_show, spk_var_store);

This breaks the build :(

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions
  2016-11-26 11:05 ` Greg KH
@ 2016-11-26 17:10   ` Walt Feasel
  2016-11-26 17:18     ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Walt Feasel @ 2016-11-26 17:10 UTC (permalink / raw)
  To: kernelnewbies

On Sat, Nov 26, 2016 at 12:05:13PM +0100, Greg KH wrote:
> On Mon, Nov 21, 2016 at 10:03:00PM -0500, Walt Feasel wrote:
> > Make suggested checkpatch modifications for
> > WARNING: Symbolic permissions 'S_IWUSR | S_IRUGO' are not preferred.
> > Consider using octal permissions '0644'.
> > WARNING: Symbolic permissions 'S_IRUGO' are not preferred.
> > Consider using octal permissions '0444'.
> > 
> > Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
> > ---
> > I am new to making trivial patches and do not make some
> > for a few type of warnings.
> > This is one of them as I am not fully certain that it is
> > as easy as this.
> > The replacing of 'S_IWUSR | S_IRUGO' with '0644' seems
> > simple enough.
> > However the adding of '_RW' to '__ATTR' to make '__ATTR_RW'
> > I saw in a reply to a patch and am not sure that it would
> > be relevant in this case.
> > I also made a previous patch adding spaces around '|' and
> > want to know if just replacing 'S_IWUSR|S_IRUGO' with
> > '0644' in one shot would be acceptable since my being new
> > and not fixing just one type of warning per patch.
> > Seems straight forward but I have spammed other peoples
> > email and the mailing list enough with improper patches.
> > 
> >  drivers/staging/speakup/speakup_acntpc.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/staging/speakup/speakup_acntpc.c b/drivers/staging/speakup/speakup_acntpc.c
> > index 27f812e..51281be 100644
> > --- a/drivers/staging/speakup/speakup_acntpc.c
> > +++ b/drivers/staging/speakup/speakup_acntpc.c
> > @@ -56,28 +56,28 @@ static struct var_t vars[] = {
> >  /* These attributes will appear in /sys/accessibility/speakup/acntpc. */
> >  
> >  static struct kobj_attribute caps_start_attribute =
> > -	__ATTR(caps_start, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
> > +	__ATTR_RW(caps_start, 0644, spk_var_show, spk_var_store);
> 
> This breaks the build :(
> 
I would assume from adding _RW as is didnt seem to be needed but
only a visual reference to the octal's permissions without
doing the math. I will look deeper into where __ATTR is
probally being called then. I'm not really worried with builds
yet till I have a better understanging of the issue as I would
expect all of them to fail at this point.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions
  2016-11-26 17:10   ` Walt Feasel
@ 2016-11-26 17:18     ` Greg KH
  2016-11-26 18:11       ` Walt Feasel
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2016-11-26 17:18 UTC (permalink / raw)
  To: kernelnewbies

On Sat, Nov 26, 2016 at 12:10:28PM -0500, Walt Feasel wrote:
> On Sat, Nov 26, 2016 at 12:05:13PM +0100, Greg KH wrote:
> > On Mon, Nov 21, 2016 at 10:03:00PM -0500, Walt Feasel wrote:
> > > Make suggested checkpatch modifications for
> > > WARNING: Symbolic permissions 'S_IWUSR | S_IRUGO' are not preferred.
> > > Consider using octal permissions '0644'.
> > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred.
> > > Consider using octal permissions '0444'.
> > > 
> > > Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
> > > ---
> > > I am new to making trivial patches and do not make some
> > > for a few type of warnings.
> > > This is one of them as I am not fully certain that it is
> > > as easy as this.
> > > The replacing of 'S_IWUSR | S_IRUGO' with '0644' seems
> > > simple enough.
> > > However the adding of '_RW' to '__ATTR' to make '__ATTR_RW'
> > > I saw in a reply to a patch and am not sure that it would
> > > be relevant in this case.
> > > I also made a previous patch adding spaces around '|' and
> > > want to know if just replacing 'S_IWUSR|S_IRUGO' with
> > > '0644' in one shot would be acceptable since my being new
> > > and not fixing just one type of warning per patch.
> > > Seems straight forward but I have spammed other peoples
> > > email and the mailing list enough with improper patches.
> > > 
> > >  drivers/staging/speakup/speakup_acntpc.c | 26 +++++++++++++-------------
> > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/staging/speakup/speakup_acntpc.c b/drivers/staging/speakup/speakup_acntpc.c
> > > index 27f812e..51281be 100644
> > > --- a/drivers/staging/speakup/speakup_acntpc.c
> > > +++ b/drivers/staging/speakup/speakup_acntpc.c
> > > @@ -56,28 +56,28 @@ static struct var_t vars[] = {
> > >  /* These attributes will appear in /sys/accessibility/speakup/acntpc. */
> > >  
> > >  static struct kobj_attribute caps_start_attribute =
> > > -	__ATTR(caps_start, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
> > > +	__ATTR_RW(caps_start, 0644, spk_var_show, spk_var_store);
> > 
> > This breaks the build :(
> > 
> I would assume from adding _RW as is didnt seem to be needed but
> only a visual reference to the octal's permissions without
> doing the math. I will look deeper into where __ATTR is
> probally being called then. I'm not really worried with builds
> yet till I have a better understanging of the issue as I would
> expect all of them to fail at this point.

Never send out a patch that you have not at least test-built, unless you
say you have not done so (and even then, it's a rare thing to do.)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions
  2016-11-26 17:18     ` Greg KH
@ 2016-11-26 18:11       ` Walt Feasel
  2016-11-26 20:40         ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Walt Feasel @ 2016-11-26 18:11 UTC (permalink / raw)
  To: kernelnewbies

On Sat, Nov 26, 2016 at 06:18:40PM +0100, Greg KH wrote:
> On Sat, Nov 26, 2016 at 12:10:28PM -0500, Walt Feasel wrote:
> > On Sat, Nov 26, 2016 at 12:05:13PM +0100, Greg KH wrote:
> > > On Mon, Nov 21, 2016 at 10:03:00PM -0500, Walt Feasel wrote:
> > > > Make suggested checkpatch modifications for
> > > > WARNING: Symbolic permissions 'S_IWUSR | S_IRUGO' are not preferred.
> > > > Consider using octal permissions '0644'.
> > > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred.
> > > > Consider using octal permissions '0444'.
> > > > 
> > > > Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
> > > > ---
> > > > I am new to making trivial patches and do not make some
> > > > for a few type of warnings.
> > > > This is one of them as I am not fully certain that it is
> > > > as easy as this.
> > > > The replacing of 'S_IWUSR | S_IRUGO' with '0644' seems
> > > > simple enough.
> > > > However the adding of '_RW' to '__ATTR' to make '__ATTR_RW'
> > > > I saw in a reply to a patch and am not sure that it would
> > > > be relevant in this case.
> > > > I also made a previous patch adding spaces around '|' and
> > > > want to know if just replacing 'S_IWUSR|S_IRUGO' with
> > > > '0644' in one shot would be acceptable since my being new
> > > > and not fixing just one type of warning per patch.
> > > > Seems straight forward but I have spammed other peoples
> > > > email and the mailing list enough with improper patches.
> > > > 
> > > >  drivers/staging/speakup/speakup_acntpc.c | 26 +++++++++++++-------------
> > > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/speakup/speakup_acntpc.c b/drivers/staging/speakup/speakup_acntpc.c
> > > > index 27f812e..51281be 100644
> > > > --- a/drivers/staging/speakup/speakup_acntpc.c
> > > > +++ b/drivers/staging/speakup/speakup_acntpc.c
> > > > @@ -56,28 +56,28 @@ static struct var_t vars[] = {
> > > >  /* These attributes will appear in /sys/accessibility/speakup/acntpc. */
> > > >  
> > > >  static struct kobj_attribute caps_start_attribute =
> > > > -	__ATTR(caps_start, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
> > > > +	__ATTR_RW(caps_start, 0644, spk_var_show, spk_var_store);
> > > 
> > > This breaks the build :(
> > > 
> > I would assume from adding _RW as is didnt seem to be needed but
> > only a visual reference to the octal's permissions without
> > doing the math. I will look deeper into where __ATTR is
> > probally being called then. I'm not really worried with builds
> > yet till I have a better understanging of the issue as I would
> > expect all of them to fail at this point.
> 
> Never send out a patch that you have not at least test-built, unless you
> say you have not done so (and even then, it's a rare thing to do.)
> 
> thanks,
> 
> greg k-h
As I though was obvious in my note I was seeking clarification on a matter
not to have a patch applied. If that were the case I would have sent it to
the appropriate people and mailing list.

>From the Kernelnewbies main page, "Kernelnewbies is a community of aspiring
Linux kernel developers who work to improve their Kernels and more
experienced developers willing to share their knowledge." So if this is not
The proper channel what is? 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions
  2016-11-26 18:11       ` Walt Feasel
@ 2016-11-26 20:40         ` Greg KH
  0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2016-11-26 20:40 UTC (permalink / raw)
  To: kernelnewbies

On Sat, Nov 26, 2016 at 01:11:23PM -0500, Walt Feasel wrote:
> On Sat, Nov 26, 2016 at 06:18:40PM +0100, Greg KH wrote:
> > On Sat, Nov 26, 2016 at 12:10:28PM -0500, Walt Feasel wrote:
> > > On Sat, Nov 26, 2016 at 12:05:13PM +0100, Greg KH wrote:
> > > > On Mon, Nov 21, 2016 at 10:03:00PM -0500, Walt Feasel wrote:
> > > > > Make suggested checkpatch modifications for
> > > > > WARNING: Symbolic permissions 'S_IWUSR | S_IRUGO' are not preferred.
> > > > > Consider using octal permissions '0644'.
> > > > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred.
> > > > > Consider using octal permissions '0444'.
> > > > > 
> > > > > Signed-off-by: Walt Feasel <waltfeasel@gmail.com>
> > > > > ---
> > > > > I am new to making trivial patches and do not make some
> > > > > for a few type of warnings.
> > > > > This is one of them as I am not fully certain that it is
> > > > > as easy as this.
> > > > > The replacing of 'S_IWUSR | S_IRUGO' with '0644' seems
> > > > > simple enough.
> > > > > However the adding of '_RW' to '__ATTR' to make '__ATTR_RW'
> > > > > I saw in a reply to a patch and am not sure that it would
> > > > > be relevant in this case.
> > > > > I also made a previous patch adding spaces around '|' and
> > > > > want to know if just replacing 'S_IWUSR|S_IRUGO' with
> > > > > '0644' in one shot would be acceptable since my being new
> > > > > and not fixing just one type of warning per patch.
> > > > > Seems straight forward but I have spammed other peoples
> > > > > email and the mailing list enough with improper patches.
> > > > > 
> > > > >  drivers/staging/speakup/speakup_acntpc.c | 26 +++++++++++++-------------
> > > > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/speakup/speakup_acntpc.c b/drivers/staging/speakup/speakup_acntpc.c
> > > > > index 27f812e..51281be 100644
> > > > > --- a/drivers/staging/speakup/speakup_acntpc.c
> > > > > +++ b/drivers/staging/speakup/speakup_acntpc.c
> > > > > @@ -56,28 +56,28 @@ static struct var_t vars[] = {
> > > > >  /* These attributes will appear in /sys/accessibility/speakup/acntpc. */
> > > > >  
> > > > >  static struct kobj_attribute caps_start_attribute =
> > > > > -	__ATTR(caps_start, S_IWUSR | S_IRUGO, spk_var_show, spk_var_store);
> > > > > +	__ATTR_RW(caps_start, 0644, spk_var_show, spk_var_store);
> > > > 
> > > > This breaks the build :(
> > > > 
> > > I would assume from adding _RW as is didnt seem to be needed but
> > > only a visual reference to the octal's permissions without
> > > doing the math. I will look deeper into where __ATTR is
> > > probally being called then. I'm not really worried with builds
> > > yet till I have a better understanging of the issue as I would
> > > expect all of them to fail at this point.
> > 
> > Never send out a patch that you have not at least test-built, unless you
> > say you have not done so (and even then, it's a rare thing to do.)
> > 
> > thanks,
> > 
> > greg k-h
> As I though was obvious in my note I was seeking clarification on a matter
> not to have a patch applied. If that were the case I would have sent it to
> the appropriate people and mailing list.
> 
> From the Kernelnewbies main page, "Kernelnewbies is a community of aspiring
> Linux kernel developers who work to improve their Kernels and more
> experienced developers willing to share their knowledge." So if this is not
> The proper channel what is? 

It is, but really, at least compile the code, the compiler will tell you
that what you are suggesting is not correct much better than anyone else
here can.  Don't rely on others to do your basic work for you :)

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-26 20:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-22  3:03 [TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions Walt Feasel
2016-11-26 11:05 ` Greg KH
2016-11-26 17:10   ` Walt Feasel
2016-11-26 17:18     ` Greg KH
2016-11-26 18:11       ` Walt Feasel
2016-11-26 20:40         ` Greg KH

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.