On Fri, Mar 6, 2026 at 11:28 AM Sami Tolvanen <[email protected]> wrote: > > On Thu, Feb 26, 2026 at 03:47:28PM -0800, Matthew Wood wrote: > > +/// Set a string module parameter from a string. > > +/// > > +/// Similar to [`set_param`] but for [`StringParam`]. > > +/// > > +/// # Safety > > +/// > > +/// Same requirements as [`set_param`]. > > +unsafe extern "C" fn set_string_param( > > + val: *const c_char, > > + param: *const bindings::kernel_param, > > +) -> c_int { > > + if val.is_null() { > > + crate::pr_warn!("Null pointer passed to > > `module_param::set_string_param`"); > > + return EINVAL.to_errno(); > > + } > > + > > + crate::error::from_result(|| { > > + // SAFETY: val points to a valid C string from the kernel. > > + let cstr_param = unsafe { StringParam::from_ptr(val) }; > > + > > + // SAFETY: By function safety requirements, param.arg points to > > our SetOnce<StringParam>. > > + let container = unsafe { > > &*((*param).__bindgen_anon_1.arg.cast::<SetOnce<StringParam>>()) }; > > I do realize this matches set_param, and there's a good chance I > missed something when reading the macros, but doesn't arg actually > point to ModuleParamAccess<T> here? Since the struct is not repr(C), > isn't the compiler technically speaking allowed to reorder the > fields, which means SetOnce<T> might not actually be at offset 0? >
Hi Sami, I appreciate the review! This does seem to be an oversight, I'll try to figure out the correct implementation. I agree, `repr(C)` is likely needed here. > > + > > + container > > + .populate(cstr_param) > > + .then_some(0) > > + .ok_or(kernel::error::code::EEXIST) > > Does this mean the behavior for Rust modules differs from C modules if > the user specifies multiple instances of the same parameter? I believe > we just use the last value of the parameter instead of failing in C. > > Sami Yes, I'll fix that, the implementations should match. Regards, Mat
