Re: [PATCH V2] include/linux/kernel.h: Move logging bits to include/linux/logging.h

Previous thread: [PATCH] include/linux/kernel.h: Move logging bits to include/linux/logging.h by Joe Perches on Monday, November 8, 2010 - 10:37 pm. (1 message)

Next thread: A MMC card transfer issue by Tomoya MORINAGA on Monday, November 8, 2010 - 10:39 pm. (5 messages)
From: Joe Perches
Date: Monday, November 8, 2010 - 10:38 pm

Move the logging bits from kernel.h into logging.h so that
there is a bit more logical separation of the generic from
the logging specific parts.

Add pr_<level>_once macros to logging.h.

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/kernel.h  |  246 +-----------------------------------
 include/linux/logging.h |  322 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 324 insertions(+), 244 deletions(-)
 create mode 100644 include/linux/logging.h

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index b526947..a8be3e4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -17,13 +17,11 @@
 #include <linux/bitops.h>
 #include <linux/log2.h>
 #include <linux/typecheck.h>
+#include <linux/logging.h>
 #include <linux/dynamic_debug.h>
 #include <asm/byteorder.h>
 #include <asm/bug.h>
 
-extern const char linux_banner[];
-extern const char linux_proc_banner[];
-
 #define USHRT_MAX	((u16)(~0U))
 #define SHRT_MAX	((s16)(USHRT_MAX>>1))
 #define SHRT_MIN	((s16)(-SHRT_MAX - 1))
@@ -42,7 +40,7 @@ extern const char linux_proc_banner[];
 #define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
 #define __ALIGN_MASK(x, mask)	__ALIGN_KERNEL_MASK((x), (mask))
 #define PTR_ALIGN(p, a)		((typeof(p))ALIGN((unsigned long)(p), (a)))
-#define IS_ALIGNED(x, a)		(((x) & ((typeof(x))(a) - 1)) == 0)
+#define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 
@@ -110,31 +108,6 @@ extern const char linux_proc_banner[];
  */
 #define lower_32_bits(n) ((u32)(n))
 
-#define	KERN_EMERG	"<0>"	/* system is unusable			*/
-#define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
-#define	KERN_CRIT	"<2>"	/* critical conditions			*/
-#define	KERN_ERR	"<3>"	/* error conditions			*/
-#define	KERN_WARNING	"<4>"	/* warning conditions			*/
-#define	KERN_NOTICE	"<5>"	/* normal but significant condition	*/
-#define	KERN_INFO	"<6>"	/* ...
From: Linus Torvalds
Date: Tuesday, November 9, 2010 - 11:23 am

I detest patches like this.

The code movement makes it really hard to see what changed. So if you
do code movement, please do _only_ code movement, and send a separate
patch that actually changes things.

                             Linus
--

From: Joe Perches
Date: Tuesday, November 9, 2010 - 11:43 am

Move the logging bits from kernel.h into logging.h so that
there is a bit more logical separation of the generic from
the logging specific parts.

Signed-off-by: Joe Perches <joe@perches.com>
---
diff from V1 - Don't add pr_<level>_once macros

 include/linux/kernel.h  |  244 +--------------------------------------
 include/linux/logging.h |  295 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 296 insertions(+), 243 deletions(-)
 create mode 100644 include/linux/logging.h

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index b526947..b7a1595 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -17,13 +17,11 @@
 #include <linux/bitops.h>
 #include <linux/log2.h>
 #include <linux/typecheck.h>
+#include <linux/logging.h>
 #include <linux/dynamic_debug.h>
 #include <asm/byteorder.h>
 #include <asm/bug.h>
 
-extern const char linux_banner[];
-extern const char linux_proc_banner[];
-
 #define USHRT_MAX	((u16)(~0U))
 #define SHRT_MAX	((s16)(USHRT_MAX>>1))
 #define SHRT_MIN	((s16)(-SHRT_MAX - 1))
@@ -110,31 +108,6 @@ extern const char linux_proc_banner[];
  */
 #define lower_32_bits(n) ((u32)(n))
 
-#define	KERN_EMERG	"<0>"	/* system is unusable			*/
-#define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
-#define	KERN_CRIT	"<2>"	/* critical conditions			*/
-#define	KERN_ERR	"<3>"	/* error conditions			*/
-#define	KERN_WARNING	"<4>"	/* warning conditions			*/
-#define	KERN_NOTICE	"<5>"	/* normal but significant condition	*/
-#define	KERN_INFO	"<6>"	/* informational			*/
-#define	KERN_DEBUG	"<7>"	/* debug-level messages			*/
-
-/* Use the default kernel loglevel */
-#define KERN_DEFAULT	"<d>"
-/*
- * Annotation for a "continued" line of log printout (only done after a
- * line that had no enclosing \n). Only to be used by core/arch code
- * during early bootup (a continued line is not SMP-safe otherwise).
- */
-#define	KERN_CONT	"<c>"
-
-extern int console_printk[];
-
-#define console_loglevel ...
From: Linus Torvalds
Date: Tuesday, November 9, 2010 - 11:49 am

So where do the extra 53 lines come from now?

This still is clearly not just code movement, and it's impossible for
me to see what actually changed. Something must have.

                                 Linus
--

From: Joe Perches
Date: Tuesday, November 9, 2010 - 12:17 pm

Differences in logging.h to original kernel.h
were done for cleanliness and checkpatch.

o comment added for purpose of struct va_format
o function no_print() broken into multiple lines
o #ifdef CONFIG_PRINTK vprintk/printk block
  and #else block moved together
o printk_ratelimit and related now have another
  CONFIG_PRINTK and #else block
o pr_debug and pr_debug_ratelimit statement

If you want a nearly identical line count
for logging.h, followed by a cleanup only pass
of logging.h, I'll do that.

Is that what you want?

I didn't recompile and verify this second patch
as all I did was remove the unused for now
pr_<level>_once macros I added.

It takes my slowish setup quite a while to do
full compiles for allyesconfig/allmodconfig/
allnoconfig/defconfig/CONFIG_EMBEDDED/CONFIG_PRINTK=n
variants though so it'd be another day or so before
it'd be verified.

--

From: Ted Ts'o
Date: Tuesday, November 9, 2010 - 2:00 pm

Yet another reason why I detest mindless use of checkpatch.  And as a
maintainer, I balance the value of code movement patches in particular
against how it breaks other people's patches, and how unless I do
*very* careful review, people can sneak in changes that could either
(a) break code accidentally, or even (b) introduce security holes
deliberately.  (i.e., if I was in charge of the cyber warfare division
of some large nation state, a really obvious way of trying to insert
vulnerabilities into code is to send "cleanup" patches that move code
around, and introduce some subtle buffer overrun or other
security change.)

As a result, I spend extra time reviewing patches that move code
around, and I **really** dislike patches that try to do any kind of
cleanup (checkpatch.pl or otherwise) at the same time as they move
code around.

	       	    	    	      	   - Ted
--

From: Joe Perches
Date: Tuesday, November 9, 2010 - 2:08 pm

If it makes you happier, I didn't use it here nor do I
generally use checkpatch, (just ask Andrew Morton), I just

So fine, I'll do a minimal code movement only patch
followed by a style cleansing patch.

Anyone else have an opinion on using logging.h vs printk.h
as an include name?


--

From: Joe Perches
Date: Tuesday, November 9, 2010 - 4:52 pm

Move the logging bits from kernel.h into printk.h so that
there is a bit more logical separation of the generic from
the printk logging specific parts.

Signed-off-by: Joe Perches <joe@perches.com>
---
v3 differences:
Rename logging.h to printk.h
Direct movement of lines from kernel.h to printk.h
One trailing space deleted from a comment to keep git am quiet

Compiled x86 defconfig, allnoconfig, allmodconfig and CONFIG_PRINTK=n only

 include/linux/kernel.h |  244 +-----------------------------------------------
 include/linux/printk.h |  247 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 248 insertions(+), 243 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index b526947..b6de9a6 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -17,13 +17,11 @@
 #include <linux/bitops.h>
 #include <linux/log2.h>
 #include <linux/typecheck.h>
+#include <linux/printk.h>
 #include <linux/dynamic_debug.h>
 #include <asm/byteorder.h>
 #include <asm/bug.h>
 
-extern const char linux_banner[];
-extern const char linux_proc_banner[];
-
 #define USHRT_MAX	((u16)(~0U))
 #define SHRT_MAX	((s16)(USHRT_MAX>>1))
 #define SHRT_MIN	((s16)(-SHRT_MAX - 1))
@@ -110,31 +108,6 @@ extern const char linux_proc_banner[];
  */
 #define lower_32_bits(n) ((u32)(n))
 
-#define	KERN_EMERG	"<0>"	/* system is unusable			*/
-#define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
-#define	KERN_CRIT	"<2>"	/* critical conditions			*/
-#define	KERN_ERR	"<3>"	/* error conditions			*/
-#define	KERN_WARNING	"<4>"	/* warning conditions			*/
-#define	KERN_NOTICE	"<5>"	/* normal but significant condition	*/
-#define	KERN_INFO	"<6>"	/* informational			*/
-#define	KERN_DEBUG	"<7>"	/* debug-level messages			*/
-
-/* Use the default kernel loglevel */
-#define KERN_DEFAULT	"<d>"
-/*
- * Annotation for a "continued" line of log printout (only done after a
- * line that had no enclosing \n). Only to be used by core/arch code
- * during early ...
From: Alexey Dobriyan
Date: Tuesday, November 9, 2010 - 1:01 pm

Why linux_banner is moved?
Why linux_proc_banner is moved?
Why header is not named printk.h?
Why random stuff like log_buf_kexec_setup() and hexdump is moved?
--

From: Joe Perches
Date: Tuesday, November 9, 2010 - 1:10 pm

hexdump functions are logging message related
as is log_buf_kexec_setup.

hexdump should/will eventually have an
#ifdef CONFIG_PRINTK
	external function prototypes
#else
static inline void
hex_dump_to_buffer(const void *buf, size_t len,
		   int rowsize, int groupsize,
		   char *linebuf, size_t linebuflen, bool ascii) {}
static inline void
print_hex_dump(const char *level, const char *prefix_str,
	       int prefix_type, int rowsize, int groupsize,
	       const void *buf, size_t len, bool ascii) {}
static inline void
print_hex_dump_bytes(const char *prefix_str, int prefix_type,
		     const void *buf, size_t len) {}
#endif

so the static strings of the call sites can be eliminated
when !CONFIG_PRINTK.


--

From: Joe Perches
Date: Tuesday, November 9, 2010 - 1:30 pm

Checking again, linux_proc_banner is only
used in one seq_printf and is not used in
a printk.

$ git grep -w linux_proc_banner *
fs/proc/version.c:      seq_printf(m, linux_proc_banner,
include/linux/logging.h:extern const char linux_proc_banner[];
init/version.c:const char linux_proc_banner[] =

I supposed it could be removed from kernel.h
or logging.h/printk.h and just declared extern
in fs/proc/version.c or moved to init.h or
maybe even version.h.

init.h already has other extern variables that
are declared in init/main.c so that might
make some sense.

--

From: Joe Perches
Date: Monday, November 15, 2010 - 11:04 am

Move the logging bits from kernel.h into printk.h so that
there is a bit more logical separation of the generic
kernel.h contents from the printk/logging specific parts.

Signed-off-by: Joe Perches <joe@perches.com>
---
V4: Update for dmesg_restrict
V3: Rename logging.h to printk.h
    Direct movement of lines from kernel.h to printk.h
    One trailing space deleted from a comment to keep git am quiet

Compiled x86 defconfig, allnoconfig, allmodconfig and CONFIG_PRINTK=n only

 include/linux/kernel.h |  245 +-----------------------------------------------
 include/linux/printk.h |  248 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 249 insertions(+), 244 deletions(-)
 create mode 100644 include/linux/printk.h

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fc3da9e..b6de9a6 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -17,13 +17,11 @@
 #include <linux/bitops.h>
 #include <linux/log2.h>
 #include <linux/typecheck.h>
+#include <linux/printk.h>
 #include <linux/dynamic_debug.h>
 #include <asm/byteorder.h>
 #include <asm/bug.h>
 
-extern const char linux_banner[];
-extern const char linux_proc_banner[];
-
 #define USHRT_MAX	((u16)(~0U))
 #define SHRT_MAX	((s16)(USHRT_MAX>>1))
 #define SHRT_MIN	((s16)(-SHRT_MAX - 1))
@@ -110,31 +108,6 @@ extern const char linux_proc_banner[];
  */
 #define lower_32_bits(n) ((u32)(n))
 
-#define	KERN_EMERG	"<0>"	/* system is unusable			*/
-#define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
-#define	KERN_CRIT	"<2>"	/* critical conditions			*/
-#define	KERN_ERR	"<3>"	/* error conditions			*/
-#define	KERN_WARNING	"<4>"	/* warning conditions			*/
-#define	KERN_NOTICE	"<5>"	/* normal but significant condition	*/
-#define	KERN_INFO	"<6>"	/* informational			*/
-#define	KERN_DEBUG	"<7>"	/* debug-level messages			*/
-
-/* Use the default kernel loglevel */
-#define KERN_DEFAULT	"<d>"
-/*
- * Annotation for a "continued" line of log printout (only done after a
- * ...
Previous thread: [PATCH] include/linux/kernel.h: Move logging bits to include/linux/logging.h by Joe Perches on Monday, November 8, 2010 - 10:37 pm. (1 message)

Next thread: A MMC card transfer issue by Tomoya MORINAGA on Monday, November 8, 2010 - 10:39 pm. (5 messages)