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.
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_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.
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3d770f55..afb81c1a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -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>
--
2.1.3
On 12/05/14 16:07, George Spelvin wrote:
> 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.
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.
> 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_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.
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3d770f55..afb81c1a 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -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>
>
>
--
~Randy
On 5 Dec 2014 19:07:59 -0500 "George Spelvin" <[email protected]> wrote:
> 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.
>
> 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_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.
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3d770f55..afb81c1a 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -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?
> VERIFY_OCTAL_PERMISSIONS() sticks out like a sore thumb in kernel.h.
> How about we move it into sysfs.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>.
Suggested-by: Andrew Morton <[email protected]>
Signed-off-by: George Spelvin <[email protected]>
---
include/linux/kernel.h | 10 ----------
include/linux/sysfs.h | 11 +++++++++++
include/linux/moduleparam.h | 4 ++--
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3d770f55..07080aa2 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -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
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f97d0dbb..9f213542 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -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) }, \
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 1c9effa2..974097df 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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. */
--
2.1.3
On 5 Dec 2014 20:18:02 -0500 "George Spelvin" <[email protected]> wrote:
> > VERIFY_OCTAL_PERMISSIONS() sticks out like a sore thumb in kernel.h.
> > How about we move it into sysfs.h?
>
> Something like this, you mean?
Kinda.
> 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.
>
> -----
This shouldn't be here because "^---" is considered "end of changelog".
> 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>.
>
> Suggested-by: Andrew Morton <[email protected]>
> Signed-off-by: George Spelvin <[email protected]>
>
> ...
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3d770f55..07080aa2 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -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
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index f97d0dbb..9f213542 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -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))
> +
Let's include bug.h into sysfs.h.
> #define __ATTR(_name, _mode, _show, _store) { \
> .attr = {.name = __stringify(_name), \
> .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 1c9effa2..974097df 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -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?
> /* You can override this manually, but generally this should match the
> module name. */
>> -----
> 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.)
> Let's include bug.h into sysfs.h.
It worked without, but yeah... since I can't figure *how* it works.
>> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
>> index 1c9effa2..974097df 100644
>> --- a/include/linux/moduleparam.h
>> +++ b/include/linux/moduleparam.h
>> @@ -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?
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.)
On Fri, 2014-12-05 at 21:49 -0500, George Spelvin wrote:
> 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.
http://patchwork.ozlabs.org/patch/411356/
On Fri, 2014-12-05 at 18:53 -0800, Joe Perches wrote:
> On Fri, 2014-12-05 at 21:49 -0500, George Spelvin wrote:
> > 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.
>
> http://patchwork.ozlabs.org/patch/411356/
>
>
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.
On 5 Dec 2014 21:49:52 -0500 "George Spelvin" <[email protected]> wrote:
> >> -----
>
> > 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 ;)
> > Let's include bug.h into sysfs.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.
> >> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> >> index 1c9effa2..974097df 100644
> >> --- a/include/linux/moduleparam.h
> >> +++ b/include/linux/moduleparam.h
> >> @@ -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?
>
> 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.)
OK, let's run with it and see what happens.
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.
Suggested-by: Andrew Morton <[email protected]>
Signed-off-by: George Spelvin <[email protected]>
---
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(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3d770f55..07080aa2 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -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
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 1c9effa2..974097df 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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. */
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f97d0dbb..3562f331 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -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) }, \
--
2.1.3
George Spelvin <[email protected]> writes:
> 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.
>
> Suggested-by: Andrew Morton <[email protected]>
> Signed-off-by: George Spelvin <[email protected]>
Acked-by: Rusty Russell <[email protected]>
Thanks,
Rusty.
> ---
> 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(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3d770f55..07080aa2 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -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
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 1c9effa2..974097df 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -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. */
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index f97d0dbb..3562f331 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -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) }, \
> --
> 2.1.3
Hi George,
On Mon, 15 Dec 2014 14:26:15 +1030 Rusty Russell <[email protected]> wrote:
>
> George Spelvin <[email protected]> writes:
> > 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.
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).
--
Cheers,
Stephen Rothwell [email protected]
Stephen Rothwell <[email protected]> wrote:
> 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.
> And testing only
> on x86_64 is 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?
Hi George,
On 15 Dec 2014 19:14:53 -0500 "George Spelvin" <[email protected]> wrote:
>
> Stephen Rothwell <[email protected]> wrote:
> > 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.
> 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.
> 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.
>
> > And testing only
> > on x86_64 is 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.
> 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.
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.
> 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.
--
Cheers,
Stephen Rothwell [email protected]