Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] The new macro no longer prevents duplicate attribute names at compile time. - [Low] The macro will panic at compile time if a raw identifier (e.g., `r#type`) is used as an attribute name. Pre-existing issues: - [High] The `configfs_attrs!` macro dynamically mutates a `static` array via `AttributeList::add`, causing a data race and undefined behavior when invoked concurrently at runtime. -- commit eb5933487fd2a34ea26fb0f106c056e03f633059 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..81037bc38188b > --- /dev/null > +++ b/rust/macros/configfs_attrs.rs [ ... ] > + let mut attrs = Vec::new(); > + for (attr_idx, (name, id)) in cfs_attrs.attributes.iter().enumerate() { > + let name_with_attr = format_ident!("{}_ATTR_{}", > name.to_string().to_uppercase(), attr_idx); [Severity: Medium] Does this change remove compile-time duplicate name checking? In the old macro, the generated static variables were named based only on the data type and attribute name (e.g., CONFIGURATION_MESSAGE_ATTR). If a user accidentally provided duplicate attribute names, the compiler would catch the duplicate static variable definition. By appending attr_idx to the identifier here, duplicate attributes will compile successfully since the variable names are unique, but they will fail at runtime when the C configfs layer attempts to register them. [Severity: Low] Will this format_ident! call panic during compilation if a raw identifier (like r#type) is used as an attribute name? When name is a raw identifier, name.to_string() yields "r#type", which becomes "R#TYPE" after to_uppercase(). Attempting to construct an identifier containing "R#" is invalid. Using name.unraw() might be needed to strip the prefix before processing. > + > + let id: u64 = match id.base10_parse::<u64>() { > + Ok(v) => v, > + Err(_) => { > + return syn::Error::new(id.span(), "Could not parse attribute > ID as a u64") > + .to_compile_error(); > + } > + }; > + > + 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))) > + }; > + > + // SAFETY: By design of this macro, the name of the variable we > + // invoke the `add` method on below, is not visible outside of > + // the macro expansion. The macro does not operate concurrently > + // on this variable, and thus we have exclusive access to the > + // variable. > + unsafe { #data_attr_ident.add::<#attr_idx, #id, > _>(&#name_with_attr) } [Severity: High] This is a pre-existing issue, but does this unsafe block introduce a data race? The macro expands to a block containing a static DATA_ATTR_LIST and unsafe runtime statements (DATA_ATTR_LIST.add(...)) to populate it. Because this macro is typically evaluated at runtime inside the make_group callback (which handles mkdir syscalls), concurrent executions by multiple threads will perform unsynchronized writes to the shared static variable. Concurrently modifying the same memory via interior mutability is undefined behavior in Rust. > + }); > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
