Previously KUnit assumed that printk would always be present, which is
not a valid assumption to make. Fix that by removing call to
vprintk_emit, and calling printk directly.
Reported-by: Randy Dunlap <[email protected]>
Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
Cc: Stephen Rothwell <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Signed-off-by: Brendan Higgins <[email protected]>
---
include/kunit/test.h | 11 ++++-----
kunit/test.c | 57 +++++---------------------------------------
2 files changed, 11 insertions(+), 57 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 8b7eb03d4971..efad2eacd6ba 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -339,9 +339,8 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
void kunit_cleanup(struct kunit *test);
-void __printf(3, 4) kunit_printk(const char *level,
- const struct kunit *test,
- const char *fmt, ...);
+#define kunit_print_level(KERN_LEVEL, test, fmt, ...) \
+ printk(KERN_LEVEL "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
/**
* kunit_info() - Prints an INFO level message associated with @test.
@@ -353,7 +352,7 @@ void __printf(3, 4) kunit_printk(const char *level,
* Takes a variable number of format parameters just like printk().
*/
#define kunit_info(test, fmt, ...) \
- kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
+ kunit_print_level(KERN_INFO, test, fmt, ##__VA_ARGS__)
/**
* kunit_warn() - Prints a WARN level message associated with @test.
@@ -364,7 +363,7 @@ void __printf(3, 4) kunit_printk(const char *level,
* Prints a warning level message.
*/
#define kunit_warn(test, fmt, ...) \
- kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
+ kunit_print_level(KERN_WARNING, test, fmt, ##__VA_ARGS__)
/**
* kunit_err() - Prints an ERROR level message associated with @test.
@@ -375,7 +374,7 @@ void __printf(3, 4) kunit_printk(const char *level,
* Prints an error level message.
*/
#define kunit_err(test, fmt, ...) \
- kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
+ kunit_print_level(KERN_ERR, test, fmt, ##__VA_ARGS__)
/**
* KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
diff --git a/kunit/test.c b/kunit/test.c
index b2ca9b94c353..c83c0fa59cbd 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -16,36 +16,12 @@ static void kunit_set_failure(struct kunit *test)
WRITE_ONCE(test->success, false);
}
-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);
-}
-
static void kunit_print_tap_version(void)
{
static bool kunit_has_printed_tap_version;
if (!kunit_has_printed_tap_version) {
- kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
+ pr_info("TAP version 14\n");
kunit_has_printed_tap_version = true;
}
}
@@ -64,10 +40,8 @@ static size_t kunit_test_cases_len(struct kunit_case *test_cases)
static void kunit_print_subtest_start(struct kunit_suite *suite)
{
kunit_print_tap_version();
- kunit_printk_emit(LOGLEVEL_INFO, "\t# Subtest: %s\n", suite->name);
- kunit_printk_emit(LOGLEVEL_INFO,
- "\t1..%zd\n",
- kunit_test_cases_len(suite->test_cases));
+ pr_info("\t# Subtest: %s\n", suite->name);
+ pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
}
static void kunit_print_ok_not_ok(bool should_indent,
@@ -87,9 +61,7 @@ static void kunit_print_ok_not_ok(bool should_indent,
else
ok_not_ok = "not ok";
- kunit_printk_emit(LOGLEVEL_INFO,
- "%s%s %zd - %s\n",
- indent, ok_not_ok, test_number, description);
+ pr_info("%s%s %zd - %s\n", indent, ok_not_ok, test_number, description);
}
static bool kunit_suite_has_succeeded(struct kunit_suite *suite)
@@ -133,11 +105,11 @@ static void kunit_print_string_stream(struct kunit *test,
kunit_err(test,
"Could not allocate buffer, dumping stream:\n");
list_for_each_entry(fragment, &stream->fragments, node) {
- kunit_err(test, fragment->fragment);
+ kunit_err(test, "%s", fragment->fragment);
}
kunit_err(test, "\n");
} else {
- kunit_err(test, buf);
+ kunit_err(test, "%s", buf);
kunit_kfree(test, buf);
}
}
@@ -504,20 +476,3 @@ void kunit_cleanup(struct kunit *test)
kunit_resource_free(test, resource);
}
}
-
-void kunit_printk(const char *level,
- const struct kunit *test,
- const char *fmt, ...)
-{
- struct va_format vaf;
- va_list args;
-
- va_start(args, fmt);
-
- vaf.fmt = fmt;
- vaf.va = &args;
-
- kunit_vprintk(test, level, &vaf);
-
- va_end(args);
-}
--
2.23.0.187.g17f5b7556c-goog
On (08/28/19 02:31), Brendan Higgins wrote:
[..]
> Previously KUnit assumed that printk would always be present, which is
> not a valid assumption to make. Fix that by removing call to
> vprintk_emit, and calling printk directly.
>
> Reported-by: Randy Dunlap <[email protected]>
> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> Cc: Stephen Rothwell <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Signed-off-by: Brendan Higgins <[email protected]>
[..]
> -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);
> -}
This patch looks good to me. I like the removal of recursive
vsprintf() (%pV).
-ss
On Wed 2019-08-28 18:49:29, Sergey Senozhatsky wrote:
> On (08/28/19 02:31), Brendan Higgins wrote:
> [..]
> > Previously KUnit assumed that printk would always be present, which is
> > not a valid assumption to make. Fix that by removing call to
> > vprintk_emit, and calling printk directly.
> >
> > Reported-by: Randy Dunlap <[email protected]>
> > Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> > Cc: Stephen Rothwell <[email protected]>
> > Cc: Sergey Senozhatsky <[email protected]>
> > Signed-off-by: Brendan Higgins <[email protected]>
>
> [..]
>
> > -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);
> > -}
>
> This patch looks good to me. I like the removal of recursive
> vsprintf() (%pV).
Same here. And I am happy that we did not add more external
vprintk_emit() callers. It would be great to rework dev_printk()
as well.
Best Regards,
Petr
On 8/28/19 2:31 AM, Brendan Higgins wrote:
> Previously KUnit assumed that printk would always be present, which is
> not a valid assumption to make. Fix that by removing call to
> vprintk_emit, and calling printk directly.
>
> Reported-by: Randy Dunlap <[email protected]>
> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> Cc: Stephen Rothwell <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Signed-off-by: Brendan Higgins <[email protected]>
> ---
> include/kunit/test.h | 11 ++++-----
> kunit/test.c | 57 +++++---------------------------------------
> 2 files changed, 11 insertions(+), 57 deletions(-)
Acked-by: Randy Dunlap <[email protected]> # build-tested
thanks.
--
~Randy
On 8/28/19 3:49 AM, Sergey Senozhatsky wrote:
> On (08/28/19 02:31), Brendan Higgins wrote:
> [..]
>> Previously KUnit assumed that printk would always be present, which is
>> not a valid assumption to make. Fix that by removing call to
>> vprintk_emit, and calling printk directly.
>>
>> Reported-by: Randy Dunlap <[email protected]>
>> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
>> Cc: Stephen Rothwell <[email protected]>
>> Cc: Sergey Senozhatsky <[email protected]>
>> Signed-off-by: Brendan Higgins <[email protected]>
>
> [..]
>
>> -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);
>> -}
>
> This patch looks good to me. I like the removal of recursive
> vsprintf() (%pV).
>
> -ss
>
Hi Sergey,
What are the guidelines for using printk(). I recall some discussion
about not using printk(). I am seeing the following from checkpatch
script:
WARNING: Prefer [subsystem eg: netdev]_level([subsystem]dev, ... then
dev_level(dev, ... then pr_level(... to printk(KERN_LEVEL ...
#105: FILE: include/kunit/test.h:343:
+ printk(KERN_LEVEL "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
Is there supposed to be pr_level() - I can find dev_level()
cc'ing Joe Perches for his feedback on this message recommending
pr_level() which isn't in 5.3.
thanks,
-- Shuah
thanks,
-- Shuah
On Thu, 2019-08-29 at 11:01 -0600, shuah wrote:
> On 8/28/19 3:49 AM, Sergey Senozhatsky wrote:
> > On (08/28/19 02:31), Brendan Higgins wrote:
> > [..]
> > > Previously KUnit assumed that printk would always be present, which is
> > > not a valid assumption to make. Fix that by removing call to
> > > vprintk_emit, and calling printk directly.
> > >
> > > Reported-by: Randy Dunlap <[email protected]>
> > > Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> > > Cc: Stephen Rothwell <[email protected]>
> > > Cc: Sergey Senozhatsky <[email protected]>
> > > Signed-off-by: Brendan Higgins <[email protected]>
> >
> > [..]
> >
> > > -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);
> > > -}
> >
> > This patch looks good to me. I like the removal of recursive
> > vsprintf() (%pV).
> >
> > -ss
> >
>
> Hi Sergey,
>
> What are the guidelines for using printk(). I recall some discussion
> about not using printk(). I am seeing the following from checkpatch
> script:
>
>
> WARNING: Prefer [subsystem eg: netdev]_level([subsystem]dev, ... then
> dev_level(dev, ... then pr_level(... to printk(KERN_LEVEL ...
> #105: FILE: include/kunit/test.h:343:
> + printk(KERN_LEVEL "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
>
>
> Is there supposed to be pr_level() - I can find dev_level()
>
> cc'ing Joe Perches for his feedback on this message recommending
> pr_level() which isn't in 5.3.
I don't care for pr_level or KERN_LEVEL in a printk.
I think this is somewhat overly complicated.
I think I'd write it like:
---
include/kunit/test.h | 11 ++++-----
kunit/test.c | 69 ++++++++++++++++------------------------------------
2 files changed, 26 insertions(+), 54 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 8b7eb03d4971..aa4abf0a22a5 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -339,9 +339,8 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
void kunit_cleanup(struct kunit *test);
-void __printf(3, 4) kunit_printk(const char *level,
- const struct kunit *test,
- const char *fmt, ...);
+__printf(2, 3)
+void kunit_printk(const struct kunit *test, const char *fmt, ...);
/**
* kunit_info() - Prints an INFO level message associated with @test.
@@ -353,7 +352,7 @@ void __printf(3, 4) kunit_printk(const char *level,
* Takes a variable number of format parameters just like printk().
*/
#define kunit_info(test, fmt, ...) \
- kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
+ kunit_printk(test, KERN_INFO fmt, ##__VA_ARGS__)
/**
* kunit_warn() - Prints a WARN level message associated with @test.
@@ -364,7 +363,7 @@ void __printf(3, 4) kunit_printk(const char *level,
* Prints a warning level message.
*/
#define kunit_warn(test, fmt, ...) \
- kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
+ kunit_printk(test, KERN_WARNING fmt, ##__VA_ARGS__)
/**
* kunit_err() - Prints an ERROR level message associated with @test.
@@ -375,7 +374,7 @@ void __printf(3, 4) kunit_printk(const char *level,
* Prints an error level message.
*/
#define kunit_err(test, fmt, ...) \
- kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
+ kunit_printk(test, KERN_ERR fmt, ##__VA_ARGS__)
/**
* KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
diff --git a/kunit/test.c b/kunit/test.c
index b2ca9b94c353..ddb9bffb5a5d 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -16,40 +16,6 @@ static void kunit_set_failure(struct kunit *test)
WRITE_ONCE(test->success, false);
}
-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);
-}
-
-static void kunit_print_tap_version(void)
-{
- static bool kunit_has_printed_tap_version;
-
- if (!kunit_has_printed_tap_version) {
- kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
- kunit_has_printed_tap_version = true;
- }
-}
-
static size_t kunit_test_cases_len(struct kunit_case *test_cases)
{
struct kunit_case *test_case;
@@ -63,11 +29,9 @@ static size_t kunit_test_cases_len(struct kunit_case *test_cases)
static void kunit_print_subtest_start(struct kunit_suite *suite)
{
- kunit_print_tap_version();
- kunit_printk_emit(LOGLEVEL_INFO, "\t# Subtest: %s\n", suite->name);
- kunit_printk_emit(LOGLEVEL_INFO,
- "\t1..%zd\n",
- kunit_test_cases_len(suite->test_cases));
+ pr_info_once("TAP version 14\n");
+ pr_info("\t# Subtest: %s\n", suite->name);
+ pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
}
static void kunit_print_ok_not_ok(bool should_indent,
@@ -87,9 +51,8 @@ static void kunit_print_ok_not_ok(bool should_indent,
else
ok_not_ok = "not ok";
- kunit_printk_emit(LOGLEVEL_INFO,
- "%s%s %zd - %s\n",
- indent, ok_not_ok, test_number, description);
+ pr_info("%s%s %zd - %s\n",
+ indent, ok_not_ok, test_number, description);
}
static bool kunit_suite_has_succeeded(struct kunit_suite *suite)
@@ -133,11 +96,11 @@ static void kunit_print_string_stream(struct kunit *test,
kunit_err(test,
"Could not allocate buffer, dumping stream:\n");
list_for_each_entry(fragment, &stream->fragments, node) {
- kunit_err(test, fragment->fragment);
+ kunit_err(test, "%s", fragment->fragment);
}
kunit_err(test, "\n");
} else {
- kunit_err(test, buf);
+ kunit_err(test, "%s", buf);
kunit_kfree(test, buf);
}
}
@@ -505,19 +468,29 @@ void kunit_cleanup(struct kunit *test)
}
}
-void kunit_printk(const char *level,
- const struct kunit *test,
- const char *fmt, ...)
+void kunit_printk(const struct kunit *test, const char *fmt, ...)
{
+ char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
struct va_format vaf;
va_list args;
+ int kern_level;
va_start(args, fmt);
+ while ((kern_level = printk_get_level(fmt)) != 0) {
+ size_t size = printk_skip_level(fmt) - fmt;
+
+ if (kern_level >= '0' && kern_level <= '7') {
+ memcpy(lvl, fmt, size);
+ lvl[size] = '\0';
+ }
+ fmt += size;
+ }
+
vaf.fmt = fmt;
vaf.va = &args;
- kunit_vprintk(test, level, &vaf);
+ printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
va_end(args);
}
On Thu, 2019-08-29 at 21:44 -0700, Joe Perches wrote:
> On Thu, 2019-08-29 at 11:01 -0600, shuah wrote:
[]
> > WARNING: Prefer [subsystem eg: netdev]_level([subsystem]dev, ... then
> > dev_level(dev, ... then pr_level(... to printk(KERN_LEVEL ...
> > #105: FILE: include/kunit/test.h:343:
> > + printk(KERN_LEVEL "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
> >
> >
> > Is there supposed to be pr_level() - I can find dev_level()
btw: the checkpatch message is meant to be interpreted as
Prefer [subsystem eg: netdev]_<level>([subsystem]dev, ...) then dev_<level>(dev, ...) then pr_<level>(...), to printk(KERN_<LEVEL> ...)
btw2:
dev_level is actually not a function, but a convenience macro argument
which indirects to an actual specific logging function.
So no, there is not supposed to be a pr_level.
On (08/29/19 11:01), shuah wrote:
[..]
> Hi Sergey,
>
> What are the guidelines for using printk(). I recall some discussion
> about not using printk(). I am seeing the following from checkpatch
> script:
Hello,
> WARNING: Prefer [subsystem eg: netdev]_level([subsystem]dev, ... then
> dev_level(dev, ... then pr_level(... to printk(KERN_LEVEL ...
> #105: FILE: include/kunit/test.h:343:
> + printk(KERN_LEVEL "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
>
Oh, right.
So we sort of want people to use pr_err()/pr_info()/pr_"level"()
because, otherwise, when people use plain printk(), they tend to
forget to add KERN_LEVEL.
In kunit case everything looks fine. KERN_LEVEL is there so I'm
fine with the patch.
You still can switch to pr_info()/pr_err()/pr_etc, just to make
checkpatch happier, but that's up to you.
> Is there supposed to be pr_level() - I can find dev_level()
No, not really. pr_level() stands for pr_"debug"()/pr_"info"()/etc.
E.g.
#define pr_emerg(fmt, ...) \
printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
#define pr_alert(fmt, ...) \
printk(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
#define pr_crit(fmt, ...) \
printk(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
#define pr_err(fmt, ...) \
printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
#define pr_warning(fmt, ...) \
printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
#define pr_warn pr_warning
#define pr_notice(fmt, ...) \
printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
#define pr_info(fmt, ...) \
printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
-ss
On Thu, Aug 29, 2019 at 09:44:58PM -0700, Joe Perches wrote:
> On Thu, 2019-08-29 at 11:01 -0600, shuah wrote:
> > On 8/28/19 3:49 AM, Sergey Senozhatsky wrote:
> > > On (08/28/19 02:31), Brendan Higgins wrote:
> > > [..]
> > > > Previously KUnit assumed that printk would always be present, which is
> > > > not a valid assumption to make. Fix that by removing call to
> > > > vprintk_emit, and calling printk directly.
> > > >
> > > > Reported-by: Randy Dunlap <[email protected]>
> > > > Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> > > > Cc: Stephen Rothwell <[email protected]>
> > > > Cc: Sergey Senozhatsky <[email protected]>
> > > > Signed-off-by: Brendan Higgins <[email protected]>
> > >
> > > [..]
> > >
> > > > -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);
> > > > -}
> > >
> > > This patch looks good to me. I like the removal of recursive
> > > vsprintf() (%pV).
> > >
> > > -ss
> > >
> >
> > Hi Sergey,
> >
> > What are the guidelines for using printk(). I recall some discussion
> > about not using printk(). I am seeing the following from checkpatch
> > script:
> >
> >
> > WARNING: Prefer [subsystem eg: netdev]_level([subsystem]dev, ... then
> > dev_level(dev, ... then pr_level(... to printk(KERN_LEVEL ...
> > #105: FILE: include/kunit/test.h:343:
> > + printk(KERN_LEVEL "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
> >
> >
> > Is there supposed to be pr_level() - I can find dev_level()
> >
> > cc'ing Joe Perches for his feedback on this message recommending
> > pr_level() which isn't in 5.3.
>
> I don't care for pr_level or KERN_LEVEL in a printk.
I don't think I follow, how does your version fix this?
> I think this is somewhat overly complicated.
>
> I think I'd write it like:
> ---
> include/kunit/test.h | 11 ++++-----
> kunit/test.c | 69 ++++++++++++++++------------------------------------
> 2 files changed, 26 insertions(+), 54 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 8b7eb03d4971..aa4abf0a22a5 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -339,9 +339,8 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>
> void kunit_cleanup(struct kunit *test);
>
> -void __printf(3, 4) kunit_printk(const char *level,
> - const struct kunit *test,
> - const char *fmt, ...);
> +__printf(2, 3)
> +void kunit_printk(const struct kunit *test, const char *fmt, ...);
>
> /**
> * kunit_info() - Prints an INFO level message associated with @test.
> @@ -353,7 +352,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> * Takes a variable number of format parameters just like printk().
> */
> #define kunit_info(test, fmt, ...) \
> - kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
> + kunit_printk(test, KERN_INFO fmt, ##__VA_ARGS__)
>
> /**
> * kunit_warn() - Prints a WARN level message associated with @test.
> @@ -364,7 +363,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> * Prints a warning level message.
> */
> #define kunit_warn(test, fmt, ...) \
> - kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
> + kunit_printk(test, KERN_WARNING fmt, ##__VA_ARGS__)
>
> /**
> * kunit_err() - Prints an ERROR level message associated with @test.
> @@ -375,7 +374,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> * Prints an error level message.
> */
> #define kunit_err(test, fmt, ...) \
> - kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
> + kunit_printk(test, KERN_ERR fmt, ##__VA_ARGS__)
>
> /**
> * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> diff --git a/kunit/test.c b/kunit/test.c
> index b2ca9b94c353..ddb9bffb5a5d 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
> @@ -16,40 +16,6 @@ static void kunit_set_failure(struct kunit *test)
> WRITE_ONCE(test->success, false);
> }
>
> -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);
> -}
> -
> -static void kunit_print_tap_version(void)
> -{
> - static bool kunit_has_printed_tap_version;
> -
> - if (!kunit_has_printed_tap_version) {
> - kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
> - kunit_has_printed_tap_version = true;
> - }
> -}
> -
> static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> {
> struct kunit_case *test_case;
> @@ -63,11 +29,9 @@ static size_t kunit_test_cases_len(struct kunit_case *test_cases)
>
> static void kunit_print_subtest_start(struct kunit_suite *suite)
> {
> - kunit_print_tap_version();
> - kunit_printk_emit(LOGLEVEL_INFO, "\t# Subtest: %s\n", suite->name);
> - kunit_printk_emit(LOGLEVEL_INFO,
> - "\t1..%zd\n",
> - kunit_test_cases_len(suite->test_cases));
> + pr_info_once("TAP version 14\n");
> + pr_info("\t# Subtest: %s\n", suite->name);
> + pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
> }
>
> static void kunit_print_ok_not_ok(bool should_indent,
> @@ -87,9 +51,8 @@ static void kunit_print_ok_not_ok(bool should_indent,
> else
> ok_not_ok = "not ok";
>
> - kunit_printk_emit(LOGLEVEL_INFO,
> - "%s%s %zd - %s\n",
> - indent, ok_not_ok, test_number, description);
> + pr_info("%s%s %zd - %s\n",
> + indent, ok_not_ok, test_number, description);
> }
>
> static bool kunit_suite_has_succeeded(struct kunit_suite *suite)
> @@ -133,11 +96,11 @@ static void kunit_print_string_stream(struct kunit *test,
> kunit_err(test,
> "Could not allocate buffer, dumping stream:\n");
> list_for_each_entry(fragment, &stream->fragments, node) {
> - kunit_err(test, fragment->fragment);
> + kunit_err(test, "%s", fragment->fragment);
> }
> kunit_err(test, "\n");
> } else {
> - kunit_err(test, buf);
> + kunit_err(test, "%s", buf);
> kunit_kfree(test, buf);
> }
> }
> @@ -505,19 +468,29 @@ void kunit_cleanup(struct kunit *test)
> }
> }
>
> -void kunit_printk(const char *level,
> - const struct kunit *test,
> - const char *fmt, ...)
> +void kunit_printk(const struct kunit *test, const char *fmt, ...)
> {
> + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
> struct va_format vaf;
> va_list args;
> + int kern_level;
>
> va_start(args, fmt);
>
> + while ((kern_level = printk_get_level(fmt)) != 0) {
> + size_t size = printk_skip_level(fmt) - fmt;
> +
> + if (kern_level >= '0' && kern_level <= '7') {
> + memcpy(lvl, fmt, size);
> + lvl[size] = '\0';
> + }
> + fmt += size;
> + }
> +
> vaf.fmt = fmt;
> vaf.va = &args;
>
> - kunit_vprintk(test, level, &vaf);
> + printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
>
> va_end(args);
> }
How is this simpler?
If we are okay with dynamically adding the KERN_<LEVEL> and %pV (and I
don't think that Sergey is), then wouldn't it be easier to pass in the
kernel level as a separate parameter and then strip off all printk
headers like this:
void kunit_printk(const char *level,
const struct kunit *test,
const char *fmt, ...)
{
struct va_format vaf;
va_list args;
va_start(args, fmt);
+ fmt = printk_skip_headers(fmt);
+
vaf.fmt = fmt;
vaf.va = &args;
- kunit_vprintk(test, level, &vaf);
+ printk("%s\t# %s %pV\n", level, test->name, &vaf);
va_end(args);
}
Then the kunit_printk function is much simpler, and I don't think my
header file has to change at all.
I don't know. I am clearly not an expert on this topic, but I don't see
the merit of the while loop you added above or dropping the level param.
On Fri, 2019-08-30 at 11:38 -0700, Brendan Higgins wrote:
> On Thu, Aug 29, 2019 at 09:44:58PM -0700, Joe Perches wrote:
> > On Thu, 2019-08-29 at 11:01 -0600, shuah wrote:
> > > On 8/28/19 3:49 AM, Sergey Senozhatsky wrote:
> > > > On (08/28/19 02:31), Brendan Higgins wrote:
> > > > [..]
> > > > > Previously KUnit assumed that printk would always be present, which is
> > > > > not a valid assumption to make. Fix that by removing call to
> > > > > vprintk_emit, and calling printk directly.
> > > > >
> > > > > Reported-by: Randy Dunlap <[email protected]>
> > > > > Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> > > > > Cc: Stephen Rothwell <[email protected]>
> > > > > Cc: Sergey Senozhatsky <[email protected]>
> > > > > Signed-off-by: Brendan Higgins <[email protected]>
> > > >
> > > > [..]
> > > >
> > > > > -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);
> > > > > -}
> > > >
> > > > This patch looks good to me. I like the removal of recursive
> > > > vsprintf() (%pV).
> > > >
> > > > -ss
> > > >
> > >
> > > Hi Sergey,
> > >
> > > What are the guidelines for using printk(). I recall some discussion
> > > about not using printk(). I am seeing the following from checkpatch
> > > script:
> > >
> > >
> > > WARNING: Prefer [subsystem eg: netdev]_level([subsystem]dev, ... then
> > > dev_level(dev, ... then pr_level(... to printk(KERN_LEVEL ...
> > > #105: FILE: include/kunit/test.h:343:
> > > + printk(KERN_LEVEL "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
> > >
> > >
> > > Is there supposed to be pr_level() - I can find dev_level()
> > >
> > > cc'ing Joe Perches for his feedback on this message recommending
> > > pr_level() which isn't in 5.3.
> >
> > I don't care for pr_level or KERN_LEVEL in a printk.
>
> I don't think I follow, how does your version fix this?
>
> > I think this is somewhat overly complicated.
> >
> > I think I'd write it like:
> > ---
> > include/kunit/test.h | 11 ++++-----
> > kunit/test.c | 69 ++++++++++++++++------------------------------------
> > 2 files changed, 26 insertions(+), 54 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 8b7eb03d4971..aa4abf0a22a5 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -339,9 +339,8 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> >
> > void kunit_cleanup(struct kunit *test);
> >
> > -void __printf(3, 4) kunit_printk(const char *level,
> > - const struct kunit *test,
> > - const char *fmt, ...);
> > +__printf(2, 3)
> > +void kunit_printk(const struct kunit *test, const char *fmt, ...);
> >
> > /**
> > * kunit_info() - Prints an INFO level message associated with @test.
> > @@ -353,7 +352,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> > * Takes a variable number of format parameters just like printk().
> > */
> > #define kunit_info(test, fmt, ...) \
> > - kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
> > + kunit_printk(test, KERN_INFO fmt, ##__VA_ARGS__)
> >
> > /**
> > * kunit_warn() - Prints a WARN level message associated with @test.
> > @@ -364,7 +363,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> > * Prints a warning level message.
> > */
> > #define kunit_warn(test, fmt, ...) \
> > - kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
> > + kunit_printk(test, KERN_WARNING fmt, ##__VA_ARGS__)
> >
> > /**
> > * kunit_err() - Prints an ERROR level message associated with @test.
> > @@ -375,7 +374,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> > * Prints an error level message.
> > */
> > #define kunit_err(test, fmt, ...) \
> > - kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
> > + kunit_printk(test, KERN_ERR fmt, ##__VA_ARGS__)
> >
> > /**
> > * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> > diff --git a/kunit/test.c b/kunit/test.c
> > index b2ca9b94c353..ddb9bffb5a5d 100644
> > --- a/kunit/test.c
> > +++ b/kunit/test.c
> > @@ -16,40 +16,6 @@ static void kunit_set_failure(struct kunit *test)
> > WRITE_ONCE(test->success, false);
> > }
> >
> > -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);
> > -}
> > -
> > -static void kunit_print_tap_version(void)
> > -{
> > - static bool kunit_has_printed_tap_version;
> > -
> > - if (!kunit_has_printed_tap_version) {
> > - kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
> > - kunit_has_printed_tap_version = true;
> > - }
> > -}
> > -
> > static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> > {
> > struct kunit_case *test_case;
> > @@ -63,11 +29,9 @@ static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> >
> > static void kunit_print_subtest_start(struct kunit_suite *suite)
> > {
> > - kunit_print_tap_version();
> > - kunit_printk_emit(LOGLEVEL_INFO, "\t# Subtest: %s\n", suite->name);
> > - kunit_printk_emit(LOGLEVEL_INFO,
> > - "\t1..%zd\n",
> > - kunit_test_cases_len(suite->test_cases));
> > + pr_info_once("TAP version 14\n");
> > + pr_info("\t# Subtest: %s\n", suite->name);
> > + pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
> > }
> >
> > static void kunit_print_ok_not_ok(bool should_indent,
> > @@ -87,9 +51,8 @@ static void kunit_print_ok_not_ok(bool should_indent,
> > else
> > ok_not_ok = "not ok";
> >
> > - kunit_printk_emit(LOGLEVEL_INFO,
> > - "%s%s %zd - %s\n",
> > - indent, ok_not_ok, test_number, description);
> > + pr_info("%s%s %zd - %s\n",
> > + indent, ok_not_ok, test_number, description);
> > }
> >
> > static bool kunit_suite_has_succeeded(struct kunit_suite *suite)
> > @@ -133,11 +96,11 @@ static void kunit_print_string_stream(struct kunit *test,
> > kunit_err(test,
> > "Could not allocate buffer, dumping stream:\n");
> > list_for_each_entry(fragment, &stream->fragments, node) {
> > - kunit_err(test, fragment->fragment);
> > + kunit_err(test, "%s", fragment->fragment);
> > }
> > kunit_err(test, "\n");
> > } else {
> > - kunit_err(test, buf);
> > + kunit_err(test, "%s", buf);
> > kunit_kfree(test, buf);
> > }
> > }
> > @@ -505,19 +468,29 @@ void kunit_cleanup(struct kunit *test)
> > }
> > }
> >
> > -void kunit_printk(const char *level,
> > - const struct kunit *test,
> > - const char *fmt, ...)
> > +void kunit_printk(const struct kunit *test, const char *fmt, ...)
> > {
> > + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
> > struct va_format vaf;
> > va_list args;
> > + int kern_level;
> >
> > va_start(args, fmt);
> >
> > + while ((kern_level = printk_get_level(fmt)) != 0) {
> > + size_t size = printk_skip_level(fmt) - fmt;
> > +
> > + if (kern_level >= '0' && kern_level <= '7') {
> > + memcpy(lvl, fmt, size);
> > + lvl[size] = '\0';
> > + }
> > + fmt += size;
> > + }
> > +
> > vaf.fmt = fmt;
> > vaf.va = &args;
> >
> > - kunit_vprintk(test, level, &vaf);
> > + printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
> >
> > va_end(args);
> > }
>
> How is this simpler?
>
> If we are okay with dynamically adding the KERN_<LEVEL> and %pV (and I
> don't think that Sergey is),
Sergey may well be in the minority overall as %pV
is now very frequently
used throughout the kernel.
$ git grep "%pV" | wc -l
241
then wouldn't it be easier to pass in the
> kernel level as a separate parameter and then strip off all printk
> headers like this:
Depends on whether or not you care for overall
object size. Consolidated formats with the
embedded KERN_<LEVEL> like suggested are smaller
overall object size.
This style is also already used in the kernel.
> I don't know. I am clearly not an expert on this topic, but I don't see
> the merit of the while loop you added above or dropping the level param.
The while use is only to avoid misuses with consecutive
KERN_<LEVEL> formats, which had happened in the past.
> -----Original Message-----
> From: Joe Perches
>
> On Fri, 2019-08-30 at 11:38 -0700, Brendan Higgins wrote:
> > On Thu, Aug 29, 2019 at 09:44:58PM -0700, Joe Perches wrote:
> > > On Thu, 2019-08-29 at 11:01 -0600, shuah wrote:
> > > > On 8/28/19 3:49 AM, Sergey Senozhatsky wrote:
> > > > > On (08/28/19 02:31), Brendan Higgins wrote:
> > > > > [..]
> > > > > > Previously KUnit assumed that printk would always be present,
> which is
> > > > > > not a valid assumption to make. Fix that by removing call to
> > > > > > vprintk_emit, and calling printk directly.
> > > > > >
> > > > > > Reported-by: Randy Dunlap <[email protected]>
> > > > > > Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-
> [email protected]/T/#t
> > > > > > Cc: Stephen Rothwell <[email protected]>
> > > > > > Cc: Sergey Senozhatsky <[email protected]>
> > > > > > Signed-off-by: Brendan Higgins <[email protected]>
> > > > >
> > > > > [..]
> > > > >
> > > > > > -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);
> > > > > > -}
> > > > >
> > > > > This patch looks good to me. I like the removal of recursive
> > > > > vsprintf() (%pV).
> > > > >
> > > > > -ss
> > > > >
> > > >
> > > > Hi Sergey,
> > > >
> > > > What are the guidelines for using printk(). I recall some discussion
> > > > about not using printk(). I am seeing the following from checkpatch
> > > > script:
> > > >
> > > >
> > > > WARNING: Prefer [subsystem eg: netdev]_level([subsystem]dev, ...
> then
> > > > dev_level(dev, ... then pr_level(... to printk(KERN_LEVEL ...
> > > > #105: FILE: include/kunit/test.h:343:
> > > > + printk(KERN_LEVEL "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
> > > >
> > > >
> > > > Is there supposed to be pr_level() - I can find dev_level()
> > > >
> > > > cc'ing Joe Perches for his feedback on this message recommending
> > > > pr_level() which isn't in 5.3.
> > >
> > > I don't care for pr_level or KERN_LEVEL in a printk.
> >
> > I don't think I follow, how does your version fix this?
> >
> > > I think this is somewhat overly complicated.
> > >
> > > I think I'd write it like:
> > > ---
> > > include/kunit/test.h | 11 ++++-----
> > > kunit/test.c | 69 ++++++++++++++++------------------------------------
> > > 2 files changed, 26 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > index 8b7eb03d4971..aa4abf0a22a5 100644
> > > --- a/include/kunit/test.h
> > > +++ b/include/kunit/test.h
> > > @@ -339,9 +339,8 @@ static inline void *kunit_kzalloc(struct kunit *test,
> size_t size, gfp_t gfp)
> > >
> > > void kunit_cleanup(struct kunit *test);
> > >
> > > -void __printf(3, 4) kunit_printk(const char *level,
> > > - const struct kunit *test,
> > > - const char *fmt, ...);
> > > +__printf(2, 3)
> > > +void kunit_printk(const struct kunit *test, const char *fmt, ...);
> > >
> > > /**
> > > * kunit_info() - Prints an INFO level message associated with @test.
> > > @@ -353,7 +352,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> > > * Takes a variable number of format parameters just like printk().
> > > */
> > > #define kunit_info(test, fmt, ...) \
> > > - kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
> > > + kunit_printk(test, KERN_INFO fmt, ##__VA_ARGS__)
> > >
> > > /**
> > > * kunit_warn() - Prints a WARN level message associated with @test.
> > > @@ -364,7 +363,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> > > * Prints a warning level message.
> > > */
> > > #define kunit_warn(test, fmt, ...) \
> > > - kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
> > > + kunit_printk(test, KERN_WARNING fmt, ##__VA_ARGS__)
> > >
> > > /**
> > > * kunit_err() - Prints an ERROR level message associated with @test.
> > > @@ -375,7 +374,7 @@ void __printf(3, 4) kunit_printk(const char *level,
> > > * Prints an error level message.
> > > */
> > > #define kunit_err(test, fmt, ...) \
> > > - kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
> > > + kunit_printk(test, KERN_ERR fmt, ##__VA_ARGS__)
> > >
> > > /**
> > > * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> > > diff --git a/kunit/test.c b/kunit/test.c
> > > index b2ca9b94c353..ddb9bffb5a5d 100644
> > > --- a/kunit/test.c
> > > +++ b/kunit/test.c
> > > @@ -16,40 +16,6 @@ static void kunit_set_failure(struct kunit *test)
> > > WRITE_ONCE(test->success, false);
> > > }
> > >
> > > -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);
> > > -}
> > > -
> > > -static void kunit_print_tap_version(void)
> > > -{
> > > - static bool kunit_has_printed_tap_version;
> > > -
> > > - if (!kunit_has_printed_tap_version) {
> > > - kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
> > > - kunit_has_printed_tap_version = true;
> > > - }
> > > -}
> > > -
> > > static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> > > {
> > > struct kunit_case *test_case;
> > > @@ -63,11 +29,9 @@ static size_t kunit_test_cases_len(struct kunit_case
> *test_cases)
> > >
> > > static void kunit_print_subtest_start(struct kunit_suite *suite)
> > > {
> > > - kunit_print_tap_version();
> > > - kunit_printk_emit(LOGLEVEL_INFO, "\t# Subtest: %s\n", suite-
> >name);
> > > - kunit_printk_emit(LOGLEVEL_INFO,
> > > - "\t1..%zd\n",
> > > - kunit_test_cases_len(suite->test_cases));
> > > + pr_info_once("TAP version 14\n");
> > > + pr_info("\t# Subtest: %s\n", suite->name);
> > > + pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
> > > }
> > >
> > > static void kunit_print_ok_not_ok(bool should_indent,
> > > @@ -87,9 +51,8 @@ static void kunit_print_ok_not_ok(bool
> should_indent,
> > > else
> > > ok_not_ok = "not ok";
> > >
> > > - kunit_printk_emit(LOGLEVEL_INFO,
> > > - "%s%s %zd - %s\n",
> > > - indent, ok_not_ok, test_number, description);
> > > + pr_info("%s%s %zd - %s\n",
> > > + indent, ok_not_ok, test_number, description);
> > > }
> > >
> > > static bool kunit_suite_has_succeeded(struct kunit_suite *suite)
> > > @@ -133,11 +96,11 @@ static void kunit_print_string_stream(struct kunit
> *test,
> > > kunit_err(test,
> > > "Could not allocate buffer, dumping stream:\n");
> > > list_for_each_entry(fragment, &stream->fragments, node) {
> > > - kunit_err(test, fragment->fragment);
> > > + kunit_err(test, "%s", fragment->fragment);
> > > }
> > > kunit_err(test, "\n");
> > > } else {
> > > - kunit_err(test, buf);
> > > + kunit_err(test, "%s", buf);
> > > kunit_kfree(test, buf);
> > > }
> > > }
> > > @@ -505,19 +468,29 @@ void kunit_cleanup(struct kunit *test)
> > > }
> > > }
> > >
> > > -void kunit_printk(const char *level,
> > > - const struct kunit *test,
> > > - const char *fmt, ...)
> > > +void kunit_printk(const struct kunit *test, const char *fmt, ...)
> > > {
> > > + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
> > > struct va_format vaf;
> > > va_list args;
> > > + int kern_level;
> > >
> > > va_start(args, fmt);
> > >
> > > + while ((kern_level = printk_get_level(fmt)) != 0) {
> > > + size_t size = printk_skip_level(fmt) - fmt;
> > > +
> > > + if (kern_level >= '0' && kern_level <= '7') {
> > > + memcpy(lvl, fmt, size);
> > > + lvl[size] = '\0';
> > > + }
> > > + fmt += size;
> > > + }
> > > +
> > > vaf.fmt = fmt;
> > > vaf.va = &args;
> > >
> > > - kunit_vprintk(test, level, &vaf);
> > > + printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
> > >
> > > va_end(args);
> > > }
> >
> > How is this simpler?
> >
> > If we are okay with dynamically adding the KERN_<LEVEL> and %pV (and I
> > don't think that Sergey is),
>
> Sergey may well be in the minority overall as %pV
> is now very frequently
> used throughout the kernel.
>
> $ git grep "%pV" | wc -l
> 241
Hmm. IMHO %pV should be avoided if possible. Just because people are
doing it doesn't mean it should be used when it is not necessary.
>
> then wouldn't it be easier to pass in the
> > kernel level as a separate parameter and then strip off all printk
> > headers like this:
>
> Depends on whether or not you care for overall
> object size. Consolidated formats with the
> embedded KERN_<LEVEL> like suggested are smaller
> overall object size.
This is an argument I can agree with. I'm generally in favor of
things that lessen kernel size creep. :-)
-- Tim
>
> This style is also already used in the kernel.
>
> > I don't know. I am clearly not an expert on this topic, but I don't see
> > the merit of the while loop you added above or dropping the level param.
>
> The while use is only to avoid misuses with consecutive
> KERN_<LEVEL> formats, which had happened in the past.
On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
> > From: Joe Perches
[]
> IMHO %pV should be avoided if possible. Just because people are
> doing it doesn't mean it should be used when it is not necessary.
Well, as the guy that created %pV, I of course
have a different opinion.
> > then wouldn't it be easier to pass in the
> > > kernel level as a separate parameter and then strip off all printk
> > > headers like this:
> >
> > Depends on whether or not you care for overall
> > object size. Consolidated formats with the
> > embedded KERN_<LEVEL> like suggested are smaller
> > overall object size.
>
> This is an argument I can agree with. I'm generally in favor of
> things that lessen kernel size creep. :-)
As am I.
On Fri, Aug 30, 2019 at 3:46 PM Joe Perches <[email protected]> wrote:
>
> On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
> > > From: Joe Perches
> []
> > IMHO %pV should be avoided if possible. Just because people are
> > doing it doesn't mean it should be used when it is not necessary.
>
> Well, as the guy that created %pV, I of course
> have a different opinion.
>
> > > then wouldn't it be easier to pass in the
> > > > kernel level as a separate parameter and then strip off all printk
> > > > headers like this:
> > >
> > > Depends on whether or not you care for overall
> > > object size. Consolidated formats with the
> > > embedded KERN_<LEVEL> like suggested are smaller
> > > overall object size.
> >
> > This is an argument I can agree with. I'm generally in favor of
> > things that lessen kernel size creep. :-)
>
> As am I.
Sorry, to be clear, we are talking about the object size penalty due
to adding a single parameter to a function. Is that right?
> -----Original Message-----
> From: Brendan Higgins
>
> On Fri, Aug 30, 2019 at 3:46 PM Joe Perches <[email protected]> wrote:
> >
> > On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
> > > > From: Joe Perches
> > []
> > > IMHO %pV should be avoided if possible. Just because people are
> > > doing it doesn't mean it should be used when it is not necessary.
> >
> > Well, as the guy that created %pV, I of course
> > have a different opinion.
> >
> > > > then wouldn't it be easier to pass in the
> > > > > kernel level as a separate parameter and then strip off all printk
> > > > > headers like this:
> > > >
> > > > Depends on whether or not you care for overall
> > > > object size. Consolidated formats with the
> > > > embedded KERN_<LEVEL> like suggested are smaller
> > > > overall object size.
> > >
> > > This is an argument I can agree with. I'm generally in favor of
> > > things that lessen kernel size creep. :-)
> >
> > As am I.
>
> Sorry, to be clear, we are talking about the object size penalty due
> to adding a single parameter to a function. Is that right?
Not exactly. The argument is that pre-pending the different KERN_LEVEL
strings onto format strings can result in several versions of nearly identical strings
being compiled into the object file. By parameterizing this (that is, adding
'%s' into the format string, and putting the level into the string as an argument),
it prevents this duplication of format strings.
I haven't seen the data on duplication of format strings, and how much this
affects it, but little things can add up. Whether it matters in this case depends
on whether the format strings that kunit uses are also used elsewhere in the kernel,
and whether these same format strings are used with multiple kernel message levels.
-- Tim
> -----Original Message-----
> From: Joe Perches
>
> On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
> > > From: Joe Perches
> []
> > IMHO %pV should be avoided if possible. Just because people are
> > doing it doesn't mean it should be used when it is not necessary.
>
> Well, as the guy that created %pV, I of course
> have a different opinion.
LOL. Well I stepped in that one.
I don't have any data to support my position on this particular printk feature,
but having worked for a while on stack size reduction for a few Sony products,
I'm always a bit leery of recursive routines in the kernel. I vaguely recall
some recursive printk routines giving me problems on a product that used
a sub-4K stack configuration I did many years ago. I don't recall if it was
specifically %pV or not. Anyway YMMV.
-- Tim
On Fri, 2019-08-30 at 23:22 +0000, [email protected] wrote:
> > -----Original Message-----
> > From: Brendan Higgins
> >
> > On Fri, Aug 30, 2019 at 3:46 PM Joe Perches <[email protected]> wrote:
> > > On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
> > > > > From: Joe Perches
> > > []
> > > > IMHO %pV should be avoided if possible. Just because people are
> > > > doing it doesn't mean it should be used when it is not necessary.
> > >
> > > Well, as the guy that created %pV, I of course
> > > have a different opinion.
> > >
> > > > > then wouldn't it be easier to pass in the
> > > > > > kernel level as a separate parameter and then strip off all printk
> > > > > > headers like this:
> > > > >
> > > > > Depends on whether or not you care for overall
> > > > > object size. Consolidated formats with the
> > > > > embedded KERN_<LEVEL> like suggested are smaller
> > > > > overall object size.
> > > >
> > > > This is an argument I can agree with. I'm generally in favor of
> > > > things that lessen kernel size creep. :-)
> > >
> > > As am I.
> >
> > Sorry, to be clear, we are talking about the object size penalty due
> > to adding a single parameter to a function. Is that right?
>
> Not exactly. The argument is that pre-pending the different KERN_LEVEL
> strings onto format strings can result in several versions of nearly identical strings
> being compiled into the object file. By parameterizing this (that is, adding
> '%s' into the format string, and putting the level into the string as an argument),
> it prevents this duplication of format strings.
>
> I haven't seen the data on duplication of format strings, and how much this
> affects it, but little things can add up. Whether it matters in this case depends
> on whether the format strings that kunit uses are also used elsewhere in the kernel,
> and whether these same format strings are used with multiple kernel message levels.
deduplication can matter as well, but so far
there is little content with kunit_(err|warn|info(=)
kunit/example-test.c: kunit_info(test, "initializing\n");
kunit/test.c: kunit_err(test,
kunit/test.c: kunit_err(test, "%s", fragment->fragment);
kunit/test.c: kunit_err(test, "\n");
kunit/test.c: kunit_err(test, "%s", buf);
kunit/test.c: kunit_err(test, "failed to initialize: %d\n", ret);
kunit/test.c: kunit_err(test, "test case timed out\n");
kunit/test.c: kunit_err(test, "internal error occurred preventing test case from running: %d\n",
kunit/try-catch.c: kunit_err(test, "try timed out\n");
kunit/try-catch.c: kunit_err(test, "wake_up_process() was never called\n");
kunit/try-catch.c: kunit_err(test, "Unknown error: %d\n", exit_code);
Of these, only two do match other kernel uses.
"initializing\n", "failed to initialize: %d\n"
On Fri, Aug 30, 2019 at 11:22:43PM +0000, [email protected] wrote:
> > -----Original Message-----
> > From: Brendan Higgins
> >
> > On Fri, Aug 30, 2019 at 3:46 PM Joe Perches <[email protected]> wrote:
> > >
> > > On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
> > > > > From: Joe Perches
> > > []
> > > > IMHO %pV should be avoided if possible. Just because people are
> > > > doing it doesn't mean it should be used when it is not necessary.
> > >
> > > Well, as the guy that created %pV, I of course
> > > have a different opinion.
> > >
> > > > > then wouldn't it be easier to pass in the
> > > > > > kernel level as a separate parameter and then strip off all printk
> > > > > > headers like this:
> > > > >
> > > > > Depends on whether or not you care for overall
> > > > > object size. Consolidated formats with the
> > > > > embedded KERN_<LEVEL> like suggested are smaller
> > > > > overall object size.
> > > >
> > > > This is an argument I can agree with. I'm generally in favor of
> > > > things that lessen kernel size creep. :-)
> > >
> > > As am I.
> >
> > Sorry, to be clear, we are talking about the object size penalty due
> > to adding a single parameter to a function. Is that right?
>
> Not exactly. The argument is that pre-pending the different KERN_LEVEL
> strings onto format strings can result in several versions of nearly identical strings
> being compiled into the object file. By parameterizing this (that is, adding
> '%s' into the format string, and putting the level into the string as an argument),
> it prevents this duplication of format strings.
>
> I haven't seen the data on duplication of format strings, and how much this
> affects it, but little things can add up. Whether it matters in this case depends
> on whether the format strings that kunit uses are also used elsewhere in the kernel,
> and whether these same format strings are used with multiple kernel message levels.
> -- Tim
I thought this portion of the discussion was about whether Joe's version
of kunit_printk was better or my critique of his version of kunit_printk:
Joe's:
> > > > -void kunit_printk(const char *level,
> > > > - const struct kunit *test,
> > > > - const char *fmt, ...)
> > > > +void kunit_printk(const struct kunit *test, const char *fmt, ...)
> > > > {
> > > > + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
> > > > struct va_format vaf;
> > > > va_list args;
> > > > + int kern_level;
> > > >
> > > > va_start(args, fmt);
> > > >
> > > > + while ((kern_level = printk_get_level(fmt)) != 0) {
> > > > + size_t size = printk_skip_level(fmt) - fmt;
> > > > +
> > > > + if (kern_level >= '0' && kern_level <= '7') {
> > > > + memcpy(lvl, fmt, size);
> > > > + lvl[size] = '\0';
> > > > + }
> > > > + fmt += size;
> > > > + }
> > > > +
> > > > vaf.fmt = fmt;
> > > > vaf.va = &args;
> > > >
> > > > - kunit_vprintk(test, level, &vaf);
> > > > + printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
> > > >
> > > > va_end(args);
> > > > }
Mine:
> void kunit_printk(const char *level,
> const struct kunit *test,
> const char *fmt, ...)
> {
> struct va_format vaf;
> va_list args;
>
> va_start(args, fmt);
>
> + fmt = printk_skip_headers(fmt);
> +
> vaf.fmt = fmt;
> vaf.va = &args;
>
> - kunit_vprintk(test, level, &vaf);
> + printk("%s\t# %s %pV\n", level, test->name, &vaf);
>
> va_end(args);
> }
I thought you and Joe were arguing that "Joe's" resulted in a smaller
object size than "Mine" (not to be confused with the actual patch I
presented here, which is what Sergey suggested I do on a different
thread).
I really don't feel strongly about what Sergey suggested I do (which is
what this patch originally introduced), versus, what Joe suggested,
versus what I suggested in response to Joe (or any of the things
suggested on other threads). I just want to pick one, fix the breakage
in linux-next, and move on with my life.
Cheers
On Fri, 2019-08-30 at 16:37 -0700, Brendan Higgins wrote:
> I thought you and Joe were arguing that "Joe's" resulted in a smaller
> object size than "Mine" (not to be confused with the actual patch I
> presented here, which is what Sergey suggested I do on a different
> thread).
>
> I really don't feel strongly about what Sergey suggested I do (which is
> what this patch originally introduced), versus, what Joe suggested,
> versus what I suggested in response to Joe (or any of the things
> suggested on other threads). I just want to pick one, fix the breakage
> in linux-next, and move on with my life.
Well, if we are voting, I vote for mine! ;)
cheers, Joe
> -----Original Message-----
> From: Brendan Higgins
>
> On Fri, Aug 30, 2019 at 11:22:43PM +0000, [email protected] wrote:
> > > -----Original Message-----
> > > From: Brendan Higgins
> > >
> > > On Fri, Aug 30, 2019 at 3:46 PM Joe Perches <[email protected]> wrote:
> > > >
> > > > On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
> > > > > > From: Joe Perches
> > > > []
> > > > > IMHO %pV should be avoided if possible. Just because people are
> > > > > doing it doesn't mean it should be used when it is not necessary.
> > > >
> > > > Well, as the guy that created %pV, I of course
> > > > have a different opinion.
> > > >
> > > > > > then wouldn't it be easier to pass in the
> > > > > > > kernel level as a separate parameter and then strip off all printk
> > > > > > > headers like this:
> > > > > >
> > > > > > Depends on whether or not you care for overall
> > > > > > object size. Consolidated formats with the
> > > > > > embedded KERN_<LEVEL> like suggested are smaller
> > > > > > overall object size.
> > > > >
> > > > > This is an argument I can agree with. I'm generally in favor of
> > > > > things that lessen kernel size creep. :-)
> > > >
> > > > As am I.
> > >
> > > Sorry, to be clear, we are talking about the object size penalty due
> > > to adding a single parameter to a function. Is that right?
> >
> > Not exactly. The argument is that pre-pending the different KERN_LEVEL
> > strings onto format strings can result in several versions of nearly identical
> strings
> > being compiled into the object file. By parameterizing this (that is, adding
> > '%s' into the format string, and putting the level into the string as an
> argument),
> > it prevents this duplication of format strings.
> >
> > I haven't seen the data on duplication of format strings, and how much this
> > affects it, but little things can add up. Whether it matters in this case
> depends
> > on whether the format strings that kunit uses are also used elsewhere in
> the kernel,
> > and whether these same format strings are used with multiple kernel
> message levels.
> > -- Tim
>
> I thought this portion of the discussion was about whether Joe's version
> of kunit_printk was better or my critique of his version of kunit_printk:
>
> Joe's:
> > > > > -void kunit_printk(const char *level,
> > > > > - const struct kunit *test,
> > > > > - const char *fmt, ...)
> > > > > +void kunit_printk(const struct kunit *test, const char *fmt, ...)
> > > > > {
> > > > > + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
> > > > > struct va_format vaf;
> > > > > va_list args;
> > > > > + int kern_level;
> > > > >
> > > > > va_start(args, fmt);
> > > > >
> > > > > + while ((kern_level = printk_get_level(fmt)) != 0) {
> > > > > + size_t size = printk_skip_level(fmt) - fmt;
> > > > > +
> > > > > + if (kern_level >= '0' && kern_level <= '7') {
> > > > > + memcpy(lvl, fmt, size);
> > > > > + lvl[size] = '\0';
> > > > > + }
> > > > > + fmt += size;
> > > > > + }
> > > > > +
> > > > > vaf.fmt = fmt;
> > > > > vaf.va = &args;
> > > > >
> > > > > - kunit_vprintk(test, level, &vaf);
> > > > > + printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
> > > > >
> > > > > va_end(args);
> > > > > }
>
> Mine:
> > void kunit_printk(const char *level,
> > const struct kunit *test,
> > const char *fmt, ...)
> > {
> > struct va_format vaf;
> > va_list args;
> >
> > va_start(args, fmt);
> >
> > + fmt = printk_skip_headers(fmt);
> > +
> > vaf.fmt = fmt;
> > vaf.va = &args;
> >
> > - kunit_vprintk(test, level, &vaf);
> > + printk("%s\t# %s %pV\n", level, test->name, &vaf);
> >
> > va_end(args);
> > }
>
> I thought you and Joe were arguing that "Joe's" resulted in a smaller
> object size than "Mine" (not to be confused with the actual patch I
> presented here, which is what Sergey suggested I do on a different
> thread).
>
> I really don't feel strongly about what Sergey suggested I do (which is
> what this patch originally introduced), versus, what Joe suggested,
> versus what I suggested in response to Joe (or any of the things
> suggested on other threads). I just want to pick one, fix the breakage
> in linux-next, and move on with my life.
When in doubt, do what the sub-system maintainer says. I'd go
with Sergey's suggestion. Maintainers often are juggling a host
of issues, and weighing new features and usages of their system
against their long-term plans for their sub-system. Sometimes
they have time to communicate all the intricacies of their
counter-proposals, and sometimes not.
But they know their system best, and much more often than not
provide sound advice.
If you don't have a strong feeling about it, just do what they
say.
-- Tim
On Fri 2019-08-30 16:37:10, Brendan Higgins wrote:
> On Fri, Aug 30, 2019 at 11:22:43PM +0000, [email protected] wrote:
> > > -----Original Message-----
> > > From: Brendan Higgins
> > >
> > > On Fri, Aug 30, 2019 at 3:46 PM Joe Perches <[email protected]> wrote:
> > > >
> > > > On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
> > > > > > From: Joe Perches
> > > > []
> > > > > IMHO %pV should be avoided if possible. Just because people are
> > > > > doing it doesn't mean it should be used when it is not necessary.
> > > >
> > > > Well, as the guy that created %pV, I of course
> > > > have a different opinion.
> > > >
> > > > > > then wouldn't it be easier to pass in the
> > > > > > > kernel level as a separate parameter and then strip off all printk
> > > > > > > headers like this:
> > > > > >
> > > > > > Depends on whether or not you care for overall
> > > > > > object size. Consolidated formats with the
> > > > > > embedded KERN_<LEVEL> like suggested are smaller
> > > > > > overall object size.
> > > > >
> > > > > This is an argument I can agree with. I'm generally in favor of
> > > > > things that lessen kernel size creep. :-)
> > > >
> > > > As am I.
> > >
> > > Sorry, to be clear, we are talking about the object size penalty due
> > > to adding a single parameter to a function. Is that right?
> >
> > Not exactly. The argument is that pre-pending the different KERN_LEVEL
> > strings onto format strings can result in several versions of nearly identical strings
> > being compiled into the object file. By parameterizing this (that is, adding
> > '%s' into the format string, and putting the level into the string as an argument),
> > it prevents this duplication of format strings.
> >
> > I haven't seen the data on duplication of format strings, and how much this
> > affects it, but little things can add up. Whether it matters in this case depends
> > on whether the format strings that kunit uses are also used elsewhere in the kernel,
> > and whether these same format strings are used with multiple kernel message levels.
> > -- Tim
>
> I thought this portion of the discussion was about whether Joe's version
> of kunit_printk was better or my critique of his version of kunit_printk:
>
> Joe's:
> > > > > -void kunit_printk(const char *level,
> > > > > - const struct kunit *test,
> > > > > - const char *fmt, ...)
> > > > > +void kunit_printk(const struct kunit *test, const char *fmt, ...)
> > > > > {
> > > > > + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
> > > > > struct va_format vaf;
> > > > > va_list args;
> > > > > + int kern_level;
> > > > >
> > > > > va_start(args, fmt);
> > > > >
> > > > > + while ((kern_level = printk_get_level(fmt)) != 0) {
> > > > > + size_t size = printk_skip_level(fmt) - fmt;
> > > > > +
> > > > > + if (kern_level >= '0' && kern_level <= '7') {
> > > > > + memcpy(lvl, fmt, size);
> > > > > + lvl[size] = '\0';
> > > > > + }
> > > > > + fmt += size;
> > > > > + }
> > > > > +
> > > > > vaf.fmt = fmt;
> > > > > vaf.va = &args;
> > > > >
> > > > > - kunit_vprintk(test, level, &vaf);
> > > > > + printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
> > > > >
> > > > > va_end(args);
> > > > > }
>
> Mine:
> > void kunit_printk(const char *level,
> > const struct kunit *test,
> > const char *fmt, ...)
> > {
> > struct va_format vaf;
> > va_list args;
> >
> > va_start(args, fmt);
> >
> > + fmt = printk_skip_headers(fmt);
> > +
> > vaf.fmt = fmt;
> > vaf.va = &args;
> >
> > - kunit_vprintk(test, level, &vaf);
> > + printk("%s\t# %s %pV\n", level, test->name, &vaf);
> >
> > va_end(args);
> > }
>
> I thought you and Joe were arguing that "Joe's" resulted in a smaller
> object size than "Mine" (not to be confused with the actual patch I
> presented here, which is what Sergey suggested I do on a different
> thread).
>
> I really don't feel strongly about what Sergey suggested I do (which is
> what this patch originally introduced), versus, what Joe suggested,
> versus what I suggested in response to Joe (or any of the things
> suggested on other threads). I just want to pick one, fix the breakage
> in linux-next, and move on with my life.
I am a bit lost in all the versions ;-) Though, I like most this
patch. I think that it is based on Sergey's suggestion.
I think that object size is not a huge concern for unit testing.
Also if I get it correctly, the object is bigger only when
the same string is used with different log levels. I am not
sure how often this happen.
Feel free to use for this patch:
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
> Cheers
On 9/2/19 6:52 AM, Petr Mladek wrote:
> On Fri 2019-08-30 16:37:10, Brendan Higgins wrote:
>> On Fri, Aug 30, 2019 at 11:22:43PM +0000, [email protected] wrote:
>>>> -----Original Message-----
>>>> From: Brendan Higgins
>>>>
>>>> On Fri, Aug 30, 2019 at 3:46 PM Joe Perches <[email protected]> wrote:
>>>>>
>>>>> On Fri, 2019-08-30 at 21:58 +0000, [email protected] wrote:
>>>>>>> From: Joe Perches
>>>>> []
>>>>>> IMHO %pV should be avoided if possible. Just because people are
>>>>>> doing it doesn't mean it should be used when it is not necessary.
>>>>>
>>>>> Well, as the guy that created %pV, I of course
>>>>> have a different opinion.
>>>>>
>>>>>>> then wouldn't it be easier to pass in the
>>>>>>>> kernel level as a separate parameter and then strip off all printk
>>>>>>>> headers like this:
>>>>>>>
>>>>>>> Depends on whether or not you care for overall
>>>>>>> object size. Consolidated formats with the
>>>>>>> embedded KERN_<LEVEL> like suggested are smaller
>>>>>>> overall object size.
>>>>>>
>>>>>> This is an argument I can agree with. I'm generally in favor of
>>>>>> things that lessen kernel size creep. :-)
>>>>>
>>>>> As am I.
>>>>
>>>> Sorry, to be clear, we are talking about the object size penalty due
>>>> to adding a single parameter to a function. Is that right?
>>>
>>> Not exactly. The argument is that pre-pending the different KERN_LEVEL
>>> strings onto format strings can result in several versions of nearly identical strings
>>> being compiled into the object file. By parameterizing this (that is, adding
>>> '%s' into the format string, and putting the level into the string as an argument),
>>> it prevents this duplication of format strings.
>>>
>>> I haven't seen the data on duplication of format strings, and how much this
>>> affects it, but little things can add up. Whether it matters in this case depends
>>> on whether the format strings that kunit uses are also used elsewhere in the kernel,
>>> and whether these same format strings are used with multiple kernel message levels.
>>> -- Tim
>>
>> I thought this portion of the discussion was about whether Joe's version
>> of kunit_printk was better or my critique of his version of kunit_printk:
>>
>> Joe's:
>>>>>> -void kunit_printk(const char *level,
>>>>>> - const struct kunit *test,
>>>>>> - const char *fmt, ...)
>>>>>> +void kunit_printk(const struct kunit *test, const char *fmt, ...)
>>>>>> {
>>>>>> + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
>>>>>> struct va_format vaf;
>>>>>> va_list args;
>>>>>> + int kern_level;
>>>>>>
>>>>>> va_start(args, fmt);
>>>>>>
>>>>>> + while ((kern_level = printk_get_level(fmt)) != 0) {
>>>>>> + size_t size = printk_skip_level(fmt) - fmt;
>>>>>> +
>>>>>> + if (kern_level >= '0' && kern_level <= '7') {
>>>>>> + memcpy(lvl, fmt, size);
>>>>>> + lvl[size] = '\0';
>>>>>> + }
>>>>>> + fmt += size;
>>>>>> + }
>>>>>> +
>>>>>> vaf.fmt = fmt;
>>>>>> vaf.va = &args;
>>>>>>
>>>>>> - kunit_vprintk(test, level, &vaf);
>>>>>> + printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
>>>>>>
>>>>>> va_end(args);
>>>>>> }
>>
>> Mine:
>>> void kunit_printk(const char *level,
>>> const struct kunit *test,
>>> const char *fmt, ...)
>>> {
>>> struct va_format vaf;
>>> va_list args;
>>>
>>> va_start(args, fmt);
>>>
>>> + fmt = printk_skip_headers(fmt);
>>> +
>>> vaf.fmt = fmt;
>>> vaf.va = &args;
>>>
>>> - kunit_vprintk(test, level, &vaf);
>>> + printk("%s\t# %s %pV\n", level, test->name, &vaf);
>>>
>>> va_end(args);
>>> }
>>
>> I thought you and Joe were arguing that "Joe's" resulted in a smaller
>> object size than "Mine" (not to be confused with the actual patch I
>> presented here, which is what Sergey suggested I do on a different
>> thread).
>>
>> I really don't feel strongly about what Sergey suggested I do (which is
>> what this patch originally introduced), versus, what Joe suggested,
>> versus what I suggested in response to Joe (or any of the things
>> suggested on other threads). I just want to pick one, fix the breakage
>> in linux-next, and move on with my life.
>
> I am a bit lost in all the versions ;-) Though, I like most this
> patch. I think that it is based on Sergey's suggestion.
>
I am too.
> I think that object size is not a huge concern for unit testing.
> Also if I get it correctly, the object is bigger only when
> the same string is used with different log levels. I am not
> sure how often this happen.
>
> Feel free to use for this patch:
>
> Reviewed-by: Petr Mladek <[email protected]>
>
Brendan,
Send me the version Sergey suggested with a short summary of the
discussion in the commit log. Tag it v3 so I don't pull the wrong
patch in.
I am going to just ignore the checkpatch warn on this and get it in.
Thanks for the discussion. It helped me clarify my understanding of
the printk.
thanks,
-- Shuah