Re: [PATCH v4] rust: pci: reject IRQ vector indices that do not fit in u32

[Alexandre Courbot]

On Tue Sep 1, 2026 at 10:36 PM JST, Danilo Krummrich wrote:
> On 9/1/26 3:32 PM, Alexandre Courbot wrote:
>> On Tue Sep 1, 2026 at 8:08 PM JST, Danilo Krummrich wrote:
>>> On Tue Sep 1, 2026 at 12:58 PM CEST, Alexandre Courbot wrote:
>>>> That makes me wonder, shouldn't we make `index` take a `u32` directly?
>>>> If that's what the C API expects, it does make sense to align to it
>>>> instead of forcing users to make a potential unneeded conversion if they
>>>> already have a u32.
>>>
>>> I intentionally did not do this, as the common type for an index is usize. Thus,
>>> I do not expect anyone to already have a u32, but to already have a usize, e.g.
>>> from some iterator.
>>>
>>> The fact that the C API did pick unsigned int as index type is an implementation
>>> detail the abstraction should bother with.
>> 
>> Thanks for the clarification (and Miguel for elaborating - I wasn't
>> completely aware of it!). In that case this patch looks correct to me.
>> 
>> Note that there is another `as` right after, in the same method:
>> 
>>   if irq < 0 {
>>       return Err(Error::from_errno(irq));
>>   }
>> 
>>   // SAFETY: `irq` is a valid IRQ number for `self.dev`, resolved from this registration.
>>   Ok(unsafe { IrqVector::new(IrqRequest::new(self.dev.as_ref(), irq as u32), self) })
>> 
>> 
>> We could get rid of it by replacing the `if irq < 0` test with:
>> 
>>   let irq = u32::try_from(irq).map_err(|_| Error::from_errno(irq))?;
>
> That's a good suggestion!

I expect this pattern to be common, so how about we add a variant of
`error::to_result` that returns the value as a `u32`? I.e.

  pub fn to_result_value(err: crate::ffi::c_int) -> Result<u32>

>
>> Sophon, if you feel like doing it I think this could improve the patch
>> further.
>
> Let's please do that as a separate patch though.

In this case, the current patch is

Reviewed-by: Alexandre Courbot <acourbot@xxxxxxxxxx>