| Message ID | 20160802103759.17958-1-baolex.ni@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 8/2/2016 1:37 PM, Baole Ni wrote: > I find that the developers often just specified the numeric value > when calling a macro which is defined with a parameter for access permission. > As we know, these numeric value for access permission have had the corresponding macro, > and that using macro can improve the robustness and readability of the code, > thus, I suggest replacing the numeric parameter with the macro. > > Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com> > Signed-off-by: Baole Ni <baolex.ni@intel.com> > --- > drivers/ata/pata_legacy.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c > index bce2a8c..1dc0beb 100644 > --- a/drivers/ata/pata_legacy.c > +++ b/drivers/ata/pata_legacy.c > @@ -76,7 +76,7 @@ > #define NR_HOST 6 > > static int all; > -module_param(all, int, 0444); > +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH); There's S_IRUGO for this case, no? MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 8/2/2016 2:30 PM, Sergei Shtylyov wrote: >> I find that the developers often just specified the numeric value >> when calling a macro which is defined with a parameter for access permission. >> As we know, these numeric value for access permission have had the >> corresponding macro, >> and that using macro can improve the robustness and readability of the code, >> thus, I suggest replacing the numeric parameter with the macro. >> >> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com> >> Signed-off-by: Baole Ni <baolex.ni@intel.com> >> --- >> drivers/ata/pata_legacy.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c >> index bce2a8c..1dc0beb 100644 >> --- a/drivers/ata/pata_legacy.c >> +++ b/drivers/ata/pata_legacy.c >> @@ -76,7 +76,7 @@ >> #define NR_HOST 6 >> >> static int all; >> -module_param(all, int, 0444); >> +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH); > > There's S_IRUGO for this case, no? Sending 1285 patches with the same subject also was a bad idea. You need a subsystem/driver prefix in order to somehow differ them. MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 02, 2016 at 03:32:57PM +0300, Sergei Shtylyov wrote: > > > static int all; > > > -module_param(all, int, 0444); > > > +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH); > > > > There's S_IRUGO for this case, no? Sure, and honestly, I understand what 0444 is better than seeing: S_IRUSR | S_IRGRP | SIROTH Heck, 0444 is more understandable to me than S_IRUGO, because honestly, those macros are just as cryptic as 0444 is. Working with Unix/Linux systems since 1991, I understand the octo numbers very well. And I'm sure most other people do to. Any file that I'm Cc'd on here will get an automatic NAK from me. > > Sending 1285 patches with the same subject also was a bad idea. You need > a subsystem/driver prefix in order to somehow differ them. Yes, it's a very good way to be added to everyone's /dev/null folder too. Each subsystem should have one patch that covers all its files. Not a patch per file! What? Is Intel now give extra bonuses for commit numbers? Sorry, but I'm a little grumpy when my phone starts popping like a popcorn machine while I'm having my breakfast because of these silly emails. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Steven, >>>> static int all; >>>> -module_param(all, int, 0444); >>>> +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH); >>> >>> There's S_IRUGO for this case, no? > > Sure, and honestly, I understand what 0444 is better than seeing: > > S_IRUSR | S_IRGRP | SIROTH > > Heck, 0444 is more understandable to me than S_IRUGO, because honestly, those > macros are just as cryptic as 0444 is. Working with Unix/Linux systems since > 1991, I understand the octo numbers very well. And I'm sure most other people > do to. Any file that I'm Cc'd on here will get an automatic NAK from me. you are not helping to reduce the 1285 patch bomb. Your automatic replies make it worse now ;) Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Tue, Aug 02, 2016 at 06:37:59PM +0800, Baole Ni wrote: > I find that the developers often just specified the numeric value > when calling a macro which is defined with a parameter for access permission. > As we know, these numeric value for access permission have had the corresponding macro, > and that using macro can improve the robustness and readability of the code, > thus, I suggest replacing the numeric parameter with the macro. > > Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com> > Signed-off-by: Baole Ni <baolex.ni@intel.com> I don't see the point. This actually makes code harder to read. Sure, in general, we want to avoid using magic raw numbers but the octal permission numbers are pretty much canonical. These conversions are pure noises. Nacked-by: Tejun Heo <tj@kernel.org> Thanks.
From: Tejun Heo <tj@kernel.org> Date: Tue, 2 Aug 2016 14:27:25 -0400 > the octal permission numbers are pretty much canonical +1 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c index bce2a8c..1dc0beb 100644 --- a/drivers/ata/pata_legacy.c +++ b/drivers/ata/pata_legacy.c @@ -76,7 +76,7 @@ #define NR_HOST 6 static int all; -module_param(all, int, 0444); +module_param(all, int, S_IRUSR | S_IRGRP | S_IROTH); MODULE_PARM_DESC(all, "Grab all legacy port devices, even if PCI(0=off, 1=on)"); enum controller {