Re: [Qemu-devel] [PATCH v4 14/17] vfio/pci: Conversion to realize

[Auger Eric]

Hi Markus

On 04/10/2016 14:58, 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"
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>> v2 -> v3:
>> - use errp directly in all cases
>>
>> v1 -> v2:
>> - correct error_setg_errno with positive error values
>> ---
>>  hw/vfio/pci.c        | 68 
>> ++++++++++++++++++++++------------------------------
>>  hw/vfio/trace-events |  2 +-
>>  2 files changed, 29 insertions(+), 41 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 40ff4a7..b316e13 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2513,13 +2513,12 @@ 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;
>>      VFIOGroup *group;
>>      char *tmp, group_path[PATH_MAX], *group_name;
>> -    Error *err = NULL;
>>      ssize_t len;
>>      struct stat st;
>>      int groupid;
>> @@ -2533,9 +2532,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>      }
>>  
>>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
>> -        error_setg_errno(&err, errno, "no such host device");
>> -        ret = -errno;
>> -        goto error;
>> +        error_setg_errno(errp, errno, "no such host device");
>> +        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
>> +        return;
>>      }
>>  
>>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
>> @@ -2547,8 +2546,8 @@ static int vfio_initfn(PCIDevice *pdev)
>>      g_free(tmp);
>>  
>>      if (len <= 0 || len >= sizeof(group_path)) {
>> -        ret = len < 0 ? -errno : -ENAMETOOLONG;
>> -        error_setg_errno(&err, -ret, "no iommu_group found");
>> +        error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG,
>> +                         "no iommu_group found");
>>          goto error;
>>      }
>>  
>> @@ -2556,36 +2555,33 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      group_name = basename(group_path);
>>      if (sscanf(group_name, "%d", &groupid) != 1) {
>> -        error_setg_errno(&err, errno, "failed to read %s", group_path);
>> -        ret = -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), 
>> &err);
>> +    group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), 
>> errp);
>>      if (!group) {
>> -        ret = -ENOENT;
>>          goto error;
>>      }
>>  
>>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>>          if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
>> -            error_setg(&err, "device is already attached");
>> +            error_setg(errp, "device is already attached");
>>              vfio_put_group(group);
>> -            ret = -EBUSY;
>>              goto error;
>>          }
>>      }
>>  
>> -    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, 
>> &err);
>> +    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, 
>> errp);
>>      if (ret) {
>>          vfio_put_group(group);
>>          goto error;
>>      }
>>  
>> -    ret = vfio_populate_device(vdev, &err);
>> -    if (err) {
>> +    ret = vfio_populate_device(vdev, errp);
>> +    if (ret) {
> 
> The if (err) comes from PATCH 03.  You could reduce churn by checking
> ret from the start.  More of the same below.  Your choice.

I removed this spurious flip for vfio_populate_device and
vfio_msix_early_setup

Thanks

Eric
> 
>>          goto error;
>>      }
>>  
> [...]
>