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

[Auger Eric]

Hi Markus,

On 13/09/2016 08:25, Markus Armbruster wrote:
> Alex Williamson <address@hidden> writes:
> 
>> On Mon, 12 Sep 2016 16:00:18 +0200
>> Auger Eric <address@hidden> wrote:
>>
>>> Hi Markus,
>>>
>>> On 12/09/2016 14:45, Markus Armbruster wrote:
>>>> Eric Auger <address@hidden> 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.

Here the end-user omitted the host device and effectively the error
message isn't very useful in that case. I will improve that. Maybe I can
use error_append_hint.

In some other parts of the realize(), vfio_populate_device, msix_setup,
understanding which device caused the error is meaningful I think.

Typically when several devices are passthrough'ed, for instance:
upstream-qemu -device vfio-pci,host=0000:01:10.0 -device
vfio-pci,host=0000:01:10.4

> 
>>>> 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.
Yes that's really useful!

Thanks

Eric
> 
> [...]
>