Re: [PATCH 03/79] block: rnull: add macros to define configfs attributes

Alice Ryhl
2026-03-16T09:24:01Z

On Mon, Feb 16, 2026 at 12:34:50AM +0100, Andreas Hindborg wrote:
> Defining configfs attributes in rust is a bit verbose at the moment. Add
> some macros to make the attribute definition less verbose.
>
> The configfs Rust abstractions should eventually provide procedural macros
> for this task. When we get more users of the configfs Rust abstractions, we
> shall consider this task.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> drivers/block/rnull/configfs.rs | 134 +++++++++------------------------
> drivers/block/rnull/configfs/macros.rs | 128 +++++++++++++++++++++++++++++++
> 2 files changed, 164 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
> index ea38b27a9011c..eafa71dfc40d3 100644
> --- a/drivers/block/rnull/configfs.rs
> +++ b/drivers/block/rnull/configfs.rs
> @@ -1,20 +1,41 @@
> // SPDX-License-Identifier: GPL-2.0
>
> -use super::{NullBlkDevice, THIS_MODULE};
> +use super::{
> + NullBlkDevice,
> + THIS_MODULE, //
> +};
> use kernel::{
> - block::mq::gen_disk::{GenDisk, GenDiskBuilder},
> + block::mq::gen_disk::{
> + GenDisk,
> + GenDiskBuilder, //
> + },
> c_str,
> - configfs::{self, AttributeOperations},
> + configfs::{
> + self,
> + AttributeOperations, //
> + },
> configfs_attrs,
> - fmt::{self, Write as _},
> + fmt::{
> + self,
> + Write as _, //
> + },
> new_mutex,
> page::PAGE_SIZE,
> prelude::*,
> - str::{kstrtobool_bytes, CString},
> - sync::Mutex,
> + str::{
> + kstrtobool_bytes,
> + CString, //
> + },
> + sync::Mutex, //
> +};
> +use macros::{
> + configfs_simple_bool_field,
> + configfs_simple_field, //
> };
> use pin_init::PinInit;
>
> +mod macros;
> +
> pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, Error> {
> let item_type = configfs_attrs! {
> container: configfs::Subsystem<Config>,
> @@ -166,99 +187,16 @@ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> }
> }
>
> -#[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 = kstrtobool_bytes(page)?;
> -
> - 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(())
> - }
> -}
> -
> -#[vtable]
> -impl configfs::AttributeOperations<4> 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().irq_mode))?;
> - Ok(writer.bytes_written())
> - }
> +configfs_simple_field!(DeviceConfig, 1, block_size, u32, check GenDiskBuilder::validate_block_size);
> +configfs_simple_bool_field!(DeviceConfig, 2, rotational);
> +configfs_simple_field!(DeviceConfig, 3, capacity_mib, u64);
> +configfs_simple_field!(DeviceConfig, 4, irq_mode, IRQMode);
>
> - fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> - if this.data.lock().powered {
> - return Err(EBUSY);
> - }
> +impl core::str::FromStr for IRQMode {
> + type Err = Error;
>
> - let text = core::str::from_utf8(page)?.trim();
> - let value = text.parse::<u8>().map_err(|_| EINVAL)?;
> -
> - this.data.lock().irq_mode = IRQMode::try_from(value)?;
> - Ok(())
> + fn from_str(s: &str) -> Result<Self> {
> + let value: u8 = s.parse().map_err(|_| EINVAL)?;
> + value.try_into()
> }
> }
> diff --git a/drivers/block/rnull/configfs/macros.rs b/drivers/block/rnull/configfs/macros.rs
> new file mode 100644
> index 0000000000000..53ce9d5dbdc8a
> --- /dev/null
> +++ b/drivers/block/rnull/configfs/macros.rs
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::DeviceConfig;
> +use core::{fmt::Write, str::FromStr};
> +use kernel::{page::PAGE_SIZE, prelude::*};
> +
> +pub(crate) fn show_field<T: fmt::Display>(value: T, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> + let mut writer = kernel::str::Formatter::new(page);
> + writer.write_fmt(fmt!("{}\n", value))?;
> + Ok(writer.bytes_written())
> +}
> +
> +pub(crate) fn store_with_power_check<F>(this: &DeviceConfig, page: &[u8], store_fn: F) -> Result
> +where
> + F: FnOnce(&DeviceConfig, &[u8]) -> Result,
> +{
> + if this.data.lock().powered {
> + return Err(EBUSY);
> + }
> + store_fn(this, page)
> +}
> +
> +pub(crate) fn store_number_with_power_check<F, T>(
> + this: &DeviceConfig,
> + page: &[u8],
> + store_fn: F,
> +) -> Result
> +where
> + F: FnOnce(&DeviceConfig, T) -> Result,
> + T: FromStr,
> +{
> + if this.data.lock().powered {
> + return Err(EBUSY);
> + }
> +
> + let text = core::str::from_utf8(page)?.trim();
> + let value = text.parse::<T>().map_err(|_| EINVAL)?;
> +
> + store_fn(this, value)
> +}
> +
> +macro_rules! configfs_attribute {
> + (
> + $type:ty,
> + $id:literal,
> + show: |$show_this:ident, $show_page:ident| $show_block:expr,
> + store: |$store_this:ident, $store_page:ident| $store_block:expr
> + $(,)?
> + ) => {
> + #[vtable]
> + impl configfs::AttributeOperations<$id> for $type {
> + type Data = $type;
> +
> + fn show($show_this: &$type, $show_page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> + $show_block
> + }
> +
> + fn store($store_this: &$type, $store_page: &[u8]) -> Result {
> + $store_block
> + }
> + }
> + };
> +}
> +pub(crate) use configfs_attribute;
> +
> +// Specialized macro for simple boolean fields that just store kstrtobool_bytes result.
> +macro_rules! configfs_simple_bool_field {
> + ($type:ty, $id:literal, $field:ident) => {
> + crate::configfs::macros::configfs_attribute!($type, $id,
> + show: |this, page| crate::configfs::macros::show_field(this.data.lock().$field, page),
> + store: |this, page|
> + crate::configfs::macros::store_with_power_check(this, page, |this, page| {
> + this.data.lock().$field = kstrtobool_bytes(page)?;
> + Ok(())
> + })
> + );
> + };
> +}
> +pub(crate) use configfs_simple_bool_field;
> +
> +// Specialized macro for simple numeric fields that just parse and assign
> +macro_rules! configfs_simple_field {
> + // Simple direct assignment
> + ($type:ty, $id:literal, $field:ident, $field_type:ty) => {
> + crate::configfs::macros::configfs_attribute!($type, $id,
> + show: |this, page| crate::configfs::macros::show_field(this.data.lock().$field, page),
> + store: |this, page| crate::configfs::macros::store_number_with_power_check(
> + this,
> + page,
> + |this, value: $field_type| {
> + this.data.lock().$field = value;

I think there's a TOCTOU issue here. You take the lock to perform a
power check in store_number_with_power_check(), then you release the
lock and check again in `store`. But `powered` could have changed in the
meantime.

Alice