VERIFY_OCTAL_PERMISSIONS needs <linux/bug.h>
Subsystems (2)
PATCH 1/1 VERIFY_OCTAL_PERMISSIONS needs <linux/bug.h>
It is possible to include <linux/kernel.h> and try to use VERIFY_OCTAL_PERMISSIONS, then puke because BUILD_BUG_ON_ZERO isn't defined.
I hit this via:
#include <linux/moduleparam.h>module_param(verbose, bool, 0);
IMHO, except in documented special cases, header files should #include their own macros' dependencies.
--- include/linux/kernel.h | 1 + 1 file changed, 1 insertion(+)
I'm not quite sure who to send this via. Rusty, you touched VERIFY_OCTAL_PERMISSIONS last. Should I send this via you, or collect acks and include it in the patch series I'm working on that wants this?
The workaround is easy enough, but I'd rather fix it than let cruft like this accumulate.
include/linux/kernel.h +1 -0
@@ -12,6 +12,7 @@#include <linux/typecheck.h> #include <linux/printk.h> #include <linux/dynamic_debug.h> +#include <linux/bug.h> #include <asm/byteorder.h> #include <uapi/linux/kernel.h>
Signature · 1 line
2.1.3
Quoted 11 lines · George Spelvin
It is possible to include <
linux/kernel.h> and try to useVERIFY_OCTAL_PERMISSIONS, then puke becauseBUILD_BUG_ON_ZEROisn't defined.I hit this via:
#include <
linux/moduleparam.h>module_param(verbose, bool, 0);IMHO, except in documented special cases, header files should #include their own macros' dependencies.
Yes. Documentation/SubmitChecklist #1:
1: If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones that you use.
Thanks.
Quoted 25 lines
Signed-off-by George Spelvin <[email protected]>--- include/linux/kernel.h | 1 + 1 file changed, 1 insertion(+)I'm not quite sure who to send this via. Rusty, you touched
VERIFY_OCTAL_PERMISSIONSlast. Should I send this via you, or collect acks and include it in the patch series I'm working on that wants this?The workaround is easy enough, but I'd rather fix it than let cruft like this accumulate.
include/linux/kernel.h +1
-0@@ -12,6 +12,7 @@#include <linux/typecheck.h> #include <linux/printk.h> #include <linux/dynamic_debug.h> +#include <linux/bug.h> #include <asm/byteorder.h> #include <uapi/linux/kernel.h>
Signature · 1 line
~Randy
Quoted 29 lines · On 5 Dec 2014 19:07:59 -0500 "George Spelvin" <[email protected]> wrote:
It is possible to include <
linux/kernel.h> and try to useVERIFY_OCTAL_PERMISSIONS, then puke becauseBUILD_BUG_ON_ZEROisn't defined.I hit this via:
#include <
linux/moduleparam.h>module_param(verbose, bool, 0);IMHO, except in documented special cases, header files should #include their own macros' dependencies.
Signed-off-by George Spelvin <[email protected]>--- include/linux/kernel.h | 1 + 1 file changed, 1 insertion(+)I'm not quite sure who to send this via. Rusty, you touched
VERIFY_OCTAL_PERMISSIONSlast. Should I send this via you, or collect acks and include it in the patch series I'm working on that wants this?The workaround is easy enough, but I'd rather fix it than let cruft like this accumulate.
include/linux/kernel.h +0
-0
include/linux/kernel.h line 17
Quoted 6 lines
include/linux/kernel.h +1
-0@@ -12,6 +12,7 @@#include <linux/typecheck.h> #include <linux/printk.h> #include <linux/dynamic_debug.h> +#include <linux/bug.h> #include <asm/byteorder.h> #include <uapi/linux/kernel.h>
Absolutely everything includes kernel.h so this might have measurable build-time impact.
VERIFY_OCTAL_PERMISSIONS() sticks out like a sore thumb in kernel.h. How about we move it into sysfs.h?
PATCH 1/1 VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Quoted 2 lines
VERIFY_OCTAL_PERMISSIONS()sticks out like a sore thumb inkernel.h. How about we move it intosysfs.h?
Something like this, you mean?
The <linux/types.h> in moduleparam.h is needed for one function prototype that passes s16 parameters. My first reaction is to wonder if that can be gotten rid of, too.
-----
It's the only user of <linux/bug.h> in kernel.h, so that reduces the compile-time cost of #include <linux/kernel.h>
One necessary consequent change in <linux/moduleparam.h>.
--- include/linux/kernel.h | 10 ---------- include/linux/sysfs.h | 11 +++++++++++ include/linux/moduleparam.h | 4 ++-- 3 files changed, 13 insertions(+), 12 deletions(-)
include/linux/kernel.h +0 -10
@@ -804,14 +804,4 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }# define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD #endif -/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */ -#define VERIFY_OCTAL_PERMISSIONS (perms) \ - (BUILD_BUG_ON_ZERO((perms) < 0) + \ - BUILD_BUG_ON_ZERO((perms) > 0777) + \ -/* User perms >= group perms >= other perms */ \ - BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) + \ - BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) + \ - /* Other writable? Generally considered a bad idea. */ \ - BUILD_BUG_ON_ZERO((perms) & 2) + \ - (perms))#endif
include/linux/sysfs.h +11 -0
@@ -70,6 +70,17 @@ struct attribute_group {* for examples.. */ +/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */ +#define VERIFY_OCTAL_PERMISSIONS (perms) \ + (BUILD_BUG_ON_ZERO((perms) < 0) + \ + BUILD_BUG_ON_ZERO((perms) > 0777) + \ +/* User perms >= group perms >= other perms */ \ + BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) + \ + BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) + \ + /* Other writable? Generally considered a bad idea. */ \ + BUILD_BUG_ON_ZERO((perms) & 2) + \ + (perms)) +#define __ATTR (_name, _mode, _show, _store) { \ .attr = {.name = __stringify(_name), \ .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
include/linux/moduleparam.h +2 -2
@@ -1,9 +1,9 @@#ifndef _LINUX_MODULE_PARAMS_H#define _LINUX_MODULE_PARAMS_H/* (C) Copyright 2001, 2002 Rusty Russell IBM Corporation */ -#include <linux/init.h> #include <linux/stringify.h> -#include <linux/kernel.h> +#include <linux/sysfs.h> +#include <linux/types.h> /* You can override this manually, but generally this should match the module name. */
Signature · 1 line
2.1.3
Re: [PATCH] [PATCH] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Quoted 4 lines · On 5 Dec 2014 20:18:02 -0500 "George Spelvin" <[email protected]> wrote:
Quoted 2 lines
VERIFY_OCTAL_PERMISSIONS()sticks out like a sore thumb inkernel.h. How about we move it intosysfs.h?Something like this, you mean?
Kinda.
Quoted 5 lines
The <
linux/types.h> inmoduleparam.his needed for one function prototype that passes s16 parameters. My first reaction is to wonder if that can be gotten rid of, too.-----
This shouldn't be here because "^---" is considered "end of changelog".
Quoted 38 lines
It's the only user of <
linux/bug.h> inkernel.h, so that reduces the compile-time cost of #include <linux/kernel.h>One necessary consequent change in <
linux/moduleparam.h>.Suggested-by Andrew Morton <[email protected]>Signed-off-by George Spelvin <[email protected]>...
include/linux/kernel.h +0
-10@@ -804,14 +804,4 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }# define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD #endif -/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */ -#define VERIFY_OCTAL_PERMISSIONS (perms) \ - (BUILD_BUG_ON_ZERO((perms) < 0) + \ - BUILD_BUG_ON_ZERO((perms) > 0777) + \ -/* User perms >= group perms >= other perms */ \ - BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) + \ - BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) + \ - /* Other writable? Generally considered a bad idea. */ \ - BUILD_BUG_ON_ZERO((perms) & 2) + \ - (perms))#endif include/linux/sysfs.h +1
-0@@ -70,6 +70,17 @@ struct attribute_group {* for examples.. */ +/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */
include/linux/sysfs.h struct attribute_group { · line 83
Quoted 10 lines
include/linux/sysfs.h +10
-0+#define VERIFY_OCTAL_PERMISSIONS (perms) \ + (BUILD_BUG_ON_ZERO((perms) < 0) + \ + BUILD_BUG_ON_ZERO((perms) > 0777) + \ +/* User perms >= group perms >= other perms */ \ +BUILD_BUG_ON_ZERO (((perms) >>6 ) < (((perms) >>3 ) &7 )) + \ +BUILD_BUG_ON_ZERO ((((perms) >>3 ) &7 ) < ((perms) &7 )) + \ +/* Other writable? Generally considered a bad idea. */ \ +BUILD_BUG_ON_ZERO ((perms) &2 ) + \ + (perms)) +
Let's include bug.h into sysfs.h.
Quoted 8 lines
include/linux/sysfs.h +0
-0#define __ATTR (_name, _mode, _show, _store) { \ .attr = {.name = __stringify(_name), \ .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \include/linux/moduleparam.h +0
-0
include/linux/moduleparam.h line 6
Quoted 8 lines
include/linux/moduleparam.h +2
-2@@ -1,9 +1,9 @@#ifndef _LINUX_MODULE_PARAMS_H#define _LINUX_MODULE_PARAMS_H/* (C) Copyright 2001, 2002 Rusty Russell IBM Corporation */ -#include <linux/init.h> #include <linux/stringify.h> -#include <linux/kernel.h> +#include <linux/sysfs.h> +#include <linux/types.h>
Removing the kernel.h inclusion is good, but risky. Are you sure there's nothing in moduleparam.h which uses kernel.h things?
Quoted 2 lines
include/linux/moduleparam.h +0
-0/* You can override this manually, but generally this should match the module name. */
Re: [PATCH] [PATCH] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Quoted 3 lines
Quoted 1 line
-----
This shouldn't be here because "^---" is considered "end of changelog".
Damn it, I couldn't remember how to add not-for-commit comments to a patch. I RTFMed and thought that was what "git mailinfo --scissors" wanted.
(Normally, I put it down with the diffstat, but preferred a more prominent place for this conversation.)
Quoted 1 line
Let's include
bug.hintosysfs.h.
It worked without, but yeah... since I can't figure *how* it works.
Quoted 8 lines
Quoted 8 lines
include/linux/moduleparam.h +0
-0@@ -1,9 +1,9 @@#ifndef _LINUX_MODULE_PARAMS_H#define _LINUX_MODULE_PARAMS_H/* (C) Copyright 2001, 2002 Rusty Russell IBM Corporation */
Quoted 8 lines
include/linux/moduleparam.h line 6
Quoted 5 lines
include/linux/moduleparam.h +2
-2-#include <linux/init.h> #include <linux/stringify.h> -#include <linux/kernel.h> +#include <linux/sysfs.h> +#include <linux/types.h> Removing the
kernel.hinclusion is good, but risky. Are you sure there's nothing inmoduleparam.hwhich useskernel.hthings?
Well, I did a quick eyeball-scan, then did my usual build, then an allyesconfig and an allmodconfig.
I found some other alarming things, but nothing related to this.
E.g.drivers/net/ethernet/intel/i40e/i40e_debugfs.c: In function 'i40e_dbg_dump_desc':drivers/net/ethernet/intel/i40e/i40e_debugfs.c:855:1: warning: the frame size of 8192 bytes is larger than 2048 bytes [-Wframe-larger-than=]
}
(Honestly, I didn't wait for the allmodconfig to finish, since I expected it to fail within the first few hundred modules if it was going to fail at all, but it has finished now.)
Re: [PATCH] [PATCH] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Quoted 5 lines · George Spelvin
I found some other alarming things, but nothing related to this.
E.g.
drivers/net/ethernet/intel/i40e/i40e_debugfs.c: In function 'i40e_dbg_dump_desc':drivers/net/ethernet/intel/i40e/i40e_debugfs.c:855:1: warning: the frame size of 8192 bytes is larger than 2048 bytes [-Wframe-larger-than=]
Patched.
Waiting for Jeff Kirsher to get 'round to applying it.
Re: [PATCH] [PATCH] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Quoted 13 lines · Joe Perches
Quoted 5 lines · George Spelvin
I found some other alarming things, but nothing related to this.
E.g.
drivers/net/ethernet/intel/i40e/i40e_debugfs.c: In function 'i40e_dbg_dump_desc':drivers/net/ethernet/intel/i40e/i40e_debugfs.c:855:1: warning: the frame size of 8192 bytes is larger than 2048 bytes [-Wframe-larger-than=]Patched.
Waiting for Jeff Kirsher to get 'round to applying it.
I have it queued up for the next series of i40e/i40evf patches I will be pushing. Just waiting on the acceptance of my latest series I pushed this morning, then I will be pushing this and other i40e patches.
Re: [PATCH] [PATCH] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
In reply to George SpelvinQuoted 9 lines · On 5 Dec 2014 21:49:52 -0500 "George Spelvin" <[email protected]> wrote:
Quoted 3 lines
Quoted 1 line
-----
This shouldn't be here because "^---" is considered "end of changelog".
Damn it, I couldn't remember how to add not-for-commit comments to a patch. I RTFMed and thought that was what "git mailinfo --scissors" wanted.
(Normally, I put it down with the diffstat, but preferred a more prominent place for this conversation.)
I think in 100% of cases I find the stuff that people put below the ^--- was useful info, so I move it into the changelog. Yours was no exception ;)
Quoted 3 lines
Quoted 1 line
Let's include
bug.hintosysfs.h.It worked without, but yeah... since I can't figure *how* it works.
heh. KCPPFLAGS=-H make foo.o' will show the include graph. I usually do make foo.i' then go poke around in foo.i to work out how foo.c received a particular definition.
Quoted 22 lines
Quoted 8 lines
Quoted 8 lines
include/linux/moduleparam.h +0
-0@@ -1,9 +1,9 @@#ifndef _LINUX_MODULE_PARAMS_H#define _LINUX_MODULE_PARAMS_H/* (C) Copyright 2001, 2002 Rusty Russell IBM Corporation */ Quoted 8 lines
include/linux/moduleparam.h line 6
Quoted 5 lines
include/linux/moduleparam.h +2
-2-#include <linux/init.h> #include <linux/stringify.h> -#include <linux/kernel.h> +#include <linux/sysfs.h> +#include <linux/types.h> Removing the
kernel.hinclusion is good, but risky. Are you sure there's nothing inmoduleparam.hwhich useskernel.hthings?Well, I did a quick eyeball-scan, then did my usual build, then an allyesconfig and an allmodconfig.
I found some other alarming things, but nothing related to this.
Quoted 8 lines
E.g.
drivers/net/ethernet/intel/i40e/i40e_debugfs.c: In function 'i40e_dbg_dump_desc':drivers/net/ethernet/intel/i40e/i40e_debugfs.c:855:1: warning: the frame size of 8192 bytes is larger than 2048 bytes [-Wframe-larger-than=]
}(Honestly, I didn't wait for the allmodconfig to finish, since I expected it to fail within the first few hundred modules if it was going to fail at all, but it has finished now.)
OK, let's run with it and see what happens.
PATCH v2 1/1 VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
It's the only user of <linux/bug.h> in kernel.h, so that reduces the compile-time cost of #include <linux/kernel.h>
Only one user has to change: <linux/moduleparam.h>. The <linux/types.h> there is needed for one function prototype that passes s16 parameters. My first reaction is to wonder if that can be gotten rid of, too.
Some other extraneous header files pruned while I was at it. Tested with allyesconfig & allmodconfig on x86-64, just to be sure.
---
Look, even more header pruning.
include/linux/kernel.h | 10 ---------- include/linux/moduleparam.h | 4 ++-- include/linux/sysfs.h | 15 ++++++++++++--- 3 files changed, 14 insertions(+), 15 deletions(-)
include/linux/kernel.h +0 -10
@@ -804,14 +804,4 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }# define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD #endif -/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */ -#define VERIFY_OCTAL_PERMISSIONS (perms) \ - (BUILD_BUG_ON_ZERO((perms) < 0) + \ - BUILD_BUG_ON_ZERO((perms) > 0777) + \ -/* User perms >= group perms >= other perms */ \ - BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) + \ - BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) + \ - /* Other writable? Generally considered a bad idea. */ \ - BUILD_BUG_ON_ZERO((perms) & 2) + \ - (perms))#endif
include/linux/moduleparam.h +2 -2
@@ -1,9 +1,9 @@#ifndef _LINUX_MODULE_PARAMS_H#define _LINUX_MODULE_PARAMS_H/* (C) Copyright 2001, 2002 Rusty Russell IBM Corporation */ -#include <linux/init.h> #include <linux/stringify.h> -#include <linux/kernel.h> +#include <linux/sysfs.h> +#include <linux/types.h> /* You can override this manually, but generally this should match the module name. */
include/linux/sysfs.h +12 -3
@@ -14,12 +14,10 @@#include <linux/kernfs.h> #include <linux/compiler.h> -#include <linux/errno.h> -#include <linux/list.h> #include <linux/lockdep.h> #include <linux/kobject_ns.h> #include <linux/stat.h> -#include <linux/atomic.h> +#include <linux/bug.h> struct kobject ;struct module ;
@@ -70,6 +68,17 @@ struct attribute_group {* for examples.. */ +/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */ +#define VERIFY_OCTAL_PERMISSIONS (perms) \ + (BUILD_BUG_ON_ZERO((perms) < 0) + \ + BUILD_BUG_ON_ZERO((perms) > 0777) + \ +/* User perms >= group perms >= other perms */ \ + BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) + \ + BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) + \ + /* Other writable? Generally considered a bad idea. */ \ + BUILD_BUG_ON_ZERO((perms) & 2) + \ + (perms)) +#define __ATTR (_name, _mode, _show, _store) { \ .attr = {.name = __stringify(_name), \ .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
Signature · 1 line
2.1.3
Re: [PATCH v2] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Quoted 7 lines · George Spelvin
It's the only user of <
linux/bug.h> inkernel.h, so that reduces the compile-time cost of #include <linux/kernel.h>Only one user has to change: <
linux/moduleparam.h>. The <linux/types.h> there is needed for one function prototype that passes s16 parameters. My first reaction is to wonder if that can be gotten rid of, too.
Quoted 6 lines
Some other extraneous header files pruned while I was at it. Tested with allyesconfig & allmodconfig on x86-64, just to be sure.
Suggested-by Andrew Morton <[email protected]>Signed-off-by George Spelvin <[email protected]>
Thanks,
Rusty.
Quoted 81 lines
---
Look, even more header pruning.include/linux/kernel.h | 10 ---------- include/linux/moduleparam.h | 4 ++-- include/linux/sysfs.h | 15 ++++++++++++--- 3 files changed, 14 insertions(+), 15 deletions(-)include/linux/kernel.h +0
-10@@ -804,14 +804,4 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }# define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD #endif -/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */ -#define VERIFY_OCTAL_PERMISSIONS (perms) \ - (BUILD_BUG_ON_ZERO((perms) < 0) + \ - BUILD_BUG_ON_ZERO((perms) > 0777) + \ -/* User perms >= group perms >= other perms */ \ - BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) + \ - BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) + \ - /* Other writable? Generally considered a bad idea. */ \ - BUILD_BUG_ON_ZERO((perms) & 2) + \ - (perms))#endif include/linux/moduleparam.h +2
-2@@ -1,9 +1,9 @@#ifndef _LINUX_MODULE_PARAMS_H#define _LINUX_MODULE_PARAMS_H/* (C) Copyright 2001, 2002 Rusty Russell IBM Corporation */ -#include <linux/init.h> #include <linux/stringify.h> -#include <linux/kernel.h> +#include <linux/sysfs.h> +#include <linux/types.h> /* You can override this manually, but generally this should match the module name. */ include/linux/sysfs.h +12
-3@@ -14,12 +14,10 @@#include <linux/kernfs.h> #include <linux/compiler.h> -#include <linux/errno.h> -#include <linux/list.h> #include <linux/lockdep.h> #include <linux/kobject_ns.h> #include <linux/stat.h> -#include <linux/atomic.h> +#include <linux/bug.h> struct kobject ;struct module ;@@ -70,6 +68,17 @@ struct attribute_group {* for examples.. */ +/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */ +#define VERIFY_OCTAL_PERMISSIONS (perms) \ + (BUILD_BUG_ON_ZERO((perms) < 0) + \ + BUILD_BUG_ON_ZERO((perms) > 0777) + \ +/* User perms >= group perms >= other perms */ \ + BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) + \ + BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) + \ + /* Other writable? Generally considered a bad idea. */ \ + BUILD_BUG_ON_ZERO((perms) & 2) + \ + (perms)) +#define __ATTR (_name, _mode, _show, _store) { \ .attr = {.name = __stringify(_name), \ .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \Signature · 1 line
2.1.3
Re: [PATCH v2] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Hi George,
Quoted 5 lines · On Mon, 15 Dec 2014 14:26:15 +1030 Rusty Russell <[email protected]> wrote:
Quoted 3 lines · George Spelvin
It's the only user of <
linux/bug.h> inkernel.h, so that reduces the compile-time cost of #include <linux/kernel.h>
Quoted 7 lines
Quoted 7 lines
Only one user has to change: <
linux/moduleparam.h>. The <linux/types.h> there is needed for one function prototype that passes s16 parameters. My first reaction is to wonder if that can be gotten rid of, too.Some other extraneous header files pruned while I was at it. Tested with allyesconfig & allmodconfig on x86-64, just to be sure.
Please do *not* mix changes up like this. Split this out into a separate patch, please (1 logical change per patch). And testing only on x86_64 is not "sure" when talking about header file pruning (but at least you did the "all" configs).
Signature · 2 lines
Cheers, Stephen Rothwell [email protected]
Re: [PATCH v2] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Quoted 2 lines · Stephen Rothwell
Please do *not* mix changes up like this. Split this out into a separate patch, please (1 logical change per patch).
Um... I thought I was doing that. More particularly, the task of untangling header file dependencies eseemed sufficiently cohesive that it could be considered one logical change.
It was one round of thinking and analysis about what declarations the affected files depend on.
Although syntactically possible, given the small size of the change (I deleted a total of 5 #includes, 2 from moduleparam.h and 3 from sysfs.h), it didn't seem worth breaking it up further.
Quoted 3 lines
And testing only
onx86_64is not "sure" when talking about header file pruning (but at least you did the "all" configs).
Well, the first round was reading and understanding the headers; the compile was just to make sure.
The files I was messing with (moduleparam.h and sysfs.h) don't have a lot of architecture-specificness within them. The main danger is that they're used in a zillion places and some caller might depend on the over-inclusion.
I was rushing to get that to Andrew while the coversation was ongoing (if you check the mail headers, only a few minutes separated the various messages) so I was less cautious than usual, but given that a mistake would result in a nice obvious compile error (with an almost as obvious fix), it seemed worth the risk.
If my haste made me judge wrong, I apologize. Was I very wrong, or just a bit over the line?
Re: [PATCH v2] VERIFY_OCTAL_PERMISSIONS: Move to <linux/sysfs.h> where it belongs
Hi George,
Quoted 8 lines · On 15 Dec 2014 19:14:53 -0500 "George Spelvin" <[email protected]> wrote:
Quoted 2 lines · Stephen Rothwell
Please do *not* mix changes up like this. Split this out into a separate patch, please (1 logical change per patch).
Um... I thought I was doing that. More particularly, the task of untangling header file dependencies eseemed sufficiently cohesive that it could be considered one logical change.
Well, given the subject of the commit, I expected a simple change that just did the move (and any immediately associated include changes). You then said "Some other extraneous header files pruned while I was at it" and that part I would expect to be in a separate patch.
Quoted 2 lines
It was one round of thinking and analysis about what declarations the affected files depend on.
Which is separate from what VERIFY_OCTAL_PERMISSIONS() requires.
Quoted 10 lines
Although syntactically possible, given the small size of the change (I deleted a total of 5 #includes, 2 from
moduleparam.hand 3 fromsysfs.h), it didn't seem worth breaking it up further.Quoted 3 lines
And testing only
onx86_64is not "sure" when talking about header file pruning (but at least you did the "all" configs).Well, the first round was reading and understanding the headers; the compile was just to make sure.
Understood, it was more a "actually changing architectures was more likely to show breakage than building lots of stuff". I guess I see more breakage from pruning includes than most people since I build for multiple architectures more than most.
Quoted 4 lines
The files I was messing with (
moduleparam.handsysfs.h) don't have a lot of architecture-specificness within them. The main danger is that they're used in a zillion places and some caller might depend on the over-inclusion.
The problem is not the direct includes and direct architecture depenedencies, but the fact that lost of stuff ends up include asm/ include files at some point in the chain and that affects the set of files implicitly included. X86 seems to implicitly include more than some other architectures.
Quoted 2 lines
If my haste made me judge wrong, I apologize. Was I very wrong, or just a bit over the line?
Probably just a bit over the line. The advantage of the split would be that when it hits Andrew's tree and then breaks linux-next (for me), I can pick on a smaller patch to get rid of/correct.
Signature · 2 lines
Cheers, Stephen Rothwell [email protected]