On Tue, 13 Sep 2016 09:21:17 +0200 Auger Eric <eric.au...@redhat.com> wrote:
> 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. I think you're identifying a bug in our ability to detect whether DEFINE_PROP_PCI_HOST_DEVADDR has been set or not. If a user had instead run: -device vfio-pci,host=0000:00:00.0 -device vfio-pci,host=0000:00:00.1 Then yes, the distinction between zeros and .1 is useful, but without specifying a host or sysfsdev, we need a new error path. The "vfio:" may certainly be redundant when "-device vfio-pci" is already pre-pended to the error message. > > > > 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. Right, it seems like this needs to be detected and a new error path added. We require either a host= or sysfsdev= option and should never try to use an unset "host" value. Thanks, Alex > 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 > > > > [...] > >
