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

[Peter Maydell]

On Thu, 13 Jun 2019 at 22:51, Alex Williamson
<address@hidden> wrote:
>
> From: Eric Auger <address@hidden>
>
> 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 <address@hidden>
> Reviewed-by: Cornelia Huck <address@hidden>
> Reviewed-by: Li Qiang <address@hidden>
> Signed-off-by: Alex Williamson <address@hidden>

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 ?

thanks
-- PMM