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

[Auger Eric <eric.auger@redhat.com>]

Hi,
On 12/09/2016 18:17, Alex Williamson wrote:
> 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.
On my side I would be inclined to keep the "vfio: BDF" prefix. Maybe the
PCI domain may be omitted?
> 
>>>
>>> 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 <eric.auger@redhat.com>
>>>> ---
>>>>  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,
OK in that case I will try to improve whenever it makes sense.

Thanks

Eric
> 
> Alex
>