[TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions

[Greg KH]

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 at 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