Re: [Devel] [PATCH hci-8.1 5/6] ve/failcount: Use ve_printk with specified ve

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

On 10/2/26 23:38, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] printk: !CONFIG_PRINTK __ve_printk_ratelimited() stub lacks the ve parameter
> --
> 
> commit 446bb8b8f19e9083361fc5d794bba0859020d3a4
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/failcount: Use ve_printk with specified ve
> 
> This patch adds __ve_printk_ratelimited() and __ve_pr_warn_ratelimited()
> helpers that take an explicit ve argument. ve_failcount_inc() now uses them
> so the message goes to the right VE when called from IRQ context.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839
> 
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
> 
> [ ... ]
> 
>>   #else
>>   #define printk_ratelimited(fmt, ...)					\
>>   	no_printk(fmt, ##__VA_ARGS__)
>>   #define ve_printk_ratelimited(dst, fmt, ...)				\
>>   	no_printk(fmt, ##__VA_ARGS__)
>> +#define __ve_printk_ratelimited(dst, fmt, ...)				\
>> +	no_printk(fmt, ##__VA_ARGS__)
>>   #endif
> 
> [Severity: Low]
> Is the parameter list of this !CONFIG_PRINTK stub correct? The
> CONFIG_PRINTK version of __ve_printk_ratelimited() takes
> (dst, ve, fmt, ...), but this stub takes only (dst, fmt, ...).

No, it is not. Will fix in v2.

> 
> [ ... ]
> 
>> +#define __ve_pr_warn_ratelimited(dst, ve, fmt, ...)				\
>> +	__ve_printk_ratelimited(dst, ve, KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> 
> This macro is defined outside the #ifdef and always passes ve as the second
> argument. With CONFIG_VE=y and CONFIG_PRINTK=n, it expands into the
> mismatched stub, so ve is bound to fmt.
> 
>> diff --git a/include/linux/ve.h b/include/linux/ve.h
>> --- a/include/linux/ve.h
>> +++ b/include/linux/ve.h
> 
> [ ... ]
> 
>> +	__ve_pr_warn_ratelimited(VE_LOG_BOTH, __ve, "CT%s: hits the " #name	\
>> +		" limit, see the '" #name "' counter in ve.failcount "		\
>> +		"of the container's ve cgroup\n", ve_name(__ve));		\
> 
> With CONFIG_PRINTK=n, ve_failcount_inc() expands like this:
> 
> ve_failcount_inc()
>    __ve_pr_warn_ratelimited(VE_LOG_BOTH, __ve, "CT%s: ...", ve_name(__ve))
>      __ve_printk_ratelimited(dst, ve, KERN_WARNING pr_fmt(fmt), ...)
>        no_printk(<struct ve_struct *>, <real format>, ve_name(__ve))
> 
> no_printk() then receives a struct ve_struct pointer as its const char *
> format argument. The real format string and ve_name(__ve) end up in the
> varargs.
> 
> Can this produce an incompatible-pointer diagnostic, which is an error with
> -Werror or newer compilers? The format string also stops being checked.
> 
> Should the stub be changed to take (dst, ve, fmt, ...)? As written, this
> can break the build when CONFIG_VE=y and CONFIG_PRINTK=n.
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel