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

[Auger Eric]

Hi Markus,

On 12/09/2016 17:50, Markus Armbruster wrote:
> Auger Eric <address@hidden> writes:
> 
>> Hi Markus,
>>
>> On 12/09/2016 14:51, Markus Armbruster wrote:
>>> Eric Auger <address@hidden> writes:
>>>
>>>> Let's expand the usage of QEMU Error objects to vfio_populate_device.
>>>>
>>>> Signed-off-by: Eric Auger <address@hidden>
>>>> ---
>>>>  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.

OK Thanks!

Eric
> 
> [...]
>