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

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

[Jim Quigley]

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 <liam.merwick@oracle.com>
Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
---
Cc: qemu-trivial@nongnu.org
Cc: mjt@tls.msk.ru
Cc: laurent@vivier.eu
Cc: alex.williamson@redhat.com
---
 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);
         return;
     }
 
@@ -486,8 +489,11 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
     if (!vector->use) {
         vector->vdev = vdev;
         vector->virq = -1;
-        if (event_notifier_init(&vector->interrupt, 0)) {
-            error_report("vfio: Error: event_notifier_init failed");
+        ret = event_notifier_init(&vector->interrupt, 0);
+        if (ret) {
+            error_report("vfio (%s): Error: "
+                         "unable to init event notifier: %s (%d)",
+                         __func__, strerror(-ret), -ret);
         }
         vector->use = true;
         msix_vector_use(pdev, nr);
@@ -658,8 +664,11 @@ retry:
         vector->virq = -1;
         vector->use = true;
 
-        if (event_notifier_init(&vector->interrupt, 0)) {
-            error_report("vfio: Error: event_notifier_init failed");
+        ret = event_notifier_init(&vector->interrupt, 0);
+        if (ret) {
+            error_report("vfio (%s): Error: "
+                         "unable to init event notifier: %s (%d)",
+                         __func__, strerror(-ret), -ret);
         }
 
         qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
@@ -2471,8 +2480,10 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
         return;
     }
 
-    if (event_notifier_init(&vdev->err_notifier, 0)) {
-        error_report("vfio: Unable to init event notifier for error detection");
+    ret = event_notifier_init(&vdev->err_notifier, 0);
+    if (ret) {
+        error_report("vfio (%s): Error: unable to init event notifier: %s (%d)"
+            " for error detection", __func__, strerror(-ret), -ret);
         vdev->pci_aer = false;
         return;
     }
@@ -2553,7 +2564,7 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
 {
     struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
                                       .index = VFIO_PCI_REQ_IRQ_INDEX };
-    int argsz;
+    int argsz, ret;
     struct vfio_irq_set *irq_set;
     int32_t *pfd;
 
@@ -2566,8 +2577,10 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
         return;
     }
 
-    if (event_notifier_init(&vdev->req_notifier, 0)) {
-        error_report("vfio: Unable to init event notifier for device request");
+    ret = event_notifier_init(&vdev->req_notifier, 0);
+    if (ret) {
+        error_report("vfio (%s): Error: unable to init event notifier: %s (%d)"
+            " for device request", __func__, strerror(-ret), -ret);
         return;
     }
 
-- 
1.8.3.1

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

[Michael Tokarev]

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 <liam.merwick@oracle.com>
> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
> ---
> Cc: qemu-trivial@nongnu.org
> Cc: mjt@tls.msk.ru
> Cc: laurent@vivier.eu
> Cc: alex.williamson@redhat.com
> ---
>  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
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?

Thanks,

/mjt

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

[Jim Quigley]

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 <liam.merwick@oracle.com>
>> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
>> ---
>> Cc: qemu-trivial@nongnu.org
>> Cc: mjt@tls.msk.ru
>> Cc: laurent@vivier.eu
>> Cc: alex.williamson@redhat.com
>> ---
>>   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
> 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 ?
     Or I can move the reporting into theevent_notifier_init() function 
if that is
     the preferred approach ?

         thanks

         regards

         Jim Q.
>
> Thanks,
>
> /mjt
>
>

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

[Markus Armbruster]

Jim Quigley <jim.quigley@oracle.com> 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 <liam.merwick@oracle.com>
>>> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
>>> ---
>>> Cc: qemu-trivial@nongnu.org
>>> Cc: mjt@tls.msk.ru
>>> Cc: laurent@vivier.eu
>>> Cc: alex.williamson@redhat.com
>>> ---
>>>   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.