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

[Alex Williamson]

On Tue, 13 Sep 2016 09:21:17 +0200
Auger Eric  wrote:

> Hi Markus,
> 
> On 13/09/2016 08:25, Markus Armbruster wrote:
> > Alex Williamson  writes:
> >   
> >> On Mon, 12 Sep 2016 16:00:18 +0200
> >> Auger Eric  wrote:
> >>  
> >>> Hi Markus,
> >>>
> >>> On 12/09/2016 14:45, Markus Armbruster wrote:  
>  Eric Auger  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: :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: :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: :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: :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=:00:00.0 -device vfio-pci,host=: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: :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=:01:10.0 -device
> vfio-pci,host=: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
> > 
> > [...]
> >   

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  writes:
> 
>> On Mon, 12 Sep 2016 16:00:18 +0200
>> Auger Eric  wrote:
>>
>>> Hi Markus,
>>>
>>> On 12/09/2016 14:45, Markus Armbruster wrote:
 Eric Auger  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: :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: :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: :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: :00:00.0" is gobbledygook
> unless you're somewhat familiar with vfio, and then it's redundant.
> 
> The "vfio: :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=:01:10.0 -device
vfio-pci,host=: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
> 
> [...]
> 

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

[Markus Armbruster]

Alex Williamson  writes:

> On Mon, 12 Sep 2016 16:00:18 +0200
> Auger Eric  wrote:
>
>> Hi Markus,
>> 
>> On 12/09/2016 14:45, Markus Armbruster wrote:
>> > Eric Auger  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: :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: :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: :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: :00:00.0" is gobbledygook
unless you're somewhat familiar with vfio, and then it's redundant.

The "vfio: :00:00.0" *is* useful in messages outside realize()
context, because then the system can't prefix the "-device vfio-pci"
part.

>> > 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.

[...]

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

[Auger Eric]

Hi,

On 12/09/2016 22:05, Alex Williamson wrote:
> On Mon, 12 Sep 2016 21:39:22 +0200
> Auger Eric  wrote:
> 
>> Hi,
>> On 12/09/2016 18:17, Alex Williamson wrote:
>>> On Mon, 12 Sep 2016 16:00:18 +0200
>>> Auger Eric  wrote:
>>>   
 Hi Markus,

 On 12/09/2016 14:45, Markus Armbruster wrote:  
> Eric Auger  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: :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: :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?
> 
> I don't really see the point of making the device name smaller.  If a
> user happens to have multiple domains, they're going to care about that
> component of the address.  Is QEMU going to probe the host system to
> see if multiple domains are available and update if a new PCI chassis
> handled as a separate domain is hot attached?  Even an approach like
> only printing the domain if it's non-zero devolves into needing logic
> to know that basename is a PCI path and not a random sysfs device
> path.  And then if we only print the domain when non-zero, what about
> the bus number or slot number.  It's a lot of logic for a problem that
> I'm not even convinced is a problem.  Thanks,
I tend to agree. So I will keep the prefix as is and take into account
Markus' other comments.

Thanks!

Eric
> 
> Alex
> 

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

[Alex Williamson]

On Mon, 12 Sep 2016 21:39:22 +0200
Auger Eric  wrote:

> Hi,
> On 12/09/2016 18:17, Alex Williamson wrote:
> > On Mon, 12 Sep 2016 16:00:18 +0200
> > Auger Eric  wrote:
> >   
> >> Hi Markus,
> >>
> >> On 12/09/2016 14:45, Markus Armbruster wrote:  
> >>> Eric Auger  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: :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: :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?

I don't really see the point of making the device name smaller.  If a
user happens to have multiple domains, they're going to care about that
component of the address.  Is QEMU going to probe the host system to
see if multiple domains are available and update if a new PCI chassis
handled as a separate domain is hot attached?  Even an approach like
only printing the domain if it's non-zero devolves into needing logic
to know that basename is a PCI path and not a random sysfs device
path.  And then if we only print the domain when non-zero, what about
the bus number or slot number.  It's a lot of logic for a problem that
I'm not even convinced is a problem.  Thanks,

Alex

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

[Auger Eric]

Hi,
On 12/09/2016 18:17, Alex Williamson wrote:
> On Mon, 12 Sep 2016 16:00:18 +0200
> Auger Eric  wrote:
> 
>> Hi Markus,
>>
>> On 12/09/2016 14:45, Markus Armbruster wrote:
>>> Eric Auger  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: :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: :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 
 ---
  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, ) < 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", ) != 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.  

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

[Alex Williamson]

On Mon, 12 Sep 2016 16:00:18 +0200
Auger Eric  wrote:

> Hi Markus,
> 
> On 12/09/2016 14:45, Markus Armbruster wrote:
> > Eric Auger  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: :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: :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 
> >> ---
> >>  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, ) < 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", ) != 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 

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

[Auger Eric]

Hi Markus,

On 12/09/2016 14:45, Markus Armbruster wrote:
> Eric Auger  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: :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: :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?
> 
> 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 
>> ---
>>  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, ) < 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", ) != 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.
> 
>>  }
>>  
>>  QLIST_FOREACH(vbasedev_iter, >device_list, next) {
>>  if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
>> -error_report("vfio: error: device %s is already attached",
>> - vdev->vbasedev.name);
>> +error_setg_errno(errp, -EBUSY, "device is already attached");
>>  vfio_put_group(group);
>> -return -EBUSY;
>> +goto error;
>>  }
>>  }
>>  
>>  ret = vfio_get_device(group, vdev->vbasedev.name, >vbasedev);
>>  if (ret) {
>> -error_report("vfio: failed to get device %s", vdev->vbasedev.name);
>> +error_setg_errno(errp, ret, "failed to get the device");
>>  vfio_put_group(group);
>> -return ret;
>> +goto error;
>>  }
>>  
>>  ret = vfio_populate_device(vdev);
>>  if (ret) {
>> -return ret;
>> +error_setg_errno(errp, ret, "failed to populate the device");
>> +goto error;
>>  }

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

[Markus Armbruster]

Eric Auger  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: :00:00.0: no iommu_group found: 
Unknown error -2

Two remarks:

* "Unknown error -2" should be "No such file or directory".  See below.

* Five colons, not counting the ones in the PCI address.  Do we need the
  "vfio: :00:00.0:" part?  If yes, could we find a nicer way to
  print it?  Up to you.

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 
> ---
>  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, ) < 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.

> + 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", ) != 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.

>  }
>  
>  QLIST_FOREACH(vbasedev_iter, >device_list, next) {
>  if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
> -error_report("vfio: error: device %s is already attached",
> - vdev->vbasedev.name);
> +error_setg_errno(errp, -EBUSY, "device is already attached");
>  vfio_put_group(group);
> -return -EBUSY;
> +goto error;
>  }
>  }
>  
>  ret = vfio_get_device(group, vdev->vbasedev.name, >vbasedev);
>  if (ret) {
> -error_report("vfio: failed to get device %s", vdev->vbasedev.name);
> +error_setg_errno(errp, ret, "failed to get the device");
>  vfio_put_group(group);
> -return ret;
> +goto error;
>  }
>  
>  ret = vfio_populate_device(vdev);
>  if (ret) {
> -return ret;
> +error_setg_errno(errp, ret, "failed to populate the device");
> +goto error;
>  }

vfio_populate_device() reports errors with error_report().  We get a
specific error via error_report(), and a generic error here.  PATCH 2
converts it to Error, getting rid of the generic error again.  Works for
me, but I usually order conversion patches the other way: first convert
vfio_populate_device(), and report its Error like this:

ret = vfio_populate_device(vdev, );
if (err) {
error_report_err(err);
return ret;
}

PRO: no intermediate state with two errors.  CON: have to touch

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

[Eric Auger]

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"

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 
---
 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, ) < 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",
+ 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", ) != 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;
 }
 
 QLIST_FOREACH(vbasedev_iter, >device_list, next) {
 if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
-error_report("vfio: error: device %s is already attached",
- vdev->vbasedev.name);
+error_setg_errno(errp, -EBUSY, "device is already attached");
 vfio_put_group(group);
-return -EBUSY;
+goto error;
 }
 }
 
 ret = vfio_get_device(group, vdev->vbasedev.name, >vbasedev);
 if (ret) {
-error_report("vfio: failed to get device %s", vdev->vbasedev.name);
+error_setg_errno(errp, ret, "failed to get the device");
 vfio_put_group(group);
-return ret;
+goto error;
 }
 
 ret = vfio_populate_device(vdev);
 if (ret) {
-return ret;
+error_setg_errno(errp, ret, "failed to populate the device");
+goto error;
 }
 
 /* Get a copy of config space */
@@ -2565,8 +2566,8 @@ static int vfio_initfn(PCIDevice *pdev)
 vdev->config_offset);
 if (ret < (int)MIN(pci_config_size(>pdev), vdev->config_size)) {
 ret = ret < 0 ? -errno : -EFAULT;
-error_report("vfio: Failed to read device config space");
-return ret;
+error_setg_errno(errp, ret, "failed to read device config space");
+goto error;
 }
 
 /* vfio emulates a lot for us, but some bits need extra love */
@@ -2582,8 +2583,8 @@ static int vfio_initfn(PCIDevice *pdev)
  */
 if (vdev->vendor_id != PCI_ANY_ID) {
 if (vdev->vendor_id >= 0x) {
-error_report("vfio: Invalid PCI vendor ID provided");
-return -EINVAL;
+error_setg_errno(errp, -EINVAL, "invalid PCI vendor ID provided");
+goto error;
 }
 vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
 trace_vfio_pci_emulated_vendor_id(vdev->vbasedev.name, 
vdev->vendor_id);
@@ -2593,8 +2594,8 @@ static int vfio_initfn(PCIDevice *pdev)
 
 if (vdev->device_id != PCI_ANY_ID) {
 if (vdev->device_id > 0x) {
-error_report("vfio: Invalid PCI device ID provided");
-return -EINVAL;
+error_setg_errno(errp, -EINVAL, "invalid PCI device ID provided");
+goto error;
 }
 vfio_add_emulated_word(vdev, PCI_DEVICE_ID,