Message-ID: <877bz7f7jg.fsf@t14s.mail-host-address-is-not-set>
Date: Wed, 13 Aug 2025 19:36:19 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Boqun Feng <boqun.feng@...il.com>, Miguel Ojeda <ojeda@...nel.org>, Alex
 Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy
 Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, Trevor
 Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Jens Axboe
 <axboe@...nel.dk>, linux-block@...r.kernel.org,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 11/15] rnull: enable configuration via `configfs`

"Alice Ryhl" <aliceryhl@...gle.com> writes:

> On Wed, Aug 13, 2025 at 3:47 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@...gle.com> writes:
>>
>> > On Tue, Aug 12, 2025 at 10:44:29AM +0200, Andreas Hindborg wrote:
>> >> Allow rust null block devices to be configured and instantiated via
>> >> `configfs`.
>> >>
>> >> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>> >
>> > Overall LGTM, but a few comments below:
>> >
>> >> diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
>> >> new file mode 100644
>> >> index 000000000000..8d469c046a39
>> >> --- /dev/null
>> >> +++ b/drivers/block/rnull/configfs.rs
>> >> @@ -0,0 +1,218 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +use super::{NullBlkDevice, THIS_MODULE};
>> >> +use core::fmt::Write;
>> >> +use kernel::{
>> >> +    block::mq::gen_disk::{GenDisk, GenDiskBuilder},
>> >> +    c_str,
>> >> +    configfs::{self, AttributeOperations},
>> >> +    configfs_attrs, new_mutex,
>> >
>> > It would be nice to add
>> >
>> >       pub use configfs_attrs;
>> >
>> > to the configfs module so that you can import the macro from the
>> > configfs module instead of the root.
>>
>> OK, I'll do that.
>>
>> >
>> >> +            try_pin_init!( DeviceConfig {
>> >> +                data <- new_mutex!( DeviceConfigInner {
>> >
>> > Extra spaces in these macros.
>>
>> Thanks. I subconsciously like the space in that location, so when
>> rustfmt is bailing, I get these things in my code.
>>
>> >> +        let power_op_str = core::str::from_utf8(page)?.trim();
>> >> +
>> >> +        let power_op = match power_op_str {
>> >> +            "0" => Ok(false),
>> >> +            "1" => Ok(true),
>> >> +            _ => Err(EINVAL),
>> >> +        }?;
>> >
>> > We probably want kstrtobool here instead of manually parsing the
>> > boolean.
>>
>> Yea, I was debating on this a bit. I did want to consolidate this code,
>> but I don't particularly like ktostrbool. But I guess in the name of
>> consistency across the kernel it is the right choice.
>>
>> I'll add it to next spin.
>
> For your convenience, I already wrote a safe wrapper of kstrtobool for
> an out-of-tree driver. You're welcome to copy-paste this:
>
> fn kstrtobool(kstr: &CStr) -> Result<bool> {
>     let mut res = false;
>     to_result(unsafe {
> kernel::bindings::kstrtobool(kstr.as_char_ptr(), &mut res) })?;
>     Ok(res)
> }

Thanks, I did one as well today, accepting `&str` instead. The examples
highlight why it is not great:


  /// Convert common user inputs into boolean values using the kernel's `kstrtobool` function.
  ///
  /// This routine returns `Ok(bool)` if the first character is one of 'YyTt1NnFf0', or
  /// [oO][NnFf] for "on" and "off". Otherwise it will return `Err(EINVAL)`.
  ///
  /// # Examples
  ///
  /// ```
  /// # use kernel::str::kstrtobool;
  ///
  /// // Lowercase
  /// assert_eq!(kstrtobool("true"), Ok(true));
  /// assert_eq!(kstrtobool("tr"), Ok(true));
  /// assert_eq!(kstrtobool("t"), Ok(true));
  /// assert_eq!(kstrtobool("twrong"), Ok(true)); // <-- 🤷
  /// assert_eq!(kstrtobool("false"), Ok(false));
  /// assert_eq!(kstrtobool("f"), Ok(false));
  /// assert_eq!(kstrtobool("yes"), Ok(true));
  /// assert_eq!(kstrtobool("no"), Ok(false));
  /// assert_eq!(kstrtobool("on"), Ok(true));
  /// assert_eq!(kstrtobool("off"), Ok(false));
  ///
  /// // Camel case
  /// assert_eq!(kstrtobool("True"), Ok(true));
  /// assert_eq!(kstrtobool("False"), Ok(false));
  /// assert_eq!(kstrtobool("Yes"), Ok(true));
  /// assert_eq!(kstrtobool("No"), Ok(false));
  /// assert_eq!(kstrtobool("On"), Ok(true));
  /// assert_eq!(kstrtobool("Off"), Ok(false));
  ///
  /// // All caps
  /// assert_eq!(kstrtobool("TRUE"), Ok(true));
  /// assert_eq!(kstrtobool("FALSE"), Ok(false));
  /// assert_eq!(kstrtobool("YES"), Ok(true));
  /// assert_eq!(kstrtobool("NO"), Ok(false));
  /// assert_eq!(kstrtobool("ON"), Ok(true));
  /// assert_eq!(kstrtobool("OFF"), Ok(false));
  ///
  /// // Numeric
  /// assert_eq!(kstrtobool("1"), Ok(true));
  /// assert_eq!(kstrtobool("0"), Ok(false));
  ///
  /// // Invalid input
  /// assert_eq!(kstrtobool("invalid"), Err(EINVAL));
  /// assert_eq!(kstrtobool("2"), Err(EINVAL));
  /// ```
  pub fn kstrtobool(input: &str) -> Result<bool> {
      let mut result: bool = false;
      let c_str = CString::try_from_fmt(fmt!("{input}"))?;

      // SAFETY: `c_str` points to a valid null-terminated C string, and `result` is a valid
      // pointer to a bool that we own.
      let ret = unsafe { bindings::kstrtobool(c_str.as_char_ptr(), &mut result as *mut bool) };

      kernel::error::to_result(ret).map(|_| result)
  }

Not sure if we should take `CStr` or `str`, what do you think?


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists