Hi Markus, On 13/09/2016 08:25, Markus Armbruster wrote: > Alex Williamson <alex.william...@redhat.com> writes: > >> On Mon, 12 Sep 2016 16:00:18 +0200 >> Auger Eric <eric.au...@redhat.com> wrote: >> >>> Hi Markus, >>> >>> On 12/09/2016 14:45, Markus Armbruster wrote: >>>> Eric Auger <eric.au...@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.
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 > > [...] >
