linux-kernel - Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87ikl1xls4.fsf@kernel.org>
Date: Thu, 12 Jun 2025 13:05:31 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <lossin@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Boqun Feng" <boqun.feng@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Benno
 Lossin" <benno.lossin@...ton.me>,  "Alice Ryhl" <aliceryhl@...gle.com>,
  "Masahiro Yamada" <masahiroy@...nel.org>,  "Nathan Chancellor"
 <nathan@...nel.org>,  "Luis Chamberlain" <mcgrof@...nel.org>,  "Danilo
 Krummrich" <dakr@...nel.org>,  "Nicolas Schier"
 <nicolas.schier@...ux.dev>,  "Trevor Gross" <tmgross@...ch.edu>,  "Adam
 Bratschi-Kaye" <ark.email@...il.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>,  <linux-kbuild@...r.kernel.org>,  "Petr
 Pavlu" <petr.pavlu@...e.com>,  "Sami Tolvanen" <samitolvanen@...gle.com>,
  "Daniel Gomez" <da.gomez@...sung.com>,  "Simona Vetter"
 <simona.vetter@...ll.ch>,  "Greg KH" <gregkh@...uxfoundation.org>,  "Fiona
 Behrens" <me@...enk.dev>,  "Daniel Almeida"
 <daniel.almeida@...labora.com>,  <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

"Benno Lossin" <lossin@...nel.org> writes:

> On Wed Jun 11, 2025 at 12:31 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@...nel.org> writes:
>>> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>>>> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
>>>> index a3ee27e29a6f..16d300ad3d3b 100644
>>>> --- a/rust/macros/helpers.rs
>>>> +++ b/rust/macros/helpers.rs
>>>> @@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
>>>>      }
>>>>  }
>>>>
>>>> +pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
>>>> +    let peek = it.clone().next();
>>>> +    match peek {
>>>> +        Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
>>>
>>> Should we also allow a leading `+`?
>>
>> I would argue no, because rust literals cannot start with `+`.
>
> Makes sense.
>
>>>> +            let _ = it.next();
>>>> +            Some(punct.as_char())
>>>> +        }
>>>> +        _ => None,
>>>> +    }
>>>> +}
>>>> +
>>>>  pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
>>>>      if let Some(TokenTree::Literal(literal)) = it.next() {
>>>>          Some(literal.to_string())
>>>> @@ -86,3 +97,17 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
>>>>      }
>>>>      None
>>>>  }
>>>> +
>>>> +/// Parse a token stream of the form `expected_name: "value",` and return the
>>>> +/// string in the position of "value".
>>>> +///
>>>> +/// # Panics
>>>> +///
>>>> +/// - On parse error.
>>>> +pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
>>>> +    assert_eq!(expect_ident(it), expected_name);
>>>> +    assert_eq!(expect_punct(it), ':');
>>>> +    let string = expect_string(it);
>>>> +    assert_eq!(expect_punct(it), ',');
>>>
>>> This won't allow omitting the trailing comma.
>>
>> This is in line with the rest of the module macro.
>
> Then we should change that:
>
>     https://github.com/Rust-for-Linux/linux/issues/1172
>
>>>> +    string
>>>> +}
>>>
>>> [...]
>>>
>>>> @@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>>>>      let info = ModuleInfo::parse(&mut it);
>>>>
>>>>      let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
>>>> -    if let Some(author) = info.author {
>>>> -        modinfo.emit("author", &author);
>>>> +    if let Some(author) = &info.author {
>>>> +        modinfo.emit("author", author);
>>>>      }
>>>> -    if let Some(authors) = info.authors {
>>>> +    if let Some(authors) = &info.authors {
>>>>          for author in authors {
>>>> -            modinfo.emit("author", &author);
>>>> +            modinfo.emit("author", author);
>>>>          }
>>>>      }
>>>> -    if let Some(description) = info.description {
>>>> -        modinfo.emit("description", &description);
>>>> +    if let Some(description) = &info.description {
>>>> +        modinfo.emit("description", description);
>>>>      }
>>>>      modinfo.emit("license", &info.license);
>>>> -    if let Some(aliases) = info.alias {
>>>> +    if let Some(aliases) = &info.alias {
>>>>          for alias in aliases {
>>>> -            modinfo.emit("alias", &alias);
>>>> +            modinfo.emit("alias", alias);
>>>>          }
>>>>      }
>>>> -    if let Some(firmware) = info.firmware {
>>>> +    if let Some(firmware) = &info.firmware {
>>>>          for fw in firmware {
>>>> -            modinfo.emit("firmware", &fw);
>>>> +            modinfo.emit("firmware", fw);
>>>
>>> I don't like that you have to change all of these.
>>
>> Why not? If I was to write this code in the first place, I would have
>> used a reference rather than pass by value.
>
> That's fine, but do it in a separate commit then.

OK, I can do that 👍


Best regards,
Andreas Hindborg