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
