Re: [PATCH v2 2/5] rust: pci: resolve IRQ in vector() and embed IrqRequest in IrqVector

[Gary Guo]

On Wed Aug 12, 2026 at 6:44 PM BST, Danilo Krummrich wrote:
> On Wed Aug 12, 2026 at 6:38 PM CEST, Gary Guo wrote:
>> On Wed Aug 12, 2026 at 12:39 AM BST, Danilo Krummrich wrote:
>>>      pub fn vector(&self, index: usize) -> Result<IrqVector<'_>> {
>>>          if index >= self.count.get() {
>>>              return Err(EINVAL);
>>>          }
>>>  
>>> -        // SAFETY: `index` is within bounds of this registration's allocation, and `self.dev` is
>>> -        // the device it was allocated from.
>>> -        Ok(unsafe { IrqVector::new(self.dev, self, index as u32) })
>>> +        // SAFETY: `self.dev.as_raw()` is a valid pointer to a `struct pci_dev`.
>>> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), index as u32) };
>>> +        if irq < 0 {
>>> +            return Err(Error::from_errno(irq));
>>> +        }
>>
>> Correct me if I'm wrong, but I believe that it's impossible for `pci_irq_vector`
>> once we have allocated vector and the index is in bounds. (If that's not the
>> case, we should ideally fix that instead.)
>
> You are correct, as of now it is unreachable with the index check above.
>
>> So I think we should just `.expect()` on the error in `Into`.
>
> I don't agree with the conclusion; I don't want this code to rely on an
> implementation detail of pci_irq_vector(), which (even though unlikely) could
> theoretically change.

I think this is expected use pattern of `pci_irq_vector`. Many C code don't
check the return code at all. If we want to mirror what C code do, we can also
just drop this error code check and rely on `irq as u32` below doing the correct
thing.

For both this and the EINVAL case for patch 1, my reasoning is that if the error
is never going to happen, then the code shouldn't be written as if it does, as
it will only add confusion to people reading the code.

I view these essentially as invariants, just not spelled out because it's
written in another language. In this case, basically you can say that
`pci_irq_vector(dev, index)` being successful is an invariant of
`IrqVectorRegistration` type.

>
> If we want to remove the redundancy, then we could maybe drop the index check
> above.

I think the index check should stay.

Best,
Gary

>
> (I also prefer IrqVector to be a new type over IrqRequest, as it also guarantees
> type wise that a valid IrqVector will always transform into a valid IrqRequest.)