On Thu, May 13, 2021 at 1:03 PM 'David Gow' via KUnit Development kunit-dev@googlegroups.com wrote:
Some KUnit functions use variable arguments to implement a printf-like format string. Use the __printf() attribute to let the compiler warn if invalid format strings are passed in.
If the kernel is build with W=1, it complained about the lack of these specifiers, e.g.: ../lib/kunit/test.c:72:2: warning: function ‘kunit_log_append’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
Signed-off-by: David Gow davidgow@google.com
Reviewed-by: Daniel Latypov dlatypov@google.com
As noted below, these additions don't really do anything. Unfortunately, they just make compiler warnings noisier in the case of kunit_log_append().
But if this silences a W=1 warning, then we should probably add them in. I guess it also serves as documentation that we're using the same standard format specifiers and not something custom, which is nice.
include/kunit/test.h | 2 +- lib/kunit/string-stream.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 49601c4b98b8..af2e386b867c 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -610,7 +610,7 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
void kunit_cleanup(struct kunit *test);
-void kunit_log_append(char *log, const char *fmt, ...); +void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
Before this patch: ../lib/kunit/kunit-example-test.c: In function ‘example_simple_test’: ../include/linux/kern_levels.h:5:18: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘int’ [-Wformat=] 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ | ^~~~~~ ../include/kunit/test.h:622:10: note: in definition of macro ‘kunit_log’ 622 | printk(lvl fmt, ##__VA_ARGS__); \ | ^~~ ../include/kunit/test.h:641:2: note: in expansion of macro ‘kunit_printk’ 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~~ ../include/linux/kern_levels.h:14:19: note: in expansion of macro ‘KERN_SOH’ 14 | #define KERN_INFO KERN_SOH "6" /* informational */ | ^~~~~~~~ ../include/kunit/test.h:641:15: note: in expansion of macro ‘KERN_INFO’ 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) | ^~~~~~~~~ ../lib/kunit/kunit-example-test.c:23:2: note: in expansion of macro ‘kunit_info’ 23 | kunit_info(test, "invalid: %s", 42);
After this patch, it gets noisier: In file included from ../lib/kunit/kunit-example-test.c:9: ../lib/kunit/kunit-example-test.c: In function ‘example_simple_test’: ../include/linux/kern_levels.h:5:18: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘int’ [-Wformat=] 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ | ^~~~~~ ../include/kunit/test.h:622:10: note: in definition of macro ‘kunit_log’ 622 | printk(lvl fmt, ##__VA_ARGS__); \ | ^~~ ../include/kunit/test.h:641:2: note: in expansion of macro ‘kunit_printk’ 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~~ ../include/linux/kern_levels.h:14:19: note: in expansion of macro ‘KERN_SOH’ 14 | #define KERN_INFO KERN_SOH "6" /* informational */ | ^~~~~~~~ ../include/kunit/test.h:641:15: note: in expansion of macro ‘KERN_INFO’ 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) | ^~~~~~~~~ ../lib/kunit/kunit-example-test.c:23:2: note: in expansion of macro ‘kunit_info’ 23 | kunit_info(test, "invalid: %s", 42); | ^~~~~~~~~~ ../include/kunit/test.h:105:31: warning: format ‘%s’ expects argument of type ‘char *’, but argument 4 has type ‘int’ [-Wformat=] 105 | #define KUNIT_SUBTEST_INDENT " " | ^~~~~~ ../include/kunit/test.h:623:42: note: in definition of macro ‘kunit_log’ 623 | kunit_log_append((test_or_suite)->log, fmt "\n", \ | ^~~ ../include/kunit/test.h:628:23: note: in expansion of macro ‘KUNIT_SUBTEST_INDENT’ 628 | kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt, \ | ^~~~~~~~~~~~~~~~~~~~ ../include/kunit/test.h:641:2: note: in expansion of macro ‘kunit_printk’ 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~~ ../lib/kunit/kunit-example-test.c:23:2: note: in expansion of macro ‘kunit_info’ 23 | kunit_info(test, "invalid: %s", 42); | ^~~~~~~~~~
/*
- printk and log to per-test or per-suite log buffer. Logging only done
diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h index fe98a00b75a9..5e94b623454f 100644 --- a/lib/kunit/string-stream.h +++ b/lib/kunit/string-stream.h @@ -35,9 +35,9 @@ struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp); int __printf(2, 3) string_stream_add(struct string_stream *stream, const char *fmt, ...);
-int string_stream_vadd(struct string_stream *stream,
const char *fmt,va_list args);+int __printf(2, 0) string_stream_vadd(struct string_stream *stream,
const char *fmt,va_list args);
This is never called with a literal `fmt` string. It's currently only ever called through the _add variant, which does have __printf(2,3).
So this can't catch any mistakes currently. And I think it's hard to imagine we'd ever pass in a literal format string w/ a va_list.
char *string_stream_get_string(struct string_stream *stream);
-- 2.31.1.751.gd2f1c929bd-goog
-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210513200350.854429-1-davidgow....