Previously KUnit assumed that printk would always be present, which is
not a valid assumption to make. Fix that by ifdefing out functions which
directly depend on printk core functions similar to what dev_printk
does.
Reported-by: Randy Dunlap <[email protected]>
Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
Cc: Stephen Rothwell <[email protected]>
Signed-off-by: Brendan Higgins <[email protected]>
---
include/kunit/test.h | 7 +++++++
kunit/test.c | 41 ++++++++++++++++++++++++-----------------
2 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 8b7eb03d4971..339af5f95c4a 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
void kunit_cleanup(struct kunit *test);
+#ifdef CONFIG_PRINTK
void __printf(3, 4) kunit_printk(const char *level,
const struct kunit *test,
const char *fmt, ...);
+#else
+static inline void __printf(3, 4) kunit_printk(const char *level,
+ const struct kunit *test,
+ const char *fmt, ...)
+{}
+#endif
/**
* kunit_info() - Prints an INFO level message associated with @test.
diff --git a/kunit/test.c b/kunit/test.c
index b2ca9b94c353..0aa1caf07a6b 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -16,6 +16,7 @@ static void kunit_set_failure(struct kunit *test)
WRITE_ONCE(test->success, false);
}
+#ifdef CONFIG_PRINTK
static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
{
return vprintk_emit(0, level, NULL, 0, fmt, args);
@@ -40,6 +41,29 @@ static void kunit_vprintk(const struct kunit *test,
kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
}
+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);
+}
+#else /* CONFIG_PRINTK */
+static inline int kunit_printk_emit(int level, const char *fmt, ...)
+{
+ return 0;
+}
+#endif /* CONFIG_PRINTK */
+
static void kunit_print_tap_version(void)
{
static bool kunit_has_printed_tap_version;
@@ -504,20 +528,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 8/27/19 10:49 AM, Brendan Higgins wrote:
> Previously KUnit assumed that printk would always be present, which is
> not a valid assumption to make. Fix that by ifdefing out functions which
> directly depend on printk core functions similar to what dev_printk
> does.
>
> Reported-by: Randy Dunlap <[email protected]>
> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> Cc: Stephen Rothwell <[email protected]>
> Signed-off-by: Brendan Higgins <[email protected]>
Acked-by: Randy Dunlap <[email protected]> # build-tested
Thanks.
> ---
> include/kunit/test.h | 7 +++++++
> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> 2 files changed, 31 insertions(+), 17 deletions(-)
--
~Randy
On 8/27/19 11:49 AM, Brendan Higgins wrote:
> Previously KUnit assumed that printk would always be present, which is
> not a valid assumption to make. Fix that by ifdefing out functions which
> directly depend on printk core functions similar to what dev_printk
> does.
>
> Reported-by: Randy Dunlap <[email protected]>
> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> Cc: Stephen Rothwell <[email protected]>
> Signed-off-by: Brendan Higgins <[email protected]>
> ---
> include/kunit/test.h | 7 +++++++
> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> 2 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 8b7eb03d4971..339af5f95c4a 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>
> void kunit_cleanup(struct kunit *test);
>
> +#ifdef CONFIG_PRINTK
Please make this #if defined(CONFIG_PRINTK)
> void __printf(3, 4) kunit_printk(const char *level,
Line these two up with const char *level,
> const struct kunit *test,
> const char *fmt, ...);
> +#else
> +static inline void __printf(3, 4) kunit_printk(const char *level,
> + const struct kunit *test,
> + const char *fmt, ...)
Same here.
> +{}
Either line this up or make it
const char *fmt, ...) { }
It is hard to read the way it is currently indented.
> +#endif
>
> /**
> * kunit_info() - Prints an INFO level message associated with @test.
> diff --git a/kunit/test.c b/kunit/test.c
> index b2ca9b94c353..0aa1caf07a6b 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
> @@ -16,6 +16,7 @@ static void kunit_set_failure(struct kunit *test)
> WRITE_ONCE(test->success, false);
> }
>
> +#ifdef CONFIG_PRINTK
Same here - if defined
> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
> return vprintk_emit(0, level, NULL, 0, fmt, args);
> @@ -40,6 +41,29 @@ static void kunit_vprintk(const struct kunit *test,
> kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
> }
>
> +void kunit_printk(const char *level,
> + const struct kunit *test,
> + const char *fmt, ...)
Line the arguments up.
> +{
> + 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);
> +}
> +#else /* CONFIG_PRINTK */
> +static inline int kunit_printk_emit(int level, const char *fmt, ...)
> +{
> + return 0;
Is there a reason to not use
> +} > +#endif /* CONFIG_PRINTK */
> +
> static void kunit_print_tap_version(void)
> {
> static bool kunit_has_printed_tap_version;
> @@ -504,20 +528,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);
> -}
>
Okay after reviewing this, I am not sure why you need to do all
this.
Why can't you just change the root function that throws the warn:
static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
{
return vprintk_emit(0, level, NULL, 0, fmt, args);
}
You aren'r really doing anything extra here, other than calling
vprintk_emit()
Unless I am missing something, can't you solve this problem by including
printk.h and let it handle the !CONFIG_PRINTK case?
thanks,
-- Shuah
On 8/27/19 1:21 PM, shuah wrote:
> On 8/27/19 11:49 AM, Brendan Higgins wrote:
>> Previously KUnit assumed that printk would always be present, which is
>> not a valid assumption to make. Fix that by ifdefing out functions which
>> directly depend on printk core functions similar to what dev_printk
>> does.
>>
>> Reported-by: Randy Dunlap <[email protected]>
>> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
>> Cc: Stephen Rothwell <[email protected]>
>> Signed-off-by: Brendan Higgins <[email protected]>
>> ---
>> include/kunit/test.h | 7 +++++++
>> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
>> 2 files changed, 31 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index 8b7eb03d4971..339af5f95c4a 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>> void kunit_cleanup(struct kunit *test);
>> +#ifdef CONFIG_PRINTK
>
> Please make this #if defined(CONFIG_PRINTK)
explain why, please?
thanks.
--
~Randy
On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
>
> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > Previously KUnit assumed that printk would always be present, which is
> > not a valid assumption to make. Fix that by ifdefing out functions which
> > directly depend on printk core functions similar to what dev_printk
> > does.
> >
> > Reported-by: Randy Dunlap <[email protected]>
> > Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> > Cc: Stephen Rothwell <[email protected]>
> > Signed-off-by: Brendan Higgins <[email protected]>
> > ---
> > include/kunit/test.h | 7 +++++++
> > kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> > 2 files changed, 31 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 8b7eb03d4971..339af5f95c4a 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
[...]
> Okay after reviewing this, I am not sure why you need to do all
> this.
>
> Why can't you just change the root function that throws the warn:
>
> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
> return vprintk_emit(0, level, NULL, 0, fmt, args);
> }
>
> You aren'r really doing anything extra here, other than calling
> vprintk_emit()
Yeah, I did that a while ago. I think it was a combination of trying
to avoid an extra layer of adding and then removing the log level, and
that's what dev_printk and friends did.
But I think you are probably right. It's a lot of maintenance overhead
to get rid of that. Probably best to just use what the printk people
have.
> Unless I am missing something, can't you solve this problem by including
> printk.h and let it handle the !CONFIG_PRINTK case?
Randy, I hope you don't mind, but I am going to ask you to re-ack my
next revision since it basically addresses the problem in a totally
different way.
On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
<[email protected]> wrote:
>
> On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
> >
> > On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > > Previously KUnit assumed that printk would always be present, which is
> > > not a valid assumption to make. Fix that by ifdefing out functions which
> > > directly depend on printk core functions similar to what dev_printk
> > > does.
> > >
> > > Reported-by: Randy Dunlap <[email protected]>
> > > Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> > > Cc: Stephen Rothwell <[email protected]>
> > > Signed-off-by: Brendan Higgins <[email protected]>
> > > ---
> > > include/kunit/test.h | 7 +++++++
> > > kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> > > 2 files changed, 31 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > index 8b7eb03d4971..339af5f95c4a 100644
> > > --- a/include/kunit/test.h
> > > +++ b/include/kunit/test.h
> > > @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> [...]
> > Okay after reviewing this, I am not sure why you need to do all
> > this.
> >
> > Why can't you just change the root function that throws the warn:
> >
> > static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > {
> > return vprintk_emit(0, level, NULL, 0, fmt, args);
> > }
> >
> > You aren'r really doing anything extra here, other than calling
> > vprintk_emit()
>
> Yeah, I did that a while ago. I think it was a combination of trying
> to avoid an extra layer of adding and then removing the log level, and
> that's what dev_printk and friends did.
>
> But I think you are probably right. It's a lot of maintenance overhead
> to get rid of that. Probably best to just use what the printk people
> have.
>
> > Unless I am missing something, can't you solve this problem by including
> > printk.h and let it handle the !CONFIG_PRINTK case?
>
> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> next revision since it basically addresses the problem in a totally
> different way.
Actually, scratch that. Checkpatch doesn't like me calling printk
without using a KERN_<LEVEL>.
Now that I am thinking back to when I wrote this. I think it also
might not like using a dynamic KERN_<LEVEL> either (printk("%s my
message", KERN_INFO)).
I am going to have to do some more investigation.
On 8/27/19 2:53 PM, Randy Dunlap wrote:
> On 8/27/19 1:21 PM, shuah wrote:
>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
>>> Previously KUnit assumed that printk would always be present, which is
>>> not a valid assumption to make. Fix that by ifdefing out functions which
>>> directly depend on printk core functions similar to what dev_printk
>>> does.
>>>
>>> Reported-by: Randy Dunlap <[email protected]>
>>> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
>>> Cc: Stephen Rothwell <[email protected]>
>>> Signed-off-by: Brendan Higgins <[email protected]>
>>> ---
>>> include/kunit/test.h | 7 +++++++
>>> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
>>> 2 files changed, 31 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>> index 8b7eb03d4971..339af5f95c4a 100644
>>> --- a/include/kunit/test.h
>>> +++ b/include/kunit/test.h
>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>>> void kunit_cleanup(struct kunit *test);
>>> +#ifdef CONFIG_PRINTK
>>
>> Please make this #if defined(CONFIG_PRINTK)
>
> explain why, please?
>
> thanks.
>
This can be used to do compound logic. I have been using this style for
that reason starting a couple of years now. I seem to work in code paths
where I have to look for multiple config vars.
In this case, it probably doesn't matter as much either way.
thanks,
-- Shuah
On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
<[email protected]> wrote:
>
> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> <[email protected]> wrote:
> >
> > On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
> > >
> > > On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > > > Previously KUnit assumed that printk would always be present, which is
> > > > not a valid assumption to make. Fix that by ifdefing out functions which
> > > > directly depend on printk core functions similar to what dev_printk
> > > > does.
> > > >
> > > > Reported-by: Randy Dunlap <[email protected]>
> > > > Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> > > > Cc: Stephen Rothwell <[email protected]>
> > > > Signed-off-by: Brendan Higgins <[email protected]>
> > > > ---
> > > > include/kunit/test.h | 7 +++++++
> > > > kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> > > > 2 files changed, 31 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > > index 8b7eb03d4971..339af5f95c4a 100644
> > > > --- a/include/kunit/test.h
> > > > +++ b/include/kunit/test.h
> > > > @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> > [...]
> > > Okay after reviewing this, I am not sure why you need to do all
> > > this.
> > >
> > > Why can't you just change the root function that throws the warn:
> > >
> > > static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > > {
> > > return vprintk_emit(0, level, NULL, 0, fmt, args);
> > > }
> > >
> > > You aren'r really doing anything extra here, other than calling
> > > vprintk_emit()
> >
> > Yeah, I did that a while ago. I think it was a combination of trying
> > to avoid an extra layer of adding and then removing the log level, and
> > that's what dev_printk and friends did.
> >
> > But I think you are probably right. It's a lot of maintenance overhead
> > to get rid of that. Probably best to just use what the printk people
> > have.
> >
> > > Unless I am missing something, can't you solve this problem by including
> > > printk.h and let it handle the !CONFIG_PRINTK case?
> >
> > Randy, I hope you don't mind, but I am going to ask you to re-ack my
> > next revision since it basically addresses the problem in a totally
> > different way.
>
> Actually, scratch that. Checkpatch doesn't like me calling printk
> without using a KERN_<LEVEL>.
>
> Now that I am thinking back to when I wrote this. I think it also
> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> message", KERN_INFO)).
>
> I am going to have to do some more investigation.
Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
Looking at the printk implementation, it appears to do the format
before it checks the log level:
https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
So I am pretty sure we can do it either with the vprintk_emit or with printk.
So it appears that we have to weigh the following trade-offs:
Using vprintk_emit:
Pros:
- That's what dev_printk uses.
- No checkpatch warnings.
Cons:
- Harder to maintain (requires ifdefery).
Using printk:
Pros:
- Easier to maintain.
Cons:
- I am less confident that it is correct (I am not 100% certain that
printk is intended to be used this way).
- Checkpatch warnings.
- Extra layer of string formatting.
My preference is to go the vprintk_emit route since I am more
confident that it is right, but I don't have a strong preference.
Quoting Brendan Higgins (2019-08-27 10:49:32)
> Previously KUnit assumed that printk would always be present, which is
> not a valid assumption to make. Fix that by ifdefing out functions which
> directly depend on printk core functions similar to what dev_printk
> does.
>
> Reported-by: Randy Dunlap <[email protected]>
> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> Cc: Stephen Rothwell <[email protected]>
> Signed-off-by: Brendan Higgins <[email protected]>
> ---
Does kunit itself have any meaning if printk doesn't work? Why not just
depend on CONFIG_PRINTK for now?
On Tue, Aug 27, 2019 at 2:46 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Brendan Higgins (2019-08-27 10:49:32)
> > Previously KUnit assumed that printk would always be present, which is
> > not a valid assumption to make. Fix that by ifdefing out functions which
> > directly depend on printk core functions similar to what dev_printk
> > does.
> >
> > Reported-by: Randy Dunlap <[email protected]>
> > Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> > Cc: Stephen Rothwell <[email protected]>
> > Signed-off-by: Brendan Higgins <[email protected]>
> > ---
>
> Does kunit itself have any meaning if printk doesn't work? Why not just
> depend on CONFIG_PRINTK for now?
I was thinking about that, but I figured it is probably easier in the
long run to make sure it always works without printk.
It also just seemed like the right thing to do, but I suppose that's
not a very good reason.
I am fine with any of the three options: depend on CONFIG_PRINTK - as
suggested by Stephen, just use printk - as suggested by Shuah, or
continue to use vprintk_emit as I have been doing. However, my
preference is the vprintk_emit option.
Anyone have any strong opinions on the matter?
On 8/27/19 3:36 PM, Brendan Higgins wrote:
> On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> <[email protected]> wrote:
>>
>> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
>> <[email protected]> wrote:
>>>
>>> On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
>>>>
>>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
>>>>> Previously KUnit assumed that printk would always be present, which is
>>>>> not a valid assumption to make. Fix that by ifdefing out functions which
>>>>> directly depend on printk core functions similar to what dev_printk
>>>>> does.
>>>>>
>>>>> Reported-by: Randy Dunlap <[email protected]>
>>>>> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
>>>>> Cc: Stephen Rothwell <[email protected]>
>>>>> Signed-off-by: Brendan Higgins <[email protected]>
>>>>> ---
>>>>> include/kunit/test.h | 7 +++++++
>>>>> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
>>>>> 2 files changed, 31 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>>>> index 8b7eb03d4971..339af5f95c4a 100644
>>>>> --- a/include/kunit/test.h
>>>>> +++ b/include/kunit/test.h
>>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>>> [...]
>>>> Okay after reviewing this, I am not sure why you need to do all
>>>> this.
>>>>
>>>> Why can't you just change the root function that throws the warn:
>>>>
>>>> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
>>>> {
>>>> return vprintk_emit(0, level, NULL, 0, fmt, args);
>>>> }
>>>>
>>>> You aren'r really doing anything extra here, other than calling
>>>> vprintk_emit()
>>>
>>> Yeah, I did that a while ago. I think it was a combination of trying
>>> to avoid an extra layer of adding and then removing the log level, and
>>> that's what dev_printk and friends did.
>>>
>>> But I think you are probably right. It's a lot of maintenance overhead
>>> to get rid of that. Probably best to just use what the printk people
>>> have.
>>>
>>>> Unless I am missing something, can't you solve this problem by including
>>>> printk.h and let it handle the !CONFIG_PRINTK case?
>>>
>>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
>>> next revision since it basically addresses the problem in a totally
>>> different way.
>>
>> Actually, scratch that. Checkpatch doesn't like me calling printk
>> without using a KERN_<LEVEL>.
>>
>> Now that I am thinking back to when I wrote this. I think it also
>> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
>> message", KERN_INFO)).
>>
>> I am going to have to do some more investigation.
>
> Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
>
> Looking at the printk implementation, it appears to do the format
> before it checks the log level:
>
> https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
>
> So I am pretty sure we can do it either with the vprintk_emit or with printk.
Let me see if we are on the same page first. I am asking if you can
just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
and !CONFIG_PRINTK cases.
I am not asking you to use printk() in place of vprintk_emit().
It is perfectly fine to use vprintk_emit()
>
> So it appears that we have to weigh the following trade-offs:
>
> Using vprintk_emit:
>
> Pros:
> - That's what dev_printk uses.
Not sure what you mean by this. I am suggesting if you can just
call vprintk_emit() and include printk.h and not have to ifdef
around all the other callers of kunit_vprintk_emit()
Yes. There is the other issue of why do you need the complexity
of having kunit_vprintk_emit() at all.
thanks,
-- Shuah
On Tue, Aug 27, 2019 at 3:00 PM shuah <[email protected]> wrote:
>
> On 8/27/19 3:36 PM, Brendan Higgins wrote:
> > On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> > <[email protected]> wrote:
> >>
> >> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> >> <[email protected]> wrote:
> >>>
> >>> On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
> >>>>
> >>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> >>>>> Previously KUnit assumed that printk would always be present, which is
> >>>>> not a valid assumption to make. Fix that by ifdefing out functions which
> >>>>> directly depend on printk core functions similar to what dev_printk
> >>>>> does.
> >>>>>
> >>>>> Reported-by: Randy Dunlap <[email protected]>
> >>>>> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> >>>>> Cc: Stephen Rothwell <[email protected]>
> >>>>> Signed-off-by: Brendan Higgins <[email protected]>
> >>>>> ---
> >>>>> include/kunit/test.h | 7 +++++++
> >>>>> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> >>>>> 2 files changed, 31 insertions(+), 17 deletions(-)
> >>>>>
> >>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >>>>> index 8b7eb03d4971..339af5f95c4a 100644
> >>>>> --- a/include/kunit/test.h
> >>>>> +++ b/include/kunit/test.h
> >>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> >>> [...]
> >>>> Okay after reviewing this, I am not sure why you need to do all
> >>>> this.
> >>>>
> >>>> Why can't you just change the root function that throws the warn:
> >>>>
> >>>> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> >>>> {
> >>>> return vprintk_emit(0, level, NULL, 0, fmt, args);
> >>>> }
> >>>>
> >>>> You aren'r really doing anything extra here, other than calling
> >>>> vprintk_emit()
> >>>
> >>> Yeah, I did that a while ago. I think it was a combination of trying
> >>> to avoid an extra layer of adding and then removing the log level, and
> >>> that's what dev_printk and friends did.
> >>>
> >>> But I think you are probably right. It's a lot of maintenance overhead
> >>> to get rid of that. Probably best to just use what the printk people
> >>> have.
> >>>
> >>>> Unless I am missing something, can't you solve this problem by including
> >>>> printk.h and let it handle the !CONFIG_PRINTK case?
> >>>
> >>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> >>> next revision since it basically addresses the problem in a totally
> >>> different way.
> >>
> >> Actually, scratch that. Checkpatch doesn't like me calling printk
> >> without using a KERN_<LEVEL>.
> >>
> >> Now that I am thinking back to when I wrote this. I think it also
> >> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> >> message", KERN_INFO)).
> >>
> >> I am going to have to do some more investigation.
> >
> > Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
> >
> > Looking at the printk implementation, it appears to do the format
> > before it checks the log level:
> >
> > https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> >
> > So I am pretty sure we can do it either with the vprintk_emit or with printk.
>
> Let me see if we are on the same page first. I am asking if you can
> just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
> and !CONFIG_PRINTK cases.
Ah sorry, I misunderstood you.
No, that doesn't work. I tried including linux/printk.h, and I get the
same error.
The reason for this is that vprintk_emit() is only defined when CONFIG_PRINTK=y:
https://elixir.bootlin.com/linux/latest/ident/vprintk_emit
> I am not asking you to use printk() in place of vprintk_emit().
> It is perfectly fine to use vprintk_emit()
Okay, cool.
> >
> > So it appears that we have to weigh the following trade-offs:
> >
> > Using vprintk_emit:
> >
> > Pros:
> > - That's what dev_printk uses.
>
> Not sure what you mean by this. I am suggesting if you can just
> call vprintk_emit() and include printk.h and not have to ifdef
> around all the other callers of kunit_vprintk_emit()
Oh, I was just saying that I heavily based my implementation of
kunit_printk on dev_printk. So I have a high degree of confidence that
it is okay to use it the way that I am using it.
> Yes. There is the other issue of why do you need the complexity
> of having kunit_vprintk_emit() at all.
Right, and the problem with the alternative, is there is no good
kernel API for logging with the log level set dynamically. printk
prefers to have it as a string prefix on the format string, but I
cannot do that because I need to add my own prefix to the format
string.
So, I guess I should just go ahead and address the earlier comments on
this patch?
> -----Original Message-----
> From: Brendan Higgins
>
> On Tue, Aug 27, 2019 at 3:00 PM shuah <[email protected]> wrote:
> >
> > On 8/27/19 3:36 PM, Brendan Higgins wrote:
> > > On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> > > <[email protected]> wrote:
> > >>
> > >> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> > >> <[email protected]> wrote:
> > >>>
> > >>> On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
> > >>>>
> > >>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > >>>>> Previously KUnit assumed that printk would always be present,
> which is
> > >>>>> not a valid assumption to make. Fix that by ifdefing out functions
> which
> > >>>>> directly depend on printk core functions similar to what dev_printk
> > >>>>> does.
> > >>>>>
> > >>>>> 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]>
> > >>>>> Signed-off-by: Brendan Higgins <[email protected]>
> > >>>>> ---
> > >>>>> include/kunit/test.h | 7 +++++++
> > >>>>> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> > >>>>> 2 files changed, 31 insertions(+), 17 deletions(-)
> > >>>>>
> > >>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > >>>>> index 8b7eb03d4971..339af5f95c4a 100644
> > >>>>> --- a/include/kunit/test.h
> > >>>>> +++ b/include/kunit/test.h
> > >>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit
> *test, size_t size, gfp_t gfp)
> > >>> [...]
> > >>>> Okay after reviewing this, I am not sure why you need to do all
> > >>>> this.
> > >>>>
> > >>>> Why can't you just change the root function that throws the warn:
> > >>>>
> > >>>> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > >>>> {
> > >>>> return vprintk_emit(0, level, NULL, 0, fmt, args);
> > >>>> }
> > >>>>
> > >>>> You aren'r really doing anything extra here, other than calling
> > >>>> vprintk_emit()
> > >>>
> > >>> Yeah, I did that a while ago. I think it was a combination of trying
> > >>> to avoid an extra layer of adding and then removing the log level, and
> > >>> that's what dev_printk and friends did.
> > >>>
> > >>> But I think you are probably right. It's a lot of maintenance overhead
> > >>> to get rid of that. Probably best to just use what the printk people
> > >>> have.
> > >>>
> > >>>> Unless I am missing something, can't you solve this problem by
> including
> > >>>> printk.h and let it handle the !CONFIG_PRINTK case?
> > >>>
> > >>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> > >>> next revision since it basically addresses the problem in a totally
> > >>> different way.
> > >>
> > >> Actually, scratch that. Checkpatch doesn't like me calling printk
> > >> without using a KERN_<LEVEL>.
> > >>
> > >> Now that I am thinking back to when I wrote this. I think it also
> > >> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> > >> message", KERN_INFO)).
> > >>
> > >> I am going to have to do some more investigation.
> > >
> > > Alright, I am pretty sure it is safe to do printk("%smessage",
> KERN_<LEVEL>);
> > >
> > > Looking at the printk implementation, it appears to do the format
> > > before it checks the log level:
> > >
> > >
> https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> > >
> > > So I am pretty sure we can do it either with the vprintk_emit or with
> printk.
> >
> > Let me see if we are on the same page first. I am asking if you can
> > just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
> > and !CONFIG_PRINTK cases.
>
> Ah sorry, I misunderstood you.
>
> No, that doesn't work. I tried including linux/printk.h, and I get the
> same error.
>
> The reason for this is that vprintk_emit() is only defined when
> CONFIG_PRINTK=y:
>
> https://elixir.bootlin.com/linux/latest/ident/vprintk_emit
Ugh. That's just a bug in include/linux/printk.h
There should be a stub definition for vprintk_emit() in the #else part
of #ifdef CONFIG_PRINTK.
You shouldn't be dealing with whether printk is present or not
in the kunit code. All the printk-related routines are supposed
to evaporate themselves, based on the conditional in
include/linux/printk.h
That should be fixed there instead of in your code.
Let me know if you'd like me to submit a patch for that. I only hesitate
because your patch depends on it, and IMHO it makes more sense to
include it in your batch than separately. Otherwise there's a patch race
condition.
-- Tim
On Tue, Aug 27, 2019 at 3:38 PM <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: Brendan Higgins
> >
> > On Tue, Aug 27, 2019 at 3:00 PM shuah <[email protected]> wrote:
> > >
> > > On 8/27/19 3:36 PM, Brendan Higgins wrote:
> > > > On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> > > > <[email protected]> wrote:
> > > >>
> > > >> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> > > >> <[email protected]> wrote:
> > > >>>
> > > >>> On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
> > > >>>>
> > > >>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > > >>>>> Previously KUnit assumed that printk would always be present,
> > which is
> > > >>>>> not a valid assumption to make. Fix that by ifdefing out functions
> > which
> > > >>>>> directly depend on printk core functions similar to what dev_printk
> > > >>>>> does.
> > > >>>>>
> > > >>>>> 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]>
> > > >>>>> Signed-off-by: Brendan Higgins <[email protected]>
> > > >>>>> ---
> > > >>>>> include/kunit/test.h | 7 +++++++
> > > >>>>> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> > > >>>>> 2 files changed, 31 insertions(+), 17 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > >>>>> index 8b7eb03d4971..339af5f95c4a 100644
> > > >>>>> --- a/include/kunit/test.h
> > > >>>>> +++ b/include/kunit/test.h
> > > >>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit
> > *test, size_t size, gfp_t gfp)
> > > >>> [...]
> > > >>>> Okay after reviewing this, I am not sure why you need to do all
> > > >>>> this.
> > > >>>>
> > > >>>> Why can't you just change the root function that throws the warn:
> > > >>>>
> > > >>>> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > > >>>> {
> > > >>>> return vprintk_emit(0, level, NULL, 0, fmt, args);
> > > >>>> }
> > > >>>>
> > > >>>> You aren'r really doing anything extra here, other than calling
> > > >>>> vprintk_emit()
> > > >>>
> > > >>> Yeah, I did that a while ago. I think it was a combination of trying
> > > >>> to avoid an extra layer of adding and then removing the log level, and
> > > >>> that's what dev_printk and friends did.
> > > >>>
> > > >>> But I think you are probably right. It's a lot of maintenance overhead
> > > >>> to get rid of that. Probably best to just use what the printk people
> > > >>> have.
> > > >>>
> > > >>>> Unless I am missing something, can't you solve this problem by
> > including
> > > >>>> printk.h and let it handle the !CONFIG_PRINTK case?
> > > >>>
> > > >>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> > > >>> next revision since it basically addresses the problem in a totally
> > > >>> different way.
> > > >>
> > > >> Actually, scratch that. Checkpatch doesn't like me calling printk
> > > >> without using a KERN_<LEVEL>.
> > > >>
> > > >> Now that I am thinking back to when I wrote this. I think it also
> > > >> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> > > >> message", KERN_INFO)).
> > > >>
> > > >> I am going to have to do some more investigation.
> > > >
> > > > Alright, I am pretty sure it is safe to do printk("%smessage",
> > KERN_<LEVEL>);
> > > >
> > > > Looking at the printk implementation, it appears to do the format
> > > > before it checks the log level:
> > > >
> > > >
> > https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> > > >
> > > > So I am pretty sure we can do it either with the vprintk_emit or with
> > printk.
> > >
> > > Let me see if we are on the same page first. I am asking if you can
> > > just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
> > > and !CONFIG_PRINTK cases.
> >
> > Ah sorry, I misunderstood you.
> >
> > No, that doesn't work. I tried including linux/printk.h, and I get the
> > same error.
> >
> > The reason for this is that vprintk_emit() is only defined when
> > CONFIG_PRINTK=y:
> >
> > https://elixir.bootlin.com/linux/latest/ident/vprintk_emit
>
> Ugh. That's just a bug in include/linux/printk.h
>
> There should be a stub definition for vprintk_emit() in the #else part
> of #ifdef CONFIG_PRINTK.
>
> You shouldn't be dealing with whether printk is present or not
> in the kunit code. All the printk-related routines are supposed
> to evaporate themselves, based on the conditional in
> include/linux/printk.h
>
> That should be fixed there instead of in your code.
Alright. That makes sense.
I will submit a patch to fix it.
Sorry for not suggesting that, I just assumed that it was my mistake
in how I was using printk.
> Let me know if you'd like me to submit a patch for that. I only hesitate
> because your patch depends on it, and IMHO it makes more sense to
> include it in your batch than separately. Otherwise there's a patch race
> condition.
Thanks for clearing up the confusion!
On 8/27/19 4:16 PM, Brendan Higgins wrote:
> On Tue, Aug 27, 2019 at 3:00 PM shuah <[email protected]> wrote:
>>
>> On 8/27/19 3:36 PM, Brendan Higgins wrote:
>>> On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
>>> <[email protected]> wrote:
>>>>
>>>> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
>>>> <[email protected]> wrote:
>>>>>
>>>>> On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
>>>>>>
>>>>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
>>>>>>> Previously KUnit assumed that printk would always be present, which is
>>>>>>> not a valid assumption to make. Fix that by ifdefing out functions which
>>>>>>> directly depend on printk core functions similar to what dev_printk
>>>>>>> does.
>>>>>>>
>>>>>>> Reported-by: Randy Dunlap <[email protected]>
>>>>>>> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
>>>>>>> Cc: Stephen Rothwell <[email protected]>
>>>>>>> Signed-off-by: Brendan Higgins <[email protected]>
>>>>>>> ---
>>>>>>> include/kunit/test.h | 7 +++++++
>>>>>>> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
>>>>>>> 2 files changed, 31 insertions(+), 17 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>>>>>> index 8b7eb03d4971..339af5f95c4a 100644
>>>>>>> --- a/include/kunit/test.h
>>>>>>> +++ b/include/kunit/test.h
>>>>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>>>>> [...]
>>>>>> Okay after reviewing this, I am not sure why you need to do all
>>>>>> this.
>>>>>>
>>>>>> Why can't you just change the root function that throws the warn:
>>>>>>
>>>>>> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
>>>>>> {
>>>>>> return vprintk_emit(0, level, NULL, 0, fmt, args);
>>>>>> }
>>>>>>
>>>>>> You aren'r really doing anything extra here, other than calling
>>>>>> vprintk_emit()
>>>>>
>>>>> Yeah, I did that a while ago. I think it was a combination of trying
>>>>> to avoid an extra layer of adding and then removing the log level, and
>>>>> that's what dev_printk and friends did.
>>>>>
>>>>> But I think you are probably right. It's a lot of maintenance overhead
>>>>> to get rid of that. Probably best to just use what the printk people
>>>>> have.
>>>>>
>>>>>> Unless I am missing something, can't you solve this problem by including
>>>>>> printk.h and let it handle the !CONFIG_PRINTK case?
>>>>>
>>>>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
>>>>> next revision since it basically addresses the problem in a totally
>>>>> different way.
>>>>
>>>> Actually, scratch that. Checkpatch doesn't like me calling printk
>>>> without using a KERN_<LEVEL>.
>>>>
>>>> Now that I am thinking back to when I wrote this. I think it also
>>>> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
>>>> message", KERN_INFO)).
>>>>
>>>> I am going to have to do some more investigation.
>>>
>>> Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
>>>
>>> Looking at the printk implementation, it appears to do the format
>>> before it checks the log level:
>>>
>>> https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
>>>
>>> So I am pretty sure we can do it either with the vprintk_emit or with printk.
>>
>> Let me see if we are on the same page first. I am asking if you can
>> just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
>> and !CONFIG_PRINTK cases.
>
> Ah sorry, I misunderstood you.
>
> No, that doesn't work. I tried including linux/printk.h, and I get the
> same error.
>
> The reason for this is that vprintk_emit() is only defined when CONFIG_PRINTK=y:
>
This is the real problem here. printk.h defines several for
!CONFIG_PRINTK case.
> https://elixir.bootlin.com/linux/latest/ident/vprintk_emit
>
>> I am not asking you to use printk() in place of vprintk_emit().
>> It is perfectly fine to use vprintk_emit()
>
> Okay, cool.
>
>>>
>>> So it appears that we have to weigh the following trade-offs:
>>>
>>> Using vprintk_emit:
>>>
>>> Pros:
>>> - That's what dev_printk uses.
>>
>> Not sure what you mean by this. I am suggesting if you can just
>> call vprintk_emit() and include printk.h and not have to ifdef
>> around all the other callers of kunit_vprintk_emit()
>
> Oh, I was just saying that I heavily based my implementation of
> kunit_printk on dev_printk. So I have a high degree of confidence that
> it is okay to use it the way that I am using it.
>
>> Yes. There is the other issue of why do you need the complexity
>> of having kunit_vprintk_emit() at all.
>
> Right, and the problem with the alternative, is there is no good
> kernel API for logging with the log level set dynamically. printk
> prefers to have it as a string prefix on the format string, but I
> cannot do that because I need to add my own prefix to the format
> string.
>
> So, I guess I should just go ahead and address the earlier comments on
> this patch?
>
So what does your code do in the case of !CONFIG_PRINTK. From my read of
it, it returns 0 from kunit_printk_emit(0 from my read of it. What I am
saying is this is a lot of indirection instead of fixing the leaf
function which is kunit_vprintk_emit().
+#else /* CONFIG_PRINTK */
+static inline int kunit_printk_emit(int level, const char *fmt, ...)
+{
+ return 0;
+}
+#endif /* CONFIG_PRINTK */
Does the following work?
#if defined CONFIG_PRINTK
static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
{
return vprintk_emit(0, level, NULL, 0, fmt, args);
}
#else
static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
{
return 0;
}
#endif
I think the real problem is in the printk.h with its missing define for
vprintk_emit() for !CONFIG_PRINTK case. There seem to only one call for
this in drivers/base/core.c in CONFIG_PRINTK path. Unless I am totally
missing some context for why there is no stub for vprintk_emit() for
!CONFIG_PRINTK case
thanks,
-- Shuah
On Tue, Aug 27, 2019 at 3:55 PM shuah <[email protected]> wrote:
>
> On 8/27/19 4:16 PM, Brendan Higgins wrote:
> > On Tue, Aug 27, 2019 at 3:00 PM shuah <[email protected]> wrote:
> >>
> >> On 8/27/19 3:36 PM, Brendan Higgins wrote:
> >>> On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> >>> <[email protected]> wrote:
> >>>>
> >>>> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> >>>> <[email protected]> wrote:
> >>>>>
> >>>>> On Tue, Aug 27, 2019 at 1:21 PM shuah <[email protected]> wrote:
> >>>>>>
> >>>>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> >>>>>>> Previously KUnit assumed that printk would always be present, which is
> >>>>>>> not a valid assumption to make. Fix that by ifdefing out functions which
> >>>>>>> directly depend on printk core functions similar to what dev_printk
> >>>>>>> does.
> >>>>>>>
> >>>>>>> Reported-by: Randy Dunlap <[email protected]>
> >>>>>>> Link: https://lore.kernel.org/linux-kselftest/[email protected]/T/#t
> >>>>>>> Cc: Stephen Rothwell <[email protected]>
> >>>>>>> Signed-off-by: Brendan Higgins <[email protected]>
> >>>>>>> ---
> >>>>>>> include/kunit/test.h | 7 +++++++
> >>>>>>> kunit/test.c | 41 ++++++++++++++++++++++++-----------------
> >>>>>>> 2 files changed, 31 insertions(+), 17 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >>>>>>> index 8b7eb03d4971..339af5f95c4a 100644
> >>>>>>> --- a/include/kunit/test.h
> >>>>>>> +++ b/include/kunit/test.h
> >>>>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> >>>>> [...]
> >>>>>> Okay after reviewing this, I am not sure why you need to do all
> >>>>>> this.
> >>>>>>
> >>>>>> Why can't you just change the root function that throws the warn:
> >>>>>>
> >>>>>> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> >>>>>> {
> >>>>>> return vprintk_emit(0, level, NULL, 0, fmt, args);
> >>>>>> }
> >>>>>>
> >>>>>> You aren'r really doing anything extra here, other than calling
> >>>>>> vprintk_emit()
> >>>>>
> >>>>> Yeah, I did that a while ago. I think it was a combination of trying
> >>>>> to avoid an extra layer of adding and then removing the log level, and
> >>>>> that's what dev_printk and friends did.
> >>>>>
> >>>>> But I think you are probably right. It's a lot of maintenance overhead
> >>>>> to get rid of that. Probably best to just use what the printk people
> >>>>> have.
> >>>>>
> >>>>>> Unless I am missing something, can't you solve this problem by including
> >>>>>> printk.h and let it handle the !CONFIG_PRINTK case?
> >>>>>
> >>>>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> >>>>> next revision since it basically addresses the problem in a totally
> >>>>> different way.
> >>>>
> >>>> Actually, scratch that. Checkpatch doesn't like me calling printk
> >>>> without using a KERN_<LEVEL>.
> >>>>
> >>>> Now that I am thinking back to when I wrote this. I think it also
> >>>> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> >>>> message", KERN_INFO)).
> >>>>
> >>>> I am going to have to do some more investigation.
> >>>
> >>> Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
> >>>
> >>> Looking at the printk implementation, it appears to do the format
> >>> before it checks the log level:
> >>>
> >>> https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> >>>
> >>> So I am pretty sure we can do it either with the vprintk_emit or with printk.
> >>
> >> Let me see if we are on the same page first. I am asking if you can
> >> just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
> >> and !CONFIG_PRINTK cases.
> >
> > Ah sorry, I misunderstood you.
> >
> > No, that doesn't work. I tried including linux/printk.h, and I get the
> > same error.
> >
> > The reason for this is that vprintk_emit() is only defined when CONFIG_PRINTK=y:
> >
>
> This is the real problem here. printk.h defines several for
> !CONFIG_PRINTK case.
Yeah, Tim pointed that out.
I think both of you are right, I should be filing my fix against them.
> > https://elixir.bootlin.com/linux/latest/ident/vprintk_emit
> >
> >> I am not asking you to use printk() in place of vprintk_emit().
> >> It is perfectly fine to use vprintk_emit()
> >
> > Okay, cool.
> >
> >>>
> >>> So it appears that we have to weigh the following trade-offs:
> >>>
> >>> Using vprintk_emit:
> >>>
> >>> Pros:
> >>> - That's what dev_printk uses.
> >>
> >> Not sure what you mean by this. I am suggesting if you can just
> >> call vprintk_emit() and include printk.h and not have to ifdef
> >> around all the other callers of kunit_vprintk_emit()
> >
> > Oh, I was just saying that I heavily based my implementation of
> > kunit_printk on dev_printk. So I have a high degree of confidence that
> > it is okay to use it the way that I am using it.
> >
> >> Yes. There is the other issue of why do you need the complexity
> >> of having kunit_vprintk_emit() at all.
> >
> > Right, and the problem with the alternative, is there is no good
> > kernel API for logging with the log level set dynamically. printk
> > prefers to have it as a string prefix on the format string, but I
> > cannot do that because I need to add my own prefix to the format
> > string.
> >
> > So, I guess I should just go ahead and address the earlier comments on
> > this patch?
> >
>
> So what does your code do in the case of !CONFIG_PRINTK. From my read of
> it, it returns 0 from kunit_printk_emit(0 from my read of it. What I am
> saying is this is a lot of indirection instead of fixing the leaf
> function which is kunit_vprintk_emit().
Agreed. My apologies, as I mentioned in response to Tim, I just
assumed I was using it wrong.
> +#else /* CONFIG_PRINTK */
> +static inline int kunit_printk_emit(int level, const char *fmt, ...)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_PRINTK */
>
> Does the following work?
>
> #if defined CONFIG_PRINTK
> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
> return vprintk_emit(0, level, NULL, 0, fmt, args);
> }
> #else
> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
> return 0;
> }
> #endif
>
> I think the real problem is in the printk.h with its missing define for
> vprintk_emit() for !CONFIG_PRINTK case. There seem to only one call for
> this in drivers/base/core.c in CONFIG_PRINTK path. Unless I am totally
> missing some context for why there is no stub for vprintk_emit() for
> !CONFIG_PRINTK case
Agreed.
Sorry again for the confusion.
Thanks!