Hi Alex, On 8/13/20 9:15 PM, Alex Williamson wrote: > On Thu, 13 Aug 2020 20:02:45 +0200 > Auger Eric <eric.au...@redhat.com> wrote: > >> Hi Alex, >> >> On 8/13/20 6:59 PM, Alex Williamson wrote: >>> On Thu, 13 Aug 2020 15:37:08 +0800 >>> Chen Qun <kuhn.chen...@huawei.com> wrote: >>> >>>> Clang static code analyzer show warning: >>>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read >>>> ret = event_notifier_test_and_clear(intp->interrupt); >>>> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> >>>> Reported-by: Euler Robot <euler.ro...@huawei.com> >>>> Signed-off-by: Chen Qun <kuhn.chen...@huawei.com> >>>> --- >>>> Cc: Alex Williamson <alex.william...@redhat.com> >>>> Cc: Eric Auger <eric.au...@redhat.com> >>>> --- >>>> hw/vfio/platform.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c >>>> index ac2cefc9b1..869ed2c39d 100644 >>>> --- a/hw/vfio/platform.c >>>> +++ b/hw/vfio/platform.c >>>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp) >>>> trace_vfio_intp_interrupt_set_pending(intp->pin); >>>> QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue, >>>> intp, pqnext); >>>> - ret = event_notifier_test_and_clear(intp->interrupt); >>>> + event_notifier_test_and_clear(intp->interrupt); >>>> return; >>>> } >>> >>> Testing that an event is pending in our notifier is generally a >>> prerequisite to doing anything in the interrupt handler, I don't >>> understand why we're just consuming it and ignoring the return value. >>> The above is in the delayed handling branch of the function, but the >>> normal non-delayed path would only go on to error_report() if the >>> notifier is not pending and then inject an interrupt anyway. This all >>> seems rather suspicious and it's a unique pattern among the vfio >>> callers of this function. Is there a more fundamental bug that this >>> function should perform this test once and return without doing >>> anything if it's called spuriously, ie. without a notifier pending? >>> Thanks, >> >> Hum that's correct that other VFIO call sites do the check. My >> understanding was that this could not fail in this case as, if we >> entered the handler there was something to be cleared. In which >> situation can this fail? > > I'm not sure what the right answer is, I see examples either way > looking outside of vfio code. On one hand, maybe we never get called > spuriously, on the other if it's the callee's responsibility to drain > events from the fd and we have it readily accessible whether there were > any events pending, why would we inject an interrupt if the result that > we have in hand shows no pending events? The overhead of returning > based on that result is minuscule.
I agree > > qemu_set_fd_handler() is a wrapper for aio_set_fd_handler(). Stefan is > a possible defacto maintainer of some of the aio code. Stefan, do you > have thoughts on whether callbacks from event notifier fds should > consider spurious events? Thanks, Indeed I saw that for instance block/nvme.c nvme_handle_event is not checking the result. Let's wait for Stefan's answer ... Thanks Eric > > Alex >
