Re: [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device

[Markus Armbruster <armbru@redhat.com>]

Auger Eric <eric.auger@redhat.com> writes:

> Hi Markus,
>
> On 12/09/2016 14:51, Markus Armbruster wrote:
>> Eric Auger <eric.auger@redhat.com> writes:
>> 
>>> Let's expand the usage of QEMU Error objects to vfio_populate_device.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  hw/vfio/pci.c | 45 ++++++++++++++++++++-------------------------
>>>  1 file changed, 20 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index ae1967c..f7768e9 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -2197,7 +2197,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
>>>      return 0;
>>>  }
>>>  
>>> -static int vfio_populate_device(VFIOPCIDevice *vdev)
>>> +static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>>>  {
>>>      VFIODevice *vbasedev = &vdev->vbasedev;
>>>      struct vfio_region_info *reg_info;
>>        struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>>        int i, ret = -1;
>>> @@ -2206,19 +2206,19 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>>  
>>>      /* Sanity check device */
>>>      if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
>>> -        error_report("vfio: Um, this isn't a PCI device");
>>> -        goto error;
>>> +        error_setg(errp, "this isn't a PCI device");
>>> +        return;
>> 
>> This is actually a bug fix :)
>> 
>> Before your series, vfio_populate_device() returns negative errno on
>> some errors, and -1 on others.  Its caller expects the former.
>
> Sorry but I don't get your comment. Who is the caller you refer to?

Correction: its caller vfio_initfn() doesn't actually expect -errno.
Regardless, mixing -errno and -1 like vfio_populate_device() does in
master is in bad taste.  So this isn't a bug fix, just a cleanup.

>> Please mention the fix in the commit message.  Fixing it in a separate
>> commit would also be fine, and possibly clearer.

Mentioning the cleanup wouldn't hurt.

[...]