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

[Greg KH]

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