[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH] hw/vfio: improve error message w
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH] hw/vfio: improve error message when cannot init vfio event notifiers |
Date: |
Mon, 13 Nov 2017 07:36:41 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
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.