On Tue, Sep 3, 2019 at 4:35 PM Joe Perches joe@perches.com wrote:
On Tue, 2019-09-03 at 16:21 -0700, 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.
This fixes a build error[1] reported by Randy.
For context this change comes after much discussion. My first stab[2] at this was just to make the KUnit logging code compile out; however, it was agreed that if we were going to use vprintk_emit, then vprintk_emit should provide a no-op stub, which lead to my second attempt[3]. In response to me trying to stub out vprintk_emit, Sergey Senozhatsky suggested a way for me to remove our usage of vprintk_emit, which led to my third attempt at solving this[4].
In my previous version of this patch[4], I completely removed vprintk_emit, as suggested by Sergey; however, there was a bit of debate over whether Sergey's solution was the best. The debate arose due to Sergey's version resulting in a checkpatch warning, which resulted in a debate over correct printk usage. Joe Perches offered an alternative fix which was somewhat less far reaching than what Sergey had suggested and importantly relied on continuing to use %pV. Much of the debated centered around whether %pV should be widely used, and whether Sergey's version would result in object size bloat. Ultimately, we decided to go with Sergey's version.
Reported-by: Randy Dunlap rdunlap@infradead.org Link[1]: https://lore.kernel.org/linux-kselftest/c7229254-0d90-d90e-f3df-5b6d6fc0b51f... Link[2]: https://lore.kernel.org/linux-kselftest/20190827174932.44177-1-brendanhiggin... Link[3]: https://lore.kernel.org/linux-kselftest/20190827234835.234473-1-brendanhiggi... Link[4]: https://lore.kernel.org/linux-kselftest/20190828093143.163302-1-brendanhiggi... Cc: Stephen Rothwell sfr@canb.auug.org.au Cc: Sergey Senozhatsky sergey.senozhatsky.work@gmail.com Cc: Joe Perches joe@perches.com Cc: Tim.Bird@sony.com Signed-off-by: Brendan Higgins brendanhiggins@google.com Acked-by: Randy Dunlap rdunlap@infradead.org # build-tested Reviewed-by: Petr Mladek pmladek@suse.com
Sorry for the long commit message, but given the long discussion (and some of the confusion that occurred in the discussion), it seemed appropriate to summarize the discussion around this patch up to this point (especially since one of the proposed patches was under a separate patch subject).
No changes have been made to this patch since v2, other than the commit log.
[]
diff --git 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__)Non trivial notes:
Please do not use KERN_LEVEL as a macro argument. It would just be a source of possible confusion.
Please use level or lvl like nearly every other macro that does this uses.
Will do.
And there is nothing wrong with using kunit_printk and it's not necessary to use an odd name like kunit_printk_level.
Sounds reasonable.
[...]
Thanks!