Kselftest header defines multiple variadic function that use printf
along with other logic

There is no format checking for the variadic functions that use
printing inside kselftest.h. Because of this the compiler won't
be able to catch instances of mismatched print formats and debugging
tests might be more difficult

Add the common __printf attribute macro to kselftest.h

Add __printf attribute to every function using formatted printing with
variadic arguments

Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
---
 tools/testing/selftests/kselftest.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 829be379545a..ff47ed711879 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -77,6 +77,8 @@
 #define KSFT_XPASS 3
 #define KSFT_SKIP  4
 
+#define __printf(a, b)   __attribute__((format(printf, a, b)))
+
 /* counters */
 struct ksft_count {
 	unsigned int ksft_pass;
@@ -134,7 +136,7 @@ static inline void ksft_print_cnts(void)
 		ksft_cnt.ksft_xskip, ksft_cnt.ksft_error);
 }
 
-static inline void ksft_print_msg(const char *msg, ...)
+static inline __printf(1, 2) void ksft_print_msg(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -146,7 +148,7 @@ static inline void ksft_print_msg(const char *msg, ...)
 	va_end(args);
 }
 
-static inline void ksft_test_result_pass(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_pass(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -160,7 +162,7 @@ static inline void ksft_test_result_pass(const char *msg, ...)
 	va_end(args);
 }
 
-static inline void ksft_test_result_fail(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_fail(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -186,7 +188,7 @@ static inline void ksft_test_result_fail(const char *msg, ...)
 		ksft_test_result_fail(fmt, ##__VA_ARGS__);\
 	} while (0)
 
-static inline void ksft_test_result_xfail(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -200,7 +202,7 @@ static inline void ksft_test_result_xfail(const char *msg, ...)
 	va_end(args);
 }
 
-static inline void ksft_test_result_skip(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_skip(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -215,7 +217,7 @@ static inline void ksft_test_result_skip(const char *msg, ...)
 }
 
 /* TODO: how does "error" differ from "fail" or "skip"? */
-static inline void ksft_test_result_error(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -262,7 +264,7 @@ static inline int ksft_exit_fail(void)
 		  ksft_cnt.ksft_xfail +	\
 		  ksft_cnt.ksft_xskip)
 
-static inline int ksft_exit_fail_msg(const char *msg, ...)
+static inline __printf(1, 2) int ksft_exit_fail_msg(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -289,7 +291,7 @@ static inline int ksft_exit_xpass(void)
 	exit(KSFT_XPASS);
 }
 
-static inline int ksft_exit_skip(const char *msg, ...)
+static inline __printf(1, 2) int ksft_exit_skip(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
-- 
2.42.0

On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:

> Kselftest header defines multiple variadic function that use printf
> along with other logic
> 
> There is no format checking for the variadic functions that use
> printing inside kselftest.h. Because of this the compiler won't
> be able to catch instances of mismatched print formats and debugging
> tests might be more difficult
> 
> Add the common __printf attribute macro to kselftest.h
> 
> Add __printf attribute to every function using formatted printing with
> variadic arguments

Please add . to terminate the sentences.

The patch looks fine:
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

...However, there are formatting errors it found yet to fix.

-- 
 i.

> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
> ---
>  tools/testing/selftests/kselftest.h | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
> index 829be379545a..ff47ed711879 100644
> --- a/tools/testing/selftests/kselftest.h
> +++ b/tools/testing/selftests/kselftest.h
> @@ -77,6 +77,8 @@
>  #define KSFT_XPASS 3
>  #define KSFT_SKIP  4
>  
> +#define __printf(a, b)   __attribute__((format(printf, a, b)))
> +
>  /* counters */
>  struct ksft_count {
>  	unsigned int ksft_pass;
> @@ -134,7 +136,7 @@ static inline void ksft_print_cnts(void)
>  		ksft_cnt.ksft_xskip, ksft_cnt.ksft_error);
>  }
>  
> -static inline void ksft_print_msg(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_print_msg(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -146,7 +148,7 @@ static inline void ksft_print_msg(const char *msg, ...)
>  	va_end(args);
>  }
>  
> -static inline void ksft_test_result_pass(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_pass(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -160,7 +162,7 @@ static inline void ksft_test_result_pass(const char *msg, ...)
>  	va_end(args);
>  }
>  
> -static inline void ksft_test_result_fail(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_fail(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -186,7 +188,7 @@ static inline void ksft_test_result_fail(const char *msg, ...)
>  		ksft_test_result_fail(fmt, ##__VA_ARGS__);\
>  	} while (0)
>  
> -static inline void ksft_test_result_xfail(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -200,7 +202,7 @@ static inline void ksft_test_result_xfail(const char *msg, ...)
>  	va_end(args);
>  }
>  
> -static inline void ksft_test_result_skip(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_skip(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -215,7 +217,7 @@ static inline void ksft_test_result_skip(const char *msg, ...)
>  }
>  
>  /* TODO: how does "error" differ from "fail" or "skip"? */
> -static inline void ksft_test_result_error(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -262,7 +264,7 @@ static inline int ksft_exit_fail(void)
>  		  ksft_cnt.ksft_xfail +	\
>  		  ksft_cnt.ksft_xskip)
>  
> -static inline int ksft_exit_fail_msg(const char *msg, ...)
> +static inline __printf(1, 2) int ksft_exit_fail_msg(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -289,7 +291,7 @@ static inline int ksft_exit_xpass(void)
>  	exit(KSFT_XPASS);
>  }
>  
> -static inline int ksft_exit_skip(const char *msg, ...)
> +static inline __printf(1, 2) int ksft_exit_skip(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> 

Hi,

On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
>On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>
>> Kselftest header defines multiple variadic function that use printf
>> along with other logic
>> 
>> There is no format checking for the variadic functions that use
>> printing inside kselftest.h. Because of this the compiler won't
>> be able to catch instances of mismatched print formats and debugging
>> tests might be more difficult
>> 
>> Add the common __printf attribute macro to kselftest.h
>> 
>> Add __printf attribute to every function using formatted printing with
>> variadic arguments
>
>Please add . to terminate the sentences.

Thanks, I'll fix it in the next version

>The patch looks fine:
>Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
>...However, there are formatting errors it found yet to fix.

I believe you mean cache.c#L297.

I think I saw you're preparing some patches that remove the line that
reports the formatting error so I chose to not correct here.

Please let me know if I still should change it or would that be
redundant.

-- 
Kind regards
Maciej Wieczór-Retman

On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
> On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
> >
> >> Kselftest header defines multiple variadic function that use printf
> >> along with other logic
> >> 
> >> There is no format checking for the variadic functions that use
> >> printing inside kselftest.h. Because of this the compiler won't
> >> be able to catch instances of mismatched print formats and debugging
> >> tests might be more difficult
> >> 
> >> Add the common __printf attribute macro to kselftest.h
> >> 
> >> Add __printf attribute to every function using formatted printing with
> >> variadic arguments
> >
> >Please add . to terminate the sentences.
> 
> Thanks, I'll fix it in the next version
> 
> >The patch looks fine:
> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >
> >...However, there are formatting errors it found yet to fix.
> 
> I believe you mean cache.c#L297.
> 
> I think I saw you're preparing some patches that remove the line that
> reports the formatting error so I chose to not correct here.
> 
> Please let me know if I still should change it or would that be
> redundant.

There are other selftests besides resctrl which had a few warnings.

Making the selftests to rebuild though might be a bit tricky (you won't 
see the warnings otherwise), I don't know the command needed to clean 
selftests but I guess one can always force their timestamps to force 
recompile with:
  git ls-files tools/testing/selftests | xargs touch

-- 
 i.

On 2023-08-25 at 11:28:17 +0300, Ilpo Järvinen wrote:
>On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
>> On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
>> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>> >
>> >> Kselftest header defines multiple variadic function that use printf
>> >> along with other logic
>> >> 
>> >> There is no format checking for the variadic functions that use
>> >> printing inside kselftest.h. Because of this the compiler won't
>> >> be able to catch instances of mismatched print formats and debugging
>> >> tests might be more difficult
>> >> 
>> >> Add the common __printf attribute macro to kselftest.h
>> >> 
>> >> Add __printf attribute to every function using formatted printing with
>> >> variadic arguments
>> >
>> >Please add . to terminate the sentences.
>> 
>> Thanks, I'll fix it in the next version
>> 
>> >The patch looks fine:
>> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> >
>> >...However, there are formatting errors it found yet to fix.
>> 
>> I believe you mean cache.c#L297.
>> 
>> I think I saw you're preparing some patches that remove the line that
>> reports the formatting error so I chose to not correct here.
>> 
>> Please let me know if I still should change it or would that be
>> redundant.
>
>There are other selftests besides resctrl which had a few warnings.
>
>Making the selftests to rebuild though might be a bit tricky (you won't 
>see the warnings otherwise), I don't know the command needed to clean 
>selftests but I guess one can always force their timestamps to force 
>recompile with:
>  git ls-files tools/testing/selftests | xargs touch

Okay, I think I counted 13 more caused by the __printf().
I just ran:
	$ make -C tools/testing/selftests
and to rerun it later:
	$ make -C tools/testing/selftests clean

But do you think all these fixes fit into this series?
Is so, should I put them in a separate patch or just append to this one?

-- 
Kind regards
Maciej Wieczór-Retman

On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:

> On 2023-08-25 at 11:28:17 +0300, Ilpo Järvinen wrote:
> >On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
> >> On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
> >> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
> >> >
> >> >> Kselftest header defines multiple variadic function that use printf
> >> >> along with other logic
> >> >> 
> >> >> There is no format checking for the variadic functions that use
> >> >> printing inside kselftest.h. Because of this the compiler won't
> >> >> be able to catch instances of mismatched print formats and debugging
> >> >> tests might be more difficult
> >> >> 
> >> >> Add the common __printf attribute macro to kselftest.h
> >> >> 
> >> >> Add __printf attribute to every function using formatted printing with
> >> >> variadic arguments
> >> >
> >> >Please add . to terminate the sentences.
> >> 
> >> Thanks, I'll fix it in the next version
> >> 
> >> >The patch looks fine:
> >> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >> >
> >> >...However, there are formatting errors it found yet to fix.
> >> 
> >> I believe you mean cache.c#L297.
> >> 
> >> I think I saw you're preparing some patches that remove the line that
> >> reports the formatting error so I chose to not correct here.
> >> 
> >> Please let me know if I still should change it or would that be
> >> redundant.
> >
> >There are other selftests besides resctrl which had a few warnings.
> >
> >Making the selftests to rebuild though might be a bit tricky (you won't 
> >see the warnings otherwise), I don't know the command needed to clean 
> >selftests but I guess one can always force their timestamps to force 
> >recompile with:
> >  git ls-files tools/testing/selftests | xargs touch
> 
> Okay, I think I counted 13 more caused by the __printf().
> I just ran:
> 	$ make -C tools/testing/selftests
> and to rerun it later:
> 	$ make -C tools/testing/selftests clean
> 
> But do you think all these fixes fit into this series?
> Is so, should I put them in a separate patch or just append to this one?

Please fix them in separate patches.

IMO, the most logical approach is to make own series out of these printf 
attribute fixes and change, and another series for anything that is purely 
resctrl related.

-- 
 i.

On 2023-08-25 at 12:14:12 +0300, Ilpo Järvinen wrote:
>On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
>
>> On 2023-08-25 at 11:28:17 +0300, Ilpo Järvinen wrote:
>> >On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
>> >> On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
>> >> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>> >> >
>> >> >> Kselftest header defines multiple variadic function that use printf
>> >> >> along with other logic
>> >> >> 
>> >> >> There is no format checking for the variadic functions that use
>> >> >> printing inside kselftest.h. Because of this the compiler won't
>> >> >> be able to catch instances of mismatched print formats and debugging
>> >> >> tests might be more difficult
>> >> >> 
>> >> >> Add the common __printf attribute macro to kselftest.h
>> >> >> 
>> >> >> Add __printf attribute to every function using formatted printing with
>> >> >> variadic arguments
>> >> >
>> >> >Please add . to terminate the sentences.
>> >> 
>> >> Thanks, I'll fix it in the next version
>> >> 
>> >> >The patch looks fine:
>> >> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> >> >
>> >> >...However, there are formatting errors it found yet to fix.
>> >> 
>> >> I believe you mean cache.c#L297.
>> >> 
>> >> I think I saw you're preparing some patches that remove the line that
>> >> reports the formatting error so I chose to not correct here.
>> >> 
>> >> Please let me know if I still should change it or would that be
>> >> redundant.
>> >
>> >There are other selftests besides resctrl which had a few warnings.
>> >
>> >Making the selftests to rebuild though might be a bit tricky (you won't 
>> >see the warnings otherwise), I don't know the command needed to clean 
>> >selftests but I guess one can always force their timestamps to force 
>> >recompile with:
>> >  git ls-files tools/testing/selftests | xargs touch
>> 
>> Okay, I think I counted 13 more caused by the __printf().
>> I just ran:
>> 	$ make -C tools/testing/selftests
>> and to rerun it later:
>> 	$ make -C tools/testing/selftests clean
>> 
>> But do you think all these fixes fit into this series?
>> Is so, should I put them in a separate patch or just append to this one?
>
>Please fix them in separate patches.
>
>IMO, the most logical approach is to make own series out of these printf 
>attribute fixes and change, and another series for anything that is purely 
>resctrl related.

Okay, in that case I'll put this patch along with other fixes into a
separate series. Thanks for the help

-- 
Kind regards
Maciej Wieczór-Retman