Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

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

Hi Alex,

On 8/13/20 9:15 PM, Alex Williamson wrote:
> On Thu, 13 Aug 2020 20:02:45 +0200
> Auger Eric <eric.auger@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.chenqun@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.robot@huawei.com>
>>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>>> ---
>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>> Cc: Eric Auger <eric.auger@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
>