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

[Alice Ryhl <aliceryhl@google.com>]

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