Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,

On 7/2/19 12:37 PM, Peter Maydell wrote:
> On Thu, 13 Jun 2019 at 22:51, Alex Williamson
> <alex.williamson@redhat.com> wrote:
>>
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> The code used to assign an interrupt index/subindex to an
>> eventfd is duplicated many times. Let's introduce an helper that
>> allows to set/unset the signaling for an ACTION_TRIGGER,
>> ACTION_MASK or ACTION_UNMASK action.
>>
>> In the error message, we now use errno in case of any
>> VFIO_DEVICE_SET_IRQS ioctl failure.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Reviewed-by: Li Qiang <liq3ea@gmail.com>
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Hi; coverity reports (CID 1402196) a possible unchecked return value
> in this code:
> 
> 
>> @@ -592,26 +550,10 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>>       * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
>>       */
>>      if (vector->virq >= 0) {
>> -        int argsz;
>> -        struct vfio_irq_set *irq_set;
>> -        int32_t *pfd;
>> -
>> -        argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -        irq_set = g_malloc0(argsz);
>> -        irq_set->argsz = argsz;
>> -        irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                         VFIO_IRQ_SET_ACTION_TRIGGER;
>> -        irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
>> -        irq_set->start = nr;
>> -        irq_set->count = 1;
>> -        pfd = (int32_t *)&irq_set->data;
>> +        int32_t fd = event_notifier_get_fd(&vector->interrupt);
>>
>> -        *pfd = event_notifier_get_fd(&vector->interrupt);
>> -
>> -        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -
>> -        g_free(irq_set);
>> +        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
>> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);
> 
> In vfp_msix_vector_release() we call vfio_set_irq_signaling(),
> but we don't check the returned error value, whereas in the other
> 7 places we call the function we do check. Is there some missing
> error handling here ?

the difference with the other calls is that we pass a NULL errp here so
we don't need to consume a potential error. Before the introduction of
vfio_set_irq_signaling we had an ioctl call whose returned value was not
tested either. So I think it properly translates what was done before.
It seems we are willingly not producing any error message in that case.
Alex, can you confirm?

Thanks

Eric
> 
> thanks
> -- PMM
>