Re: [PATCH v8 4/9] rust: acpi: add `acpi::DeviceId` abstraction

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Jun 26, 2025 at 06:40:06PM +0100, Igor Korotin wrote:
> 
> 
> On 6/26/25 16:25, Danilo Krummrich wrote:
> > On 6/20/25 5:24 PM, Igor Korotin wrote:
> >> +impl DeviceId {
> >> +    const ACPI_ID_LEN: usize = 16;
> >> +
> >> +    /// Create a new device id from an ACPI 'id' string.
> >> +    pub const fn new<const N: usize>(id: &[u8; N]) -> Self {
> > 
> > Didn't notice before, but why was this silently changed from &CStr to
> > &[u8; N]
> > from v6 to v7?
> > 
> >> +        build_assert!(N <= Self::ACPI_ID_LEN, "ID exceeds 16 bytes");
> >> +        // Replace with `bindings::acpi_device_id::default()` once
> >> stabilized for `const`.
> >> +        // SAFETY: FFI type is valid to be zero-initialized.
> >> +        let mut acpi: bindings::acpi_device_id = unsafe
> >> { core::mem::zeroed() };
> >> +        let mut i = 0;
> >> +        while i < N {
> >> +            acpi.id[i] = id[i];
> >> +            i += 1;
> >> +        }
> >> +
> >> +        Self(acpi)
> >> +    }
> >> +}
> 
> In v6 I was asked to change assert! (runtime) to build_assert! (build time)
> It was as follows:
> 
> > +    pub const fn new(id: &'static CStr) -> Self {
> > +        assert!(id.len() <= Self::ACPI_ID_LEN, "ID exceeds 16 bytes");
> 
> but id.len() breaks const context and so build_assert! triggers
> assertion.

It does indeed, but I'm not sure why it does...

> If I needed to explicitly describe change from CStr to
> [u8;20], then it's my bad.

Yes, that's usually better. Otherwise reviewers might skip the changed part. In
this case I think it actually introduced a bug:

Checking for N <= Self::ACPI_ID_LEN it can happen that acpi_device_id::id is not
NULL terminated anymore, whereas before this was ensured by
CStr::as_bytes_with_nul(). See also [1].

I think we can easily fix this by just checking N < Self::ACPI_ID_LEN.

However, I'd still like to check why build_assert!() does not work with
CStr::len() in this case first.

[1] https://elixir.bootlin.com/linux/v6.15.3/source/drivers/acpi/bus.c#L899