Re: [PATCH] net: sfc: avoid format string warning

[Edward Cree <ecree.xilinx@gmail.com>]

On 20/03/2026 15:19, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Three sfc drivers contain the same *_fill_test() function that takes
> a format string argument that gets passed down to snprintf(), producing
> a warning with clang-22:
> 
> drivers/net/ethernet/sfc/falcon/ethtool.c:227:60: error: diagnostic behavior may be improved by adding
>       the 'format(printf, 7, 8)' attribute to the declaration of 'ef4_fill_test' [-Werror,-Wmissing-format-attribute]
>   210 |                 snprintf(test_str, sizeof(test_str), test_format, test_id);
>       |                                                                          ^
> drivers/net/ethernet/sfc/falcon/ethtool.c:210:13: note: 'ef4_fill_test' declared here
> 
> Rework these to take a varargs based test_format that allows better
> type checking and avoids this warning.
> 
> Fixes: 3273c2e8c66a ("[netdrvr] sfc: sfc: Add self-test support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I'm not convinced this is an improvement.  The function takes *two* printf
 format strings (unit_format and test_format), and this only annotates and
 varargs-ifies one of them (and doesn't harden the unit_format, which has
 the considerably more worrying-looking "if it has a % in, assume it takes
 precisely one int argument").
So it may make things 'look' hardened enough to satisfy the compiler
 warning, but that just hides the unsafety and might sucker future
 developers into thinking "oh, the compiler knows about the format strings
 so I don't need to check it by hand".
I'm not sure what a proper fix would look like — possibly a rewrite adding
 a separate efx_fill_test_perq() function for the cases with a nontrivial
 unit_format, baking in the knowledge that the format is always "foo%d"
 for some constant string foo and thus we don't actually need to pass in
 an arbitrary format string.

> -static void efx_fill_test(unsigned int test_index, u8 *strings, u64 *data,
> -			  int *test, const char *unit_format, int unit_id,
> -			  const char *test_format, const char *test_id)
> +static void __printf(7, 8)
> +efx_fill_test(unsigned int test_index, u8 *strings, u64 *data,

I don't like splitting the specifiers/return type from the function name
 like this.  It's not Linux style[1].
(From a quick poke around the tree, there seems to be less consensus on
 where the __printf attribute goes — some put it on its own line before
 `static`, some would put it between `static` and `void`… sigh.)

-Ed

[1]: https://lore.kernel.org/all/1054519757.161606@palladium.transmeta.com/T/#u