Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize

[Markus Armbruster <armbru@redhat.com>]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Mon, 12 Sep 2016 16:00:18 +0200
> Auger Eric <eric.auger@redhat.com> wrote:
>
>> Hi Markus,
>> 
>> On 12/09/2016 14:45, Markus Armbruster wrote:
>> > Eric Auger <eric.auger@redhat.com> writes:
>> >   
>> >> This patch converts VFIO PCI to realize function.
>> >>
>> >> Also original initfn errors now are propagated using QEMU
>> >> error objects. All errors are formatted with the same pattern:
>> >> "vfio: %s: the error description"  
>> > 
>> > Example:
>> > 
>> > $ upstream-qemu -device vfio-pci
>> > upstream-qemu: -device vfio-pci: vfio: 0000:00:00.0: no iommu_group found: Unknown error -2
>> > 
>> > Two remarks:
>> > 
>> > * "Unknown error -2" should be "No such file or directory".  See below.  
>> Hum. I noticed that but I didn't have the presence of mind to get it was
>> due to -errno!
>> > 
>> > * Five colons, not counting the ones in the PCI address.  Do we need the
>> >   "vfio: 0000:00:00.0:" part?  If yes, could we find a nicer way to
>> >   print it?  Up to you.  
>> Well we have quite a lot of traces with such format, hence my choice.
>> Alex do you want to change this?
>
> Well, we need to identify the component with the error, it's not
> uncommon to have multiple assigned devices.  The PCI address is just
> the basename in vfio code, it might also be the name of a device node
> in sysfs, such as a uuid of an mdev devices.  AFAIK we cannot count on
> a id and even if we could libvirt assigns them based on order in the
> xml, making them a bit magic.  Maybe I'm not understanding the
> question.  Regarding trace vs error message, I expect that it's going
> to be a more advanced user/developer enabling tracing, error reports
> should try a little harder to be comprehensible to an average user.

Yes, the error message needs to identify the part that's wrong.
However, we need to consider the *complete* error message to judge it.
Consider:

    $ upstream-qemu -device vfio-pci
    upstream-qemu: -device vfio-pci: vfio: 0000:00:00.0: no iommu_group found: No such file or directory

Which parts are actually useful for the user?  "-device vfio-pci"
identifies the part that's wrong.  "vfio: 0000:00:00.0" is gobbledygook
unless you're somewhat familiar with vfio, and then it's redundant.

The "vfio: 0000:00:00.0" *is* useful in messages outside realize()
context, because then the system can't prefix the "-device vfio-pci"
part.

>> > Always, always, always test your error messages :)

Because what you think you see in the code may differ from what the user
will see.

Anyway, your choice, I'm just giving feedback on the error messages I
observe in testing.

[...]