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/ECADFF3FD767C149AD96A924E7EA6EAF977A...

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

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

Thoughts?

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!