Re: [Qemu-devel] [PATCH] hw/vfio: improve error message when cannot init vfio event notifiers

[Markus Armbruster]

Jim Quigley <address@hidden> writes:

> On 16/10/2017 19:07, Michael Tokarev wrote:
>> 10.10.2017 13:22, Jim Quigley wrote:
>>> More information is required to assist trouble-shooting when
>>> QEMU fails to initialise the event notifications for devices
>>> assigned with VFIO-PCI. Instead of supplying the user with a cryptic
>>> error number only, print out a proper error message with strerror()
>>> so that the user has a better way to figure out what the problem is.
>>>
>>> Reviewed-by: Liam Merwick <address@hidden>
>>> Signed-off-by: Jim Quigley <address@hidden>
>>> ---
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> ---
>>>   hw/vfio/pci.c | 35 ++++++++++++++++++++++++-----------
>>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 31e1edf..3bffb93 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -430,13 +430,16 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, 
>>> bool msix)
>>>   static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector 
>>> *vector,
>>>                                     int vector_n, bool msix)
>>>   {
>>> -    int virq;
>>> +    int virq, ret;
>>>         if ((msix && vdev->no_kvm_msix) || (!msix &&
>>> vdev->no_kvm_msi)) {
>>>           return;
>>>       }
>>>   -    if (event_notifier_init(&vector->kvm_interrupt, 0)) {
>>> +    ret = event_notifier_init(&vector->kvm_interrupt, 0);
>>> +    if (ret) {
>>> +        error_report("vfio (%s): Error: unable to init event notifier: %s 
>>> (%d)",
>>> +                     __func__, strerror(-ret), -ret);
>> Since this pattern gets repeated again and again, maybe we can either

Parts of it are anti-patterns:

* Error messages are by definition meant for the user.  If you feel you
  need to include __func__ so it makes sense, you're obviously failing
  to address the end user.

* Likewise for the numeric encoding of errno.

* Printing "Error: " or similar is error_report()'s job.

That leaves printing strerror().

>> use a common wrapper or move that eror reporting into event_notifier_init()?
>> Note there are other users of this function, besides hw/vfio, and maybe
>> these, too, can benefit from better error reporting?
>
>     Ideally the strerror() would be included in the error_report()
> function,
>     (as per the error_setg() function), which obviously would involve
> a more
>     extensive change to the code base. Would that be an acceptable
> solution ?

All existing uses error_report() that print strerror() would have to be
converted to the new function.  Same for error_vreport(), warn_report(),
...  Whether that's worthwhile is not obvious until we see patches.

>     Or I can move the reporting into theevent_notifier_init() function
> if that is
>     the preferred approach ?

Moving error_report() into event_notifier_init() makes it unusable in
contexts where error_report() is unwanted or wrong.