Re: [PATCH v2 fixed 03/16] util: vfio-helpers: Remove Error parameter from qemu_vfio_undo_mapping()

[David Hildenbrand]

On 18.02.20 23:07, Peter Xu wrote:
> On Wed, Feb 12, 2020 at 02:42:41PM +0100, David Hildenbrand wrote:
>> Everybody discards the error. Let's error_report() instead so this error
>> doesn't get lost.
>>
>> Cc: Richard Henderson <address@hidden>
>> Cc: Paolo Bonzini <address@hidden>
>> Cc: Eduardo Habkost <address@hidden>
>> Cc: Marcel Apfelbaum <address@hidden>
>> Cc: Alex Williamson <address@hidden>
>> Cc: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: David Hildenbrand <address@hidden>
> 
> IMHO error_setg() should be preferred comparing to error_report()
> because it has a context to be delivered to the caller, so the error
> has a better chance to be used in a better way (e.g., QMP only
> supports error_setg()).

Please note that I decided to go for error_report() because that's also
what's being done in the matching opposite way: qemu_vfio_do_mapping().

> 
> A better solution is that we deliver the error upper.  For example,
> qemu_vfio_dma_map() is one caller of qemu_vfio_undo_mapping, if you
> see the callers of qemu_vfio_dma_map() you'll notice most of them has
> Error** defined (e.g., nvme_init_queue).  Then we can link all of them
> up.

Propagating errors is helpful if the caller can actually do something
with the error. If it's a function that's supposed to never fail (which
is the case here obviously), then there is not much benefit in doing so.

> 
> Another lazy solution (and especially if vfio-helpers are still mostly
> used only by advanced users) is we can simply pass in &error_abort for
> the three callers then they won't be missed...

I prefer this variant, as it matches qemu_vfio_do_mapping() - except
that we don't report -errno - which is fine, because the function is
expected to not fail in any sane use case.

Thanks!

-- 
Thanks,

David / dhildenb