Hi Peter, On 7/2/19 12:37 PM, Peter Maydell wrote: > On Thu, 13 Jun 2019 at 22:51, Alex Williamson > <[email protected]> wrote: >> >> From: Eric Auger <[email protected]> >> >> 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 <[email protected]> >> Reviewed-by: Cornelia Huck <[email protected]> >> Reviewed-by: Li Qiang <[email protected]> >> Signed-off-by: Alex Williamson <[email protected]> > > 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 >
