[TEST PATCH] staging: speakup: speakup_acntpc.c Consider octal permissions
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
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 :(
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.
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
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?
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
participants (2)
-
Greg KH -
Walt Feasel