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

[Alex Williamson]

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.

> > 
> > Always, always, always test your error messages :)
> >   
> >> Subsequent patches will pass the error objects to
> >> - vfio_populate_device,
> >> - vfio_msix_early_setup.
> >>
> >> At this point if those functions fail let's just report an error
> >> at realize level.
> >>
> >> Signed-off-by: Eric Auger <address@hidden>
> >> ---
> >>  hw/vfio/pci.c        | 81 
> >> ++++++++++++++++++++++++++++------------------------
> >>  hw/vfio/trace-events |  2 +-
> >>  2 files changed, 44 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index 7bfa17c..ae1967c 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -2485,7 +2485,7 @@ static void 
> >> vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> >>      vdev->req_enabled = false;
> >>  }
> >>  
> >> -static int vfio_initfn(PCIDevice *pdev)
> >> +static void vfio_realize(PCIDevice *pdev, Error **errp)
> >>  {
> >>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> >>      VFIODevice *vbasedev_iter;
> >> @@ -2504,9 +2504,9 @@ static int vfio_initfn(PCIDevice *pdev)
> >>      }
> >>  
> >>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
> >> -        error_report("vfio: error: no such host device: %s",
> >> -                     vdev->vbasedev.sysfsdev);
> >> -        return -errno;
> >> +        error_setg_errno(errp, -errno, "vfio: error: no such host device: 
> >> %s",  
> > 
> > error_setg_errno() takes a *positive* errno.  Same elsewhere.  
> OK thanks!
> >   
> >> +                         vdev->vbasedev.sysfsdev);
> >> +        return;
> >>      }
> >>  
> >>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
> >> @@ -2518,45 +2518,46 @@ static int vfio_initfn(PCIDevice *pdev)
> >>      g_free(tmp);
> >>  
> >>      if (len <= 0 || len >= sizeof(group_path)) {
> >> -        error_report("vfio: error no iommu_group for device");
> >> -        return len < 0 ? -errno : -ENAMETOOLONG;
> >> +        error_setg_errno(errp, len < 0 ? -errno : -ENAMETOOLONG,
> >> +                         "no iommu_group found");
> >> +        goto error;
> >>      }
> >>  
> >>      group_path[len] = 0;
> >>  
> >>      group_name = basename(group_path);
> >>      if (sscanf(group_name, "%d", &groupid) != 1) {
> >> -        error_report("vfio: error reading %s: %m", group_path);
> >> -        return -errno;
> >> +        error_setg_errno(errp, -errno, "failed to read %s", group_path);
> >> +        goto error;
> >>      }
> >>  
> >> -    trace_vfio_initfn(vdev->vbasedev.name, groupid);
> >> +    trace_vfio_realize(vdev->vbasedev.name, groupid);
> >>  
> >>      group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
> >>      if (!group) {
> >> -        error_report("vfio: failed to get group %d", groupid);
> >> -        return -ENOENT;
> >> +        error_setg_errno(errp, -ENOENT, "failed to get group %d", 
> >> groupid);
> >> +        goto error;  
> > 
> > I understand this is a mechanical conversion, but are you sure the ": No
> > such file or directory" suffix we get from passing ENOENT buys us
> > anything?  More of the same below.  
> At that stage I prefered to stick to the original messages since Alex &
> VFIO users may have their habits.

ENOENT may be a relic from previous conversions.  In general I have no
attachment to any of our error messages.  I might pay more attention to
tracing since I definitely don't want to lose functionality there, but
for errors, so long as it's unique and descriptive, please update as
you see fit.  You can always use google to see how common a particular
error is and whether significantly rewording it could further confuse
or help users.  Thanks,

Alex