Re: [PATCH v3] kunit: fix failure to build without printk

[Brendan Higgins]

On Tue, Sep 3, 2019 at 4:35 PM Joe Perches <joe@xxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > Link[1]: https://lore.kernel.org/linux-kselftest/c7229254-0d90-d90e-f3df-5b6d6fc0b51f@xxxxxxxxxxxxx/
> > Link[2]: https://lore.kernel.org/linux-kselftest/20190827174932.44177-1-brendanhiggins@xxxxxxxxxx/
> > Link[3]: https://lore.kernel.org/linux-kselftest/20190827234835.234473-1-brendanhiggins@xxxxxxxxxx/
> > Link[4]: https://lore.kernel.org/linux-kselftest/20190828093143.163302-1-brendanhiggins@xxxxxxxxxx/
> > Cc: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
> > Cc: Sergey Senozhatsky <sergey.senozhatsky.work@xxxxxxxxx>
> > Cc: Joe Perches <joe@xxxxxxxxxxx>
> > Cc: Tim.Bird@xxxxxxxx
> > Signed-off-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
> > Acked-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx> # build-tested
> > Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
> > ---
> >
> > 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!