[PATCH] console_print: change the function api to make it have printk style interface

[PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

Hi Folks:

I bumped into this function in printk.c a while back. As I started using
it, I wondered if we could enhance it with printk style interface that
could make it somewhat more useful. 

One straightforward thing to do here would perhaps be to define this as
a macro. Something like this:

#define console_print(fmt,msg...) printk(KERN_EMERG fmt,##msg)

However, there might be some important reason why this was not done. I
do not know. 
Anyway, keeping the existing code of using a proper C function and
exporting the symbol, here's a patch that enhances the function to use
prink style interface. To the best of my knowledge, I do not think it
will break existing code that uses this function. I did a grep through
the kernel code and didn't look like this call was used in lot of other
places. I have also built the kernel locally with this change and looks
like at least for mips, it does not break our build.

CCing Linus for his opinion.

Cheers,

Ani


Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
---
 drivers/char/serial167.c |    2 +-
 include/linux/tty.h      |    2 +-
 kernel/printk.c          |   14 ++++++++++++--
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
index 51e7a46..029aa0d 100644
--- a/drivers/char/serial167.c
+++ b/drivers/char/serial167.c
@@ -171,7 +171,7 @@ static int startup(struct cyclades_port *);
 static void cy_throttle(struct tty_struct *);
 static void cy_unthrottle(struct tty_struct *);
 static void config_setup(struct cyclades_port *);
-extern void console_print(const char *);
+extern int console_print(const char *, ...);
 #ifdef CYCLOM_SHOW_STATUS
 static void show_status(int);
 #endif
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e8c6c91..eb3e4fe 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -519,7 +519,7 @@ extern int pcxe_open(struct tty_struct *tty, struct
file *filp);
 
 /* printk.c */
 
-extern void console_print(const char *);
+extern int console_print(const char *, ...);
 
 /* vt.c */
 
diff --git a/kernel/printk.c b/kernel/printk.c
index b4d97b5..729b0ff 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1060,9 +1060,19 @@ void __sched console_conditional_schedule(void)
 }
 EXPORT_SYMBOL(console_conditional_schedule);
 
-void console_print(const char *s)
+int console_print(const char *fmt, ...)
 {
-	printk(KERN_EMERG "%s", s);
+	va_list args;
+	int r;
+	char _fmt[1024] = KERN_EMERG;
+
+	strncat(_fmt, fmt,sizeof(_fmt)- sizeof(KERN_EMERG));
+
+	va_start(args, fmt);
+	r = vprintk(_fmt, args);
+	va_end(args);
+
+	return r;
 }
 EXPORT_SYMBOL(console_print);
 
-- 
1.6.4

Re: [PATCH] console_print: change the function api to make it have printk style interface

[Linus Torvalds]

On Tue, 25 Aug 2009, Anirban Sinha wrote:
>  
> -void console_print(const char *s)
> +int console_print(const char *fmt, ...)
>  {
> -	printk(KERN_EMERG "%s", s);
> +	va_list args;
> +	int r;
> +	char _fmt[1024] = KERN_EMERG;

Don't do this. That's a 1kB stack frame right there. You'll overflow the 
stack very quickly.

			Linus

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

>
>Don't do this. That's a 1kB stack frame right there. You'll overflow
the
>stack very quickly.

Ah! Of course you are correct! Can we simply have a macro instead of a
full blown function instead? If that is not acceptable, I will think of
something else to get around it.

Ani

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Linus Torvalds]

On Tue, 25 Aug 2009, Anirban Sinha wrote:
> 
> Ah! Of course you are correct! Can we simply have a macro instead of a
> full blown function instead? If that is not acceptable, I will think of
> something else to get around it.

A macro is what others are generally doing.

I'd suggest looking at "dev_printk()" as an example. Sure, it basically 
requires that the first argument be a string constant (so that the 
automatic concatenations of string constants in C does its job), so it's 
not a _generic_ solution, but it's a very powerful and simple one.

In other words, you'll likely have to change a few existing

	console_print(data);

to

	console_print("%s", data);

to make things work, but hey, not a big deal.

That said, quite frankly, I suspect an even better solution would be to 
just remove the whole 'console_print()' function entirely, and just make 
people use 'printk()' or 'dev_printk()' instead. The only real valid 
reason for that thing existing in the first place is likely the 
_assembler_ interface of doing a printout of an ASCII string (see 
arch/ia64/kernel/head.S).

The other uses of 'console_print()' seem totally pointless, really. It's a 
stupid legacy interface that shouldn't exist, I'm afraid.

		Linus

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

>A macro is what others are generally doing.


Linus, 

I completely agree with you on both points. When I first saw this
interface, the first question that popped in my mind was "why is it even
here?" And the very next question I asked myself was "why is this isn't
a macro?" 

As far as I can see, except for arch/ia64/kernel/head.S, among the codes
that actually uses this interface, others are wrapped within #if 0 ...
#endif blocks. So effectively, this is not being used at all :)


Cheers,

Ani

Re: [PATCH] console_print: change the function api to make it have printk style interface

[Randy Dunlap]

On Tue, 25 Aug 2009 16:47:49 -0700 Anirban Sinha wrote:

> Hi Folks:
> 
> I bumped into this function in printk.c a while back. As I started using
> it, I wondered if we could enhance it with printk style interface that
> could make it somewhat more useful. 
> 
> One straightforward thing to do here would perhaps be to define this as
> a macro. Something like this:
> 
> #define console_print(fmt,msg...) printk(KERN_EMERG fmt,##msg)
> 
> However, there might be some important reason why this was not done. I
> do not know. 
> Anyway, keeping the existing code of using a proper C function and
> exporting the symbol, here's a patch that enhances the function to use
> prink style interface. To the best of my knowledge, I do not think it
> will break existing code that uses this function. I did a grep through
> the kernel code and didn't look like this call was used in lot of other
> places. I have also built the kernel locally with this change and looks
> like at least for mips, it does not break our build.
> 
> CCing Linus for his opinion.
> 
> Cheers,
> 
> Ani
> 
> 
> Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
> ---
>  drivers/char/serial167.c |    2 +-
>  include/linux/tty.h      |    2 +-
>  kernel/printk.c          |   14 ++++++++++++--
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
> index 51e7a46..029aa0d 100644
> --- a/drivers/char/serial167.c
> +++ b/drivers/char/serial167.c
> @@ -171,7 +171,7 @@ static int startup(struct cyclades_port *);
>  static void cy_throttle(struct tty_struct *);
>  static void cy_unthrottle(struct tty_struct *);
>  static void config_setup(struct cyclades_port *);
> -extern void console_print(const char *);
> +extern int console_print(const char *, ...);

Drop that.  Add
#include <linux/tty.h>
if it's not already #included.


>  #ifdef CYCLOM_SHOW_STATUS
>  static void show_status(int);
>  #endif
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index e8c6c91..eb3e4fe 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -519,7 +519,7 @@ extern int pcxe_open(struct tty_struct *tty, struct
> file *filp);
>  
>  /* printk.c */
>  
> -extern void console_print(const char *);
> +extern int console_print(const char *, ...);

Make that prototype be like printk, with the __attribute__ ((format (printf, 1, 2))):


int printk(const char * fmt, ...)
	__attribute__ ((format (printf, 1, 2)));


>  
>  /* vt.c */
>  
> diff --git a/kernel/printk.c b/kernel/printk.c
> index b4d97b5..729b0ff 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1060,9 +1060,19 @@ void __sched console_conditional_schedule(void)
>  }
>  EXPORT_SYMBOL(console_conditional_schedule);
>  
> -void console_print(const char *s)
> +int console_print(const char *fmt, ...)
>  {
> -	printk(KERN_EMERG "%s", s);
> +	va_list args;
> +	int r;
> +	char _fmt[1024] = KERN_EMERG;
> +
> +	strncat(_fmt, fmt,sizeof(_fmt)- sizeof(KERN_EMERG));
> +
> +	va_start(args, fmt);
> +	r = vprintk(_fmt, args);
> +	va_end(args);
> +
> +	return r;
>  }
>  EXPORT_SYMBOL(console_print);
>  
> -- 


---
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

>
>Make that prototype be like printk, with the __attribute__ ((format
>(printf, 1, 2))):
>
>
>int printk(const char * fmt, ...)
>	__attribute__ ((format (printf, 1, 2)));
>
>
>

Thanks Randy. These are all very nice comments on the patch. However, I
do think that having a full blown C function is a overkill for such a
thing (except for the case which Linus has already pointed out - use
from assembly code). At the very least we should make it a macro if we
at all decide to keep the interface. I am also completely ok to remove
the interface altogether. The arcs that need that sort of function can
then implement an appropriate version (macro vs proper C function)
locally for it's use.

Since Linus has already agreed for the later, I will post a patch here
to remove the function. I am not very certain as to what we should do
about the only one single case of its genuine use - call from
ia64/head.S. What I have done is one solution. Please feel free to
debate/disagree.

Cheers,

Ani


Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
---
arch/ia64/kernel/head.S    |    1 +
arch/ia64/kernel/head.h    |    1 +
arch/ia64/kernel/process.c |    7 +++++++
drivers/char/serial167.c   |    5 ++---
include/linux/dtlk.h       |    6 +++---
include/linux/tty.h        |    4 ----
kernel/printk.c            |    6 ------
7 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index 23f846d..e6c5c3d 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -34,6 +34,7 @@
 #include <asm/mca_asm.h>
 #include <linux/init.h>
 #include <linux/linkage.h>
+#include "head.h"
 
 #ifdef CONFIG_HOTPLUG_CPU
 #define SAL_PSR_BITS_TO_SET				\
diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
new file mode 100644
index 0000000..2e2ac68
--- /dev/null
+++ b/arch/ia64/kernel/head.h
@@ -0,0 +1 @@
+extern void console_print(const char *s);
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 5d7c0e5..c2bb591 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -161,6 +161,13 @@ show_regs (struct pt_regs *regs)
 		show_stack(NULL, NULL);
 }
 
+/* local support for deprecated console_print */
+void 
+console_print(const char *s)
+{
+	printk(KERN_EMERG "%s", s);
+}
+
 void
 do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long
in_syscall)
 {
diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
index 51e7a46..5942a9d 100644
--- a/drivers/char/serial167.c
+++ b/drivers/char/serial167.c
@@ -171,7 +171,6 @@ static int startup(struct cyclades_port *);
 static void cy_throttle(struct tty_struct *);
 static void cy_unthrottle(struct tty_struct *);
 static void config_setup(struct cyclades_port *);
-extern void console_print(const char *);
 #ifdef CYCLOM_SHOW_STATUS
 static void show_status(int);
 #endif
@@ -245,7 +244,7 @@ void SP(char *data)
 {
 	unsigned long flags;
 	local_irq_save(flags);
-	console_print(data);
+	printk(KERN_EMERG "%s", data);
 	local_irq_restore(flags);
 }
 
@@ -255,7 +254,7 @@ void CP(char data)
 	unsigned long flags;
 	local_irq_save(flags);
 	scrn[0] = data;
-	console_print(scrn);
+	printk(KERN_EMERG "%c", scrn);
 	local_irq_restore(flags);
 }				/* CP */
 
diff --git a/include/linux/dtlk.h b/include/linux/dtlk.h
index 2896d90..4f93c45 100644
--- a/include/linux/dtlk.h
+++ b/include/linux/dtlk.h
@@ -4,15 +4,15 @@
 	{ \
 	  if(dtlk_trace) \
 	  { \
-	    console_print(text); \
-	    console_print("\n"); \
+	    printk(KERN_EMERG "%s", text);	\
+	    printk(KERN_EMERG "\n");		\
 	  } \
 	}
 
 #define TRACE_CHR(chr) \
 	{ \
 	  if(dtlk_trace) \
-	    console_print(chr); \
+	    printk(KERN_EMERG "%c", chr); \
 	} \
 
 #endif
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e8c6c91..ba191e3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -517,10 +517,6 @@ extern void serial_console_init(void);
 
 extern int pcxe_open(struct tty_struct *tty, struct file *filp);
 
-/* printk.c */
-
-extern void console_print(const char *);
-
 /* vt.c */
 
 extern int vt_ioctl(struct tty_struct *tty, struct file *file,
diff --git a/kernel/printk.c b/kernel/printk.c
index b4d97b5..32e15ce 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1060,12 +1060,6 @@ void __sched console_conditional_schedule(void)
 }
 EXPORT_SYMBOL(console_conditional_schedule);
 
-void console_print(const char *s)
-{
-	printk(KERN_EMERG "%s", s);
-}
-EXPORT_SYMBOL(console_print);
-
 void console_unblank(void)
 {
 	struct console *c;

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Linus Torvalds]

On Wed, 26 Aug 2009, Anirban Sinha wrote:
> 
> Since Linus has already agreed for the later, I will post a patch here
> to remove the function. I am not very certain as to what we should do
> about the only one single case of its genuine use - call from
> ia64/head.S. What I have done is one solution. Please feel free to
> debate/disagree.

Looks ok to me, but I'm not going to apply it in late -rc, since it 
doesn't really fix any bug. Feel free to re-send after 2.6.31 is out, or 
try to get it into one of the trees to be merged later.

		Linus

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

>Looks ok to me, but I'm not going to apply it in late -rc, since it
>doesn't really fix any bug. Feel free to re-send after 2.6.31 is out,
or
>try to get it into one of the trees to be merged later.
>

Thanks Linus. I will resend the patch once 2.6.31 is out of the door.

Ani

[PATCH] cleanup legacy console_print function

[Anirban Sinha]

Hi Linus:

>>Looks ok to me, but I'm not going to apply it in late -rc, since it
>>doesn't really fix any bug. Feel free to re-send after 2.6.31 is out,
>or
>>try to get it into one of the trees to be merged later.
>>
>
>Thanks Linus. I will resend the patch once 2.6.31 is out of the door.

Here's the patch again:


Subject: cleanup legacy console_print function

console_print() is an old legacy interface mostly unused in the entire
kernel tree. It's best to clean up its existing use and let developers
usew their own implementation of it as they feel fit.


Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
---
arch/ia64/kernel/head.S    |    1 +
arch/ia64/kernel/head.h    |    1 +
arch/ia64/kernel/process.c |    7 +++++++
drivers/char/serial167.c   |    5 ++---
include/linux/dtlk.h       |    6 +++---
include/linux/tty.h        |    4 ----
kernel/printk.c            |    6 ------
7 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index 23f846d..e6c5c3d 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -34,6 +34,7 @@
 #include <asm/mca_asm.h>
 #include <linux/init.h>
 #include <linux/linkage.h>
+#include "head.h"
 
 #ifdef CONFIG_HOTPLUG_CPU
 #define SAL_PSR_BITS_TO_SET				\
diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
new file mode 100644
index 0000000..2e2ac68
--- /dev/null
+++ b/arch/ia64/kernel/head.h
@@ -0,0 +1 @@
+extern void console_print(const char *s);
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 5d7c0e5..c2bb591 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -161,6 +161,13 @@ show_regs (struct pt_regs *regs)
 		show_stack(NULL, NULL);
 }
 
+/* local support for deprecated console_print */
+void 
+console_print(const char *s)
+{
+	printk(KERN_EMERG "%s", s);
+}
+
 void
 do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long
in_syscall)
 {
diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
index 51e7a46..5942a9d 100644
--- a/drivers/char/serial167.c
+++ b/drivers/char/serial167.c
@@ -171,7 +171,6 @@ static int startup(struct cyclades_port *);
 static void cy_throttle(struct tty_struct *);
 static void cy_unthrottle(struct tty_struct *);
 static void config_setup(struct cyclades_port *);
-extern void console_print(const char *);
 #ifdef CYCLOM_SHOW_STATUS
 static void show_status(int);
 #endif
@@ -245,7 +244,7 @@ void SP(char *data)
 {
 	unsigned long flags;
 	local_irq_save(flags);
-	console_print(data);
+	printk(KERN_EMERG "%s", data);
 	local_irq_restore(flags);
 }
 
@@ -255,7 +254,7 @@ void CP(char data)
 	unsigned long flags;
 	local_irq_save(flags);
 	scrn[0] = data;
-	console_print(scrn);
+	printk(KERN_EMERG "%c", scrn);
 	local_irq_restore(flags);
 }				/* CP */
 
diff --git a/include/linux/dtlk.h b/include/linux/dtlk.h
index 2896d90..4f93c45 100644
--- a/include/linux/dtlk.h
+++ b/include/linux/dtlk.h
@@ -4,15 +4,15 @@
 	{ \
 	  if(dtlk_trace) \
 	  { \
-	    console_print(text); \
-	    console_print("\n"); \
+	    printk(KERN_EMERG "%s", text);	\
+	    printk(KERN_EMERG "\n");		\
 	  } \
 	}
 
 #define TRACE_CHR(chr) \
 	{ \
 	  if(dtlk_trace) \
-	    console_print(chr); \
+	    printk(KERN_EMERG "%c", chr); \
 	} \
 
 #endif
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e8c6c91..ba191e3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -517,10 +517,6 @@ extern void serial_console_init(void);
 
 extern int pcxe_open(struct tty_struct *tty, struct file *filp);
 
-/* printk.c */
-
-extern void console_print(const char *);
-
 /* vt.c */
 
 extern int vt_ioctl(struct tty_struct *tty, struct file *file,
diff --git a/kernel/printk.c b/kernel/printk.c
index b4d97b5..32e15ce 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1060,12 +1060,6 @@ void __sched console_conditional_schedule(void)
 }
 EXPORT_SYMBOL(console_conditional_schedule);
 
-void console_print(const char *s)
-{
-	printk(KERN_EMERG "%s", s);
-}
-EXPORT_SYMBOL(console_print);
-
 void console_unblank(void)
 {
 	struct console *c;

Re: [PATCH] cleanup legacy console_print function

[Linus Torvalds]

On Fri, 11 Sep 2009, Anirban Sinha wrote:
>
> -           console_print(text); \
> -           console_print("\n"); \
> +           printk(KERN_EMERG "%s", text);      \
> +           printk(KERN_EMERG "\n");            \

Just clean it up to be

	printk(KERN_EMERG "%s\n", text);

instead while at it.

>  #define TRACE_CHR(chr) \
>  	{ \
>  	  if(dtlk_trace) \
> -	    console_print(chr); \
> +	    printk(KERN_EMERG "%c", chr); \

That can't be right. The translation is to use "%s", not "%c".

		Linus

RE: [PATCH] cleanup legacy console_print function

[Anirban Sinha]

>On Fri, 11 Sep 2009, Anirban Sinha wrote:
>>
>> -           console_print(text); \
>> -           console_print("\n"); \
>> +           printk(KERN_EMERG "%s", text);      \
>> +           printk(KERN_EMERG "\n");            \
>
>Just clean it up to be
>
>	printk(KERN_EMERG "%s\n", text);
>
>instead while at it.
>
>>  #define TRACE_CHR(chr) \
>>  	{ \
>>  	  if(dtlk_trace) \
>> -	    console_print(chr); \
>> +	    printk(KERN_EMERG "%c", chr); \
>
>That can't be right. The translation is to use "%s", not "%c".

Better still, for both the above cases, I will simply remove those macro
definitions. They are enclosed within {#if0 #endif}, so no code could
possibly be using it. If anyone has any strong reasons for keeping it
still, please raise your voice now.

Cheers,

Ani