Re: [PATCH v5] rust: configfs: add procedural macro for declaring configfs attributes

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Raw identifiers generate incorrect configfs attribute names with an 'r#' prefix.
- [Low] The new macro strictly requires a trailing comma after the last field, reverting a usability fix and breaking compatibility with existing macro invocations.

Pre-existing issues:
- [High] Concurrent configfs directory creations of the same type will cause a data race on the `static` `DATA_ATTR_LIST` array.
--

commit 884f1e9ac3af92759b06d060216464edd49fb53f
Author: Malte Wechter <[email protected]>

rust: configfs: add procedural macro for declaring configfs attributes

Implement `configfs_attrs!` as a procedural macro using `syn`, this
improves readability and maintainability. Remove the old macro and
replace all uses with the new macro. Add the new macro implementation
file to MAINTAINERS.

> diff --git a/rust/macros/configfs_attrs.rs b/rust/macros/configfs_attrs.rs
> new file mode 100644
> index 0000000000000..f829a42cece88
> --- /dev/null
> +++ b/rust/macros/configfs_attrs.rs
[ ... ]
> +        attrs.push(quote! {
> +        static #name_with_attr: kernel::configfs::Attribute<#id, #data_ty, #data_ty> =
> +            // SAFETY: We are expanding `configfs_attrs`.
> +            unsafe {
> +              kernel::configfs::Attribute::new(kernel::c_str!(::core::stringify!(#name)))
             ^^^^

[Severity: Medium]
Will this generate incorrect configfs attribute names with an "r#" prefix if
a raw identifier is used? The macro uses unraw() for the Rust static names
earlier, but seems to pass the raw string into stringify!() here.

> +            };
> +
> +          // SAFETY: By design of this macro, the name of the variable we
> +          // invoke the `add` method on below, is not visible outside of
[ ... ]
> +          // by per subsystem mutex.
> +          unsafe { #data_attr_ident.add::<#attr_idx, #id, _>(&#name_with_attr) }
             ^^^^

[Severity: High]
This is a pre-existing issue, but does this code cause a data race? If
multiple threads call make_group concurrently (for example, creating
directories in different subsystems that share the same configuration struct),
they would execute this add() concurrently and mutate the static
DATA_ATTR_LIST array without synchronization, leading to undefined behavior.

> +        });
> +    }

> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
> index d18fbf4daa0a5..305dcbddf7974 100644
> --- a/rust/macros/helpers.rs
> +++ b/rust/macros/helpers.rs
[ ... ]
> +            match &*key.to_string() {
> +                $(
> +                    stringify!($key) => $name = Some($parser),
> +                )*
> +                _ => {
> +                    Err(Error::new_spanned(
> +                        &key,
> +                        format!(r#"unknown key "{key}". Valid keys are: {EXPECTED_KEYS:?}."#),
> +                    ))?
> +                }
> +            }
> +
> +            $input.parse::<Token![,]>()?;
             ^^^^

[Severity: Low]
Does this strictly require a trailing comma after the last field? The
previous declarative macro allowed an optional trailing comma, so
this strict requirement might break compatibility with existing macro
invocations.

> +            seen_keys.push(key);
> +        }

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