[PATCH v1] printk: add dummy vprintk_emit for when CONFIG_PRINTK=n

[PATCH v1] printk: add dummy vprintk_emit for when CONFIG_PRINTK=n

[Brendan Higgins]

Previously vprintk_emit was only defined when CONFIG_PRINTK=y, this
caused a build failure in kunit/test.c when CONFIG_PRINTK was not set.
Add a no-op dummy so that callers don't have to ifdef around this.

Note: It has been suggested that this go in through the kselftest tree
along with the KUnit patches, because KUnit depends on this. See the
second link for the discussion on this.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
Link: https://lore.kernel.org/linux-kselftest/ECADFF3FD767C149AD96A924E7EA6EAF977A5D82@USCULXMSG01.am.sony.com/
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 include/linux/printk.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index cefd374c47b1..85b7970615a9 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -206,6 +206,13 @@ extern void printk_safe_init(void);
 extern void printk_safe_flush(void);
 extern void printk_safe_flush_on_panic(void);
 #else
+static inline __printf(5, 0)
+int vprintk_emit(int facility, int level,
+		 const char *dict, size_t dictlen,
+		 const char *fmt, va_list args)
+{
+	return 0;
+}
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)
 {
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH v1] printk: add dummy vprintk_emit for when CONFIG_PRINTK=n

[Randy Dunlap]

On 8/27/19 4:48 PM, Brendan Higgins wrote:
> Previously vprintk_emit was only defined when CONFIG_PRINTK=y, this
> caused a build failure in kunit/test.c when CONFIG_PRINTK was not set.
> Add a no-op dummy so that callers don't have to ifdef around this.
> 
> Note: It has been suggested that this go in through the kselftest tree
> along with the KUnit patches, because KUnit depends on this. See the
> second link for the discussion on this.
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> Link: https://lore.kernel.org/linux-kselftest/ECADFF3FD767C149AD96A924E7EA6EAF977A5D82@USCULXMSG01.am.sony.com/
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>

Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested

Thanks.

> ---
>  include/linux/printk.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index cefd374c47b1..85b7970615a9 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -206,6 +206,13 @@ extern void printk_safe_init(void);
>  extern void printk_safe_flush(void);
>  extern void printk_safe_flush_on_panic(void);
>  #else
> +static inline __printf(5, 0)
> +int vprintk_emit(int facility, int level,
> +		 const char *dict, size_t dictlen,
> +		 const char *fmt, va_list args)
> +{
> +	return 0;
> +}
>  static inline __printf(1, 0)
>  int vprintk(const char *s, va_list args)
>  {
> 


-- 
~Randy

Re: [PATCH v1] printk: add dummy vprintk_emit for when CONFIG_PRINTK=n

[Sergey Senozhatsky]

On (08/27/19 16:48), Brendan Higgins wrote:
> Previously vprintk_emit was only defined when CONFIG_PRINTK=y, this
> caused a build failure in kunit/test.c when CONFIG_PRINTK was not set.
> Add a no-op dummy so that callers don't have to ifdef around this.
> 
> Note: It has been suggested that this go in through the kselftest tree
> along with the KUnit patches, because KUnit depends on this. See the
> second link for the discussion on this.

Is there any reason for kunit to use vprintk_emit()? Can you switch
to pr_err()/pr_info()/pr_foo()?

vprintk_emit() function is pretty internal. It's not static because
drivers/base/core.c wants to add some extra payload to printk()
messages (extended headers, etc).

	-ss

Re: [PATCH v1] printk: add dummy vprintk_emit for when CONFIG_PRINTK=n

[Brendan Higgins]

On Wed, Aug 28, 2019 at 12:02:31PM +0900, Sergey Senozhatsky wrote:
> On (08/27/19 16:48), Brendan Higgins wrote:
> > Previously vprintk_emit was only defined when CONFIG_PRINTK=y, this
> > caused a build failure in kunit/test.c when CONFIG_PRINTK was not set.
> > Add a no-op dummy so that callers don't have to ifdef around this.
> > 
> > Note: It has been suggested that this go in through the kselftest tree
> > along with the KUnit patches, because KUnit depends on this. See the
> > second link for the discussion on this.
> 
> Is there any reason for kunit to use vprintk_emit()? Can you switch
> to pr_err()/pr_info()/pr_foo()?
> 
> vprintk_emit() function is pretty internal. It's not static because
> drivers/base/core.c wants to add some extra payload to printk()
> messages (extended headers, etc).

I actually use it in a very similar way as dev_printk() does. I am using
it to define an equivalent kunit_printk(), which takes a log level, and
adds its own test information to the log.

What I have now is:

static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
{
	return vprintk_emit(0, level, NULL, 0, fmt, args);
}

static int kunit_printk_emit(int level, const char *fmt, ...)
{
	va_list args;
	int ret;

	va_start(args, fmt);
	ret = kunit_vprintk_emit(level, fmt, args);
	va_end(args);

	return ret;
}

static void kunit_vprintk(const struct kunit *test,
			  const char *level,
			  struct va_format *vaf)
{
	kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
}

The closest thing I can do without vprintk_emit is:

static void kunit_vprintk(const struct kunit *test,
			  const char *level,
			  struct va_format *vaf)
{
	printk("%s\t# %s: %pV", level, test->name, vaf);
}

But checkpatch complains:

WARNING: printk() should include KERN_<LEVEL> facility level

Based on the printk() implementation, it looks like it should be fine to
provide the level via format string since the formatting is performed
before the log level is checked; nevertheless, it seemed to me like
vprintk_emit was closer to what I wanted (again, it's what dev_printk
uses). Shuah and Tim seemed to agree with me:

https://lore.kernel.org/linux-kselftest/ECADFF3FD767C149AD96A924E7EA6EAF977A5D82@USCULXMSG01.am.sony.com/

Nevertheless, I probably don't want add any custom dict entries.

Really, what I think I want is a printk_level().

Thoughts?

Re: [PATCH v1] printk: add dummy vprintk_emit for when CONFIG_PRINTK=n

[Sergey Senozhatsky]

On (08/27/19 21:45), Brendan Higgins wrote:
[..]
> I actually use it in a very similar way as dev_printk() does. I am using
> it to define an equivalent kunit_printk(), which takes a log level, and
> adds its own test information to the log.
> 
> What I have now is:
> 
> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
> 	return vprintk_emit(0, level, NULL, 0, fmt, args);
> }
> 
> static int kunit_printk_emit(int level, const char *fmt, ...)
> {
> 	va_list args;
> 	int ret;
> 
> 	va_start(args, fmt);
> 	ret = kunit_vprintk_emit(level, fmt, args);
> 	va_end(args);
> 
> 	return ret;
> }
> 
> static void kunit_vprintk(const struct kunit *test,
> 			  const char *level,
> 			  struct va_format *vaf)
> {
> 	kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
> }

Basically, for prefixes we have pr_fmt().

#define pr_fmt(fmt) "module name: " fmt

but pr_fmt() is mostly for static prefixes. If that doesn't work for
you, then maybe you can tweak kunit_foo() macros?

E.g. something like this

#define kunit_info(test, fmt, ...)                                   \
	printk(KERN_INFO "\t# %s: " pr_fmt(fmt), (test)->name, ##__VA_ARGS__)

#define kunit_err(test, fmt, ...)                                    \
	printk(KERN_ERR "\t# %s: " pr_fmt(fmt), (test)->name, ##__VA_ARGS__)

#define kunit_debug(test, fmt, ...)                                  \
	printk(KERN_DEBUG "\t# %s: " pr_fmt(fmt), (test)->name, ##__VA_ARGS__)

Would that do the trick? Am I missing something?

	-ss

Re: [PATCH v1] printk: add dummy vprintk_emit for when CONFIG_PRINTK=n

[Brendan Higgins]

On Tue, Aug 27, 2019 at 10:24 PM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (08/27/19 21:45), Brendan Higgins wrote:
> [..]
> > I actually use it in a very similar way as dev_printk() does. I am using
> > it to define an equivalent kunit_printk(), which takes a log level, and
> > adds its own test information to the log.
> >
> > What I have now is:
> >
> > static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > {
> >       return vprintk_emit(0, level, NULL, 0, fmt, args);
> > }
> >
> > static int kunit_printk_emit(int level, const char *fmt, ...)
> > {
> >       va_list args;
> >       int ret;
> >
> >       va_start(args, fmt);
> >       ret = kunit_vprintk_emit(level, fmt, args);
> >       va_end(args);
> >
> >       return ret;
> > }
> >
> > static void kunit_vprintk(const struct kunit *test,
> >                         const char *level,
> >                         struct va_format *vaf)
> > {
> >       kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
> > }
>
> Basically, for prefixes we have pr_fmt().
>
> #define pr_fmt(fmt) "module name: " fmt
>
> but pr_fmt() is mostly for static prefixes. If that doesn't work for
> you, then maybe you can tweak kunit_foo() macros?

That doesn't work. The prefix is dynamic.

> E.g. something like this
>
> #define kunit_info(test, fmt, ...)                                   \
>         printk(KERN_INFO "\t# %s: " pr_fmt(fmt), (test)->name, ##__VA_ARGS__)
>
> #define kunit_err(test, fmt, ...)                                    \
>         printk(KERN_ERR "\t# %s: " pr_fmt(fmt), (test)->name, ##__VA_ARGS__)
>
> #define kunit_debug(test, fmt, ...)                                  \
>         printk(KERN_DEBUG "\t# %s: " pr_fmt(fmt), (test)->name, ##__VA_ARGS__)
>
> Would that do the trick? Am I missing something?

This appears to work. I will send out a patch that incorporates this.

Thanks!