Re: [PATCH v7 04/10] rust: sizes: implement SizeConstants for Alignment

[email protected]
Newsgroups org.freedesktop.lists.dri-devel
Message-ID <[email protected]>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Implementing SizeConstants for Alignment logically conflates memory sizes with alignment constraints, preventing the definition of valid non-power-of-two sizes.
--

--- Patch [4]: [PATCH v7 04/10] rust: sizes: implement SizeConstants for Alignment ---
commit 3249458e082cdf9dedef6947c3ab8e2d8e84bcc1
Author: Eliot Courtney <[email protected]>

rust: sizes: implement SizeConstants for Alignment

This commit implements the SizeConstants trait for Alignment and adds more
size constants mirroring include/linux/sizes.h. This allows constructing
alignments using size constants like Alignment::SZ_8.

Link: https://lore.kernel.org/an4xDp29VX8Am0uR@yury

> diff --git a/rust/kernel/sizes.rs b/rust/kernel/sizes.rs
> index 521b2b38bfe77..825d6b5669524 100644
> --- a/rust/kernel/sizes.rs
> +++ b/rust/kernel/sizes.rs
[ ... ]
> @@ -117,6 +152,16 @@ pub trait SizeConstants {
>  
>      (@impls [] $($(#[$meta:meta])* $name:ident,)*) => {};
>  
> +    (@impl_alignment $($(#[$meta:meta])* $name:ident,)*) => {
> +        impl SizeConstants for crate::ptr::Alignment {
> +            $(
> +                $(#[$meta])*
> +                // A non-power-of-two constant will fail the build here if used.
> +                const $name: Self = crate::ptr::Alignment::new_checked(self::$name).unwrap();

[Severity: Medium]
Does implementing SizeConstants for Alignment logically conflate memory sizes
with alignment constraints?

If a developer later adds a valid non-power-of-two size (like SZ_3M from
include/linux/sizes.h) to the define_sizes! macro, won't it cause a mandatory
compile-time panic here because Alignment::new_checked() requires a power of
two?

This seems like it would prevent developers from adding legitimate 
non-power-of-two size constants to the macro in the future, limiting the
ability to fully mirror the C header as stated in the commit message.

> +            )*
> +        }
> +    };
> +
>      (@impls [$first:ty $(, $rest:ty)*] $($(#[$meta:meta])* $name:ident,)*) => {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.