Re: [PATCH RFC v2 04/13] sysctl: add register_sysctl() wrapper for MODULE_SYSCTL_TABLE

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The macro `__UNIQUE_ID` is used without an argument in `__MODULE_SYSCTL_TABLE_NAME`, preventing its expansion.
- [Low] Dangling trailing backslash on the last line of the `register_sysctl` macro definition.
--

--- Patch [4]: [PATCH RFC v2 04/13] sysctl: add register_sysctl() wrapper for MODULE_SYSCTL_TABLE ---
commit 5518fc079c95785ae760d86da6be17693ac8aeea
Author: Mauricio Faria de Oliveira <[email protected]>

sysctl: add register_sysctl() wrapper for MODULE_SYSCTL_TABLE

Add a MODULE_SYSCTL_TABLE call into register_sysctl() for existing callers
to automatically use it.

> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 6960fe22c50bf..7e05fafd5544e 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -335,8 +335,58 @@ struct ctl_table_root {
>  #define MODULE_SYSCTL_TABLE(path, table)
>  #endif

[Severity: Medium]
Does this code prevent __UNIQUE_ID from being expanded correctly?

Looking at __MODULE_SYSCTL_TABLE_NAME in include/linux/sysctl.h:

#define __MODULE_SYSCTL_TABLE_NAME(table) \
        __mod_device_table(sysctl, __UNIQUE_ID)

Since __UNIQUE_ID is a function-like macro that requires an argument, using it
without parentheses prevents preprocessor expansion. This results in the
literal string being used to construct the symbol name, which defeats the
purpose of generating a unique identifier and could cause symbol redefinition
errors if MODULE_SYSCTL_TABLE is used multiple times.

> -#define register_sysctl(path, table)	\
> -	register_sysctl_sz(path, table, ARRAY_SIZE(table))
> +/*
> + * The register_sysctl() wrapper for the MODULE_SYSCTL_TABLE macro

[ ... ]

> +#define _register_sysctl(path, table, table_tmpl, path_tmpl)			\
> +({										\
> +	MODULE_SYSCTL_TABLE(path_tmpl, table_tmpl);				\
> +	register_sysctl_sz(path, table, ARRAY_SIZE(table));			\
> +})
> +
> +#define register_sysctl(path, table, tmpl_args...)				\
> +	_register_sysctl(path, table,						\
> +			 __sysctl_table_tmpl_or_default(table, ## tmpl_args),	\
> +			 __sysctl_path_tmpl_or_default(path, ## tmpl_args))	\

[Severity: Low]
Is this trailing backslash intended?

The trailing backslash on the last line of the register_sysctl macro
definition will unnecessarily consume the empty line that follows it.

> +
> +/* Helper macros for optional template arguments */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4