Re: [PATCH 2/2 v2] char: misc: use octal permissions for the proc entry
On Sat, Apr 29, 2017 at 10:05 AM, Martin Kaiser wrote: > Thus wrote Andy Shevchenko (andy.shevche...@gmail.com): >> On Tue, Apr 18, 2017 at 12:11 PM, Martin Kaiser wrote: >> Aren't 0 and 0444 different by meaning? > > my undestanding is that proc_create() calls proc_create_data(), where > >if ((mode & S_IALLUGO) == 0) > mode |= S_IRUGO; > > sets mode to 0444 when it was 0. Ah, okay. Fair enough. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2 v2] char: misc: use octal permissions for the proc entry
Hello Andy, Thus wrote Andy Shevchenko (andy.shevche...@gmail.com): > On Tue, Apr 18, 2017 at 12:11 PM, Martin Kaiser wrote: > > checkpatch is asking for a 4 digit octal number. And at least for me, > > 0444 makes it clearer what the permissions actually are. Yes, somewhere > > in the code, I can dig up that 0 is changed to 0444... > Aren't 0 and 0444 different by meaning? my undestanding is that proc_create() calls proc_create_data(), where if ((mode & S_IALLUGO) == 0) mode |= S_IRUGO; sets mode to 0444 when it was 0. Best regards, Martin (added the lkml back to Cc, I removed it by accident in an earlier mail)
Re: [PATCH 2/2 v2] char: misc: use octal permissions for the proc entry
Thus wrote Greg Kroah-Hartman (gre...@linuxfoundation.org):
> > ERROR: Use 4 digit octal (0777) not decimal permissions
> > #285: FILE: drivers/char/misc.c:285:
> > + ret = proc_create("misc", 0, NULL, &misc_proc_fops);
> Come on now, think about what this is saying. Is 0 not also an octal
> number?
checkpatch is asking for a 4 digit octal number. And at least for me,
0444 makes it clearer what the permissions actually are. Yes, somewhere
in the code, I can dig up that 0 is changed to 0444...
> checkpatch requires you to use your brain, it is but a dumb perl
> script...
Sorry, I won't be arguing on a personal level.
If you don't like the patch, feel free to reject it. No problem for me.
Do you think checkpatch shouldn't be looking for exactly 4 digits here
(and elsewhere, it has a list of functions with a permission parameter)?
Best regards,
Martin
Re: [PATCH 2/2 v2] char: misc: use octal permissions for the proc entry
On Tue, Apr 18, 2017 at 09:51:31AM +0200, Martin Kaiser wrote:
> Thus wrote Greg Kroah-Hartman (gre...@linuxfoundation.org):
>
> > > - ret = proc_create("misc", 0, NULL, &misc_proc_fops);
> > > + ret = proc_create("misc", 0444, NULL, &misc_proc_fops);
>
> > What checkpatch warning does this fix? 0 is a number :)
>
> ERROR: Use 4 digit octal (0777) not decimal permissions
> #285: FILE: drivers/char/misc.c:285:
> + ret = proc_create("misc", 0, NULL, &misc_proc_fops);
Come on now, think about what this is saying. Is 0 not also an octal
number?
checkpatch requires you to use your brain, it is but a dumb perl
script...
greg k-h
Re: [PATCH 2/2 v2] char: misc: use octal permissions for the proc entry
Thus wrote Greg Kroah-Hartman (gre...@linuxfoundation.org):
> > - ret = proc_create("misc", 0, NULL, &misc_proc_fops);
> > + ret = proc_create("misc", 0444, NULL, &misc_proc_fops);
> What checkpatch warning does this fix? 0 is a number :)
ERROR: Use 4 digit octal (0777) not decimal permissions
#285: FILE: drivers/char/misc.c:285:
+ ret = proc_create("misc", 0, NULL, &misc_proc_fops);
Best regards,
Martin
Re: [PATCH 2/2 v2] char: misc: use octal permissions for the proc entry
On Mon, Apr 17, 2017 at 08:22:57PM +0200, Martin Kaiser wrote:
> Set the permissions for /proc/misc to 0444 explicitly. At the moment,
> we're using 0 and have proc_create_data() convert this to 0444.
> This fixes a checkpatch warning.
>
> Signed-off-by: Martin Kaiser
> ---
> v2:
>separate patch for each checkpatch warning
>
> drivers/char/misc.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index 3447b2e..3c633d5 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -280,7 +280,7 @@ static int __init misc_init(void)
> int err;
> struct proc_dir_entry *ret;
>
> - ret = proc_create("misc", 0, NULL, &misc_proc_fops);
> + ret = proc_create("misc", 0444, NULL, &misc_proc_fops);
What checkpatch warning does this fix? 0 is a number :)
thanks,
greg k-h
