Allow rust null block devices to be configured and instantiated via
`configfs`.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
drivers/block/rnull/Kconfig | 2 +-
drivers/block/rnull/configfs.rs | 218 +++++++++++++++++++++++++++++++++++++++
drivers/block/rnull/rnull.rs | 58 ++++++-----
rust/kernel/block/mq/gen_disk.rs | 2 +-
4 files changed, 251 insertions(+), 29 deletions(-)
diff --git a/drivers/block/rnull/Kconfig b/drivers/block/rnull/Kconfig
index 6dc5aff96bf4..7bc5b376c128 100644
--- a/drivers/block/rnull/Kconfig
+++ b/drivers/block/rnull/Kconfig
@@ -4,7 +4,7 @@
config BLK_DEV_RUST_NULL
tristate "Rust null block driver (Experimental)"
- depends on RUST
+ depends on RUST && CONFIGFS_FS
help
This is the Rust implementation of the null block driver. Like
the C version, the driver allows the user to create virutal block
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,
+ page::PAGE_SIZE,
+ prelude::*,
+ str::CString,
+ sync::Mutex,
+};
+use pin_init::PinInit;
+
+pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, Error> {
+ let item_type = configfs_attrs! {
+ container: configfs::Subsystem<Config>,
+ data: Config,
+ child: DeviceConfig,
+ attributes: [
+ features: 0,
+ ],
+ };
+
+ kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, try_pin_init!(Config {}))
+}
+
+#[pin_data]
+pub(crate) struct Config {}
+
+#[vtable]
+impl AttributeOperations<0> for Config {
+ type Data = Config;
+
+ fn show(_this: &Config, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+ let mut writer = kernel::str::Formatter::new(page);
+ writer.write_str("blocksize,size,rotational\n")?;
+ Ok(writer.bytes_written())
+ }
+}
+
+#[vtable]
+impl configfs::GroupOperations for Config {
+ type Child = DeviceConfig;
+
+ fn make_group(
+ &self,
+ name: &CStr,
+ ) -> Result<impl PinInit<configfs::Group<DeviceConfig>, Error>> {
+ let item_type = configfs_attrs! {
+ container: configfs::Group<DeviceConfig>,
+ data: DeviceConfig,
+ attributes: [
+ // Named for compatibility with C null_blk
+ power: 0,
+ blocksize: 1,
+ rotational: 2,
+ size: 3,
+ ],
+ };
+
+ Ok(configfs::Group::new(
+ name.try_into()?,
+ item_type,
+ // TODO: cannot coerce new_mutex!() to impl PinInit<_, Error>, so put mutex inside
+ try_pin_init!( DeviceConfig {
+ data <- new_mutex!( DeviceConfigInner {
+ powered: false,
+ block_size: 4096,
+ rotational: false,
+ disk: None,
+ capacity_mib: 4096,
+ name: name.try_into()?,
+ }),
+ }),
+ ))
+ }
+}
+
+#[pin_data]
+pub(crate) struct DeviceConfig {
+ #[pin]
+ data: Mutex<DeviceConfigInner>,
+}
+
+#[pin_data]
+struct DeviceConfigInner {
+ powered: bool,
+ name: CString,
+ block_size: u32,
+ rotational: bool,
+ capacity_mib: u64,
+ disk: Option<GenDisk<NullBlkDevice>>,
+}
+
+#[vtable]
+impl configfs::AttributeOperations<0> for DeviceConfig {
+ type Data = DeviceConfig;
+
+ fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+ let mut writer = kernel::str::Formatter::new(page);
+
+ if this.data.lock().powered {
+ writer.write_str("1\n")?;
+ } else {
+ writer.write_str("0\n")?;
+ }
+
+ Ok(writer.bytes_written())
+ }
+
+ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+ 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),
+ }?;
+
+ let mut guard = this.data.lock();
+
+ if !guard.powered && power_op {
+ guard.disk = Some(NullBlkDevice::new(
+ &guard.name,
+ guard.block_size,
+ guard.rotational,
+ guard.capacity_mib,
+ )?);
+ guard.powered = true;
+ } else if guard.powered && !power_op {
+ drop(guard.disk.take());
+ guard.powered = false;
+ }
+
+ Ok(())
+ }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<1> for DeviceConfig {
+ type Data = DeviceConfig;
+
+ fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+ let mut writer = kernel::str::Formatter::new(page);
+ writer.write_fmt(fmt!("{}\n", this.data.lock().block_size))?;
+ Ok(writer.bytes_written())
+ }
+
+ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+ if this.data.lock().powered {
+ return Err(EBUSY);
+ }
+
+ let text = core::str::from_utf8(page)?.trim();
+ let value = text.parse::<u32>().map_err(|_| EINVAL)?;
+
+ GenDiskBuilder::validate_block_size(value)?;
+ this.data.lock().block_size = value;
+ Ok(())
+ }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<2> for DeviceConfig {
+ type Data = DeviceConfig;
+
+ fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+ let mut writer = kernel::str::Formatter::new(page);
+
+ if this.data.lock().rotational {
+ writer.write_str("1\n")?;
+ } else {
+ writer.write_str("0\n")?;
+ }
+
+ Ok(writer.bytes_written())
+ }
+
+ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+ if this.data.lock().powered {
+ return Err(EBUSY);
+ }
+
+ this.data.lock().rotational = match core::str::from_utf8(page)?.trim() {
+ "0" => false,
+ "1" => true,
+ _ => return Err(EINVAL),
+ };
+
+ Ok(())
+ }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<3> for DeviceConfig {
+ type Data = DeviceConfig;
+
+ fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+ let mut writer = kernel::str::Formatter::new(page);
+ writer.write_fmt(fmt!("{}\n", this.data.lock().capacity_mib))?;
+ Ok(writer.bytes_written())
+ }
+
+ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+ if this.data.lock().powered {
+ return Err(EBUSY);
+ }
+
+ let text = core::str::from_utf8(page)?.trim();
+ let value = text.parse::<u64>().map_err(|_| EINVAL)?;
+
+ this.data.lock().capacity_mib = value;
+ Ok(())
+ }
+}
diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
index d07e76ae2c13..d09bc77861e4 100644
--- a/drivers/block/rnull/rnull.rs
+++ b/drivers/block/rnull/rnull.rs
@@ -1,28 +1,26 @@
// SPDX-License-Identifier: GPL-2.0
//! This is a Rust implementation of the C null block driver.
-//!
-//! Supported features:
-//!
-//! - blk-mq interface
-//! - direct completion
-//! - block size 4k
-//!
-//! The driver is not configurable.
+
+mod configfs;
use kernel::{
alloc::flags,
- block::mq::{
+ block::{
self,
- gen_disk::{self, GenDisk},
- Operations, TagSet,
+ mq::{
+ self,
+ gen_disk::{self, GenDisk},
+ Operations, TagSet,
+ },
},
error::Result,
- new_mutex, pr_info,
+ pr_info,
prelude::*,
- sync::{Arc, Mutex},
+ sync::Arc,
types::ARef,
};
+use pin_init::PinInit;
module! {
type: NullBlkModule,
@@ -35,33 +33,39 @@
#[pin_data]
struct NullBlkModule {
#[pin]
- _disk: Mutex<GenDisk<NullBlkDevice>>,
+ configfs_subsystem: kernel::configfs::Subsystem<configfs::Config>,
}
impl kernel::InPlaceModule for NullBlkModule {
fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
pr_info!("Rust null_blk loaded\n");
- // Use a immediately-called closure as a stable `try` block
- let disk = /* try */ (|| {
- let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
-
- gen_disk::GenDiskBuilder::new()
- .capacity_sectors(4096 << 11)
- .logical_block_size(4096)?
- .physical_block_size(4096)?
- .rotational(false)
- .build(format_args!("rnullb{}", 0), tagset)
- })();
-
try_pin_init!(Self {
- _disk <- new_mutex!(disk?, "nullb:disk"),
+ configfs_subsystem <- configfs::subsystem(),
})
}
}
struct NullBlkDevice;
+impl NullBlkDevice {
+ fn new(
+ name: &CStr,
+ block_size: u32,
+ rotational: bool,
+ capacity_mib: u64,
+ ) -> Result<GenDisk<Self>> {
+ let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
+
+ gen_disk::GenDiskBuilder::new()
+ .capacity_sectors(capacity_mib << (20 - block::SECTOR_SHIFT))
+ .logical_block_size(block_size)?
+ .physical_block_size(block_size)?
+ .rotational(rotational)
+ .build(fmt!("{}", name.to_str()?), tagset)
+ }
+}
+
#[vtable]
impl Operations for NullBlkDevice {
#[inline(always)]
diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index 20f1d46c774d..6b1b846874db 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -51,7 +51,7 @@ pub fn rotational(mut self, rotational: bool) -> Self {
/// Validate block size by verifying that it is between 512 and `PAGE_SIZE`,
/// and that it is a power of two.
- fn validate_block_size(size: u32) -> Result {
+ pub fn validate_block_size(size: u32) -> Result {
if !(512..=bindings::PAGE_SIZE as u32).contains(&size) || !size.is_power_of_two() {
Err(error::code::EINVAL)
} else {
--
2.47.2On 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@kernel.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.
> + try_pin_init!( DeviceConfig {
> + data <- new_mutex!( DeviceConfigInner {
Extra spaces in these macros.
> + 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.
Alice
"Alice Ryhl" <aliceryhl@google.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@kernel.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.
Best regards,
Andreas Hindborg
On Wed, Aug 13, 2025 at 3:47 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.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@kernel.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)
}
Alice
"Alice Ryhl" <aliceryhl@google.com> writes:
> On Wed, Aug 13, 2025 at 3:47 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.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@kernel.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
On Wed, Aug 13, 2025 at 7:36 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > 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:
Yeah, well, I think we should still use it for consistency.
> /// 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?
Using CStr makes sense, since it avoids having the caller perform a
useless utf-8 check.
Alice
"Alice Ryhl" <aliceryhl@google.com> writes:
> On Wed, Aug 13, 2025 at 7:36 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > 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:
>
> Yeah, well, I think we should still use it for consistency.
>
>> /// 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?
>
> Using CStr makes sense, since it avoids having the caller perform a
> useless utf-8 check.
If we re-implement the entire function in rust, we can do the processing
on a `&str`. That way, we can skip the allocation to enforce null
termination. At least for this use case. I would rather do a utf8 check
than allocate and copy.
Best regards,
Andreas Hindborg
On Thu, Aug 14, 2025 at 9:40 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > On Wed, Aug 13, 2025 at 7:36 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> "Alice Ryhl" <aliceryhl@google.com> writes:
> >>
> >> > 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:
> >
> > Yeah, well, I think we should still use it for consistency.
> >
> >> /// 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?
> >
> > Using CStr makes sense, since it avoids having the caller perform a
> > useless utf-8 check.
>
> If we re-implement the entire function in rust, we can do the processing
> on a `&str`. That way, we can skip the allocation to enforce null
> termination. At least for this use case. I would rather do a utf8 check
> than allocate and copy.
You can copy to an array on the stack, so allocations aren't necessary
even if the string is not nul-terminated. I don't think we should
duplicate this logic.
And if we do add a Rust method that does not enforce a nul-check, then
I'd say it should use &[u8] as the argument rather than &str. (Or
possibly &Bstr.)
Alice
© 2016 - 2025 Red Hat, Inc.