[2/3] vfio/pci: pass an error object to vfio_populate_device
diff mbox

Message ID 1473145893-17088-3-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Auger Eric Sept. 6, 2016, 7:11 a.m. UTC
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(-)

Comments

Markus Armbruster Sept. 12, 2016, 12:51 p.m. UTC | #1
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.

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

>      }
>  
>      if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
> -        error_report("vfio: unexpected number of io regions %u",
> -                     vbasedev->num_regions);
> -        goto error;
> +        error_setg(errp, "unexpected number of io regions %u",
> +                   vbasedev->num_regions);
> +        return;
>      }
>  
>      if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
> -        error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs);
> -        goto error;
> +        error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
> +        return;
>      }
>  
>      for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) {
> @@ -2229,8 +2229,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>          g_free(name);
>  
>          if (ret) {
> -            error_report("vfio: Error getting region %d info: %m", i);
> -            goto error;
> +            error_setg_errno(errp, ret, "failed to get region %d info", i);
> +            return;
>          }
>  
>          QLIST_INIT(&vdev->bars[i].quirks);
> @@ -2239,8 +2239,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>      ret = vfio_get_region_info(vbasedev,
>                                 VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
>      if (ret) {
> -        error_report("vfio: Error getting config info: %m");
> -        goto error;
> +        error_setg_errno(errp, ret, "failed to get config info");
> +        return;
>      }
>  
>      trace_vfio_populate_device_config(vdev->vbasedev.name,
> @@ -2259,9 +2259,9 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>      if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
>          ret = vfio_populate_vga(vdev);
>          if (ret) {
> -            error_report(
> -                "vfio: Device does not support requested feature x-vga");
> -            goto error;
> +            error_setg_errno(errp, ret,
> +                "device does not support requested feature x-vga");
> +            return;
>          }
>      }
>  
> @@ -2271,17 +2271,11 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>      if (ret) {
>          /* This can fail for an old kernel or legacy PCI dev */
>          trace_vfio_populate_device_get_irq_info_failure();
> -        ret = 0;
>      } else if (irq_info.count == 1) {
>          vdev->pci_aer = true;
>      } else {
> -        error_report("vfio: %s "
> -                     "Could not enable error recovery for the device",
> -                     vbasedev->name);
> +        error_setg_errno(errp, ret, "could not enable error recovery");

This isn't an error before this patch (ret stays zero).  Your patch
turns it into one.  Intentional?

>      }
> -
> -error:
> -    return ret;
>  }
>  
>  static void vfio_put_device(VFIOPCIDevice *vdev)
> @@ -2491,6 +2485,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>      VFIODevice *vbasedev_iter;
>      VFIOGroup *group;
>      char *tmp, group_path[PATH_MAX], *group_name;
> +    Error *err = NULL;
>      ssize_t len;
>      struct stat st;
>      int groupid;
> @@ -2554,9 +2549,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto error;
>      }
>  
> -    ret = vfio_populate_device(vdev);
> -    if (ret) {
> -        error_setg_errno(errp, ret, "failed to populate the device");
> +    vfio_populate_device(vdev, &err);
> +    if (err) {
> +        error_propagate(errp, err);
>          goto error;
>      }
Auger Eric Sept. 12, 2016, 3:15 p.m. UTC | #2
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?

> 
> Please mention the fix in the commit message.  Fixing it in a separate
> commit would also be fine, and possibly clearer.
> 
>>      }
>>  
>>      if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
>> -        error_report("vfio: unexpected number of io regions %u",
>> -                     vbasedev->num_regions);
>> -        goto error;
>> +        error_setg(errp, "unexpected number of io regions %u",
>> +                   vbasedev->num_regions);
>> +        return;
>>      }
>>  
>>      if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
>> -        error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs);
>> -        goto error;
>> +        error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
>> +        return;
>>      }
>>  
>>      for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) {
>> @@ -2229,8 +2229,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>          g_free(name);
>>  
>>          if (ret) {
>> -            error_report("vfio: Error getting region %d info: %m", i);
>> -            goto error;
>> +            error_setg_errno(errp, ret, "failed to get region %d info", i);
>> +            return;
>>          }
>>  
>>          QLIST_INIT(&vdev->bars[i].quirks);
>> @@ -2239,8 +2239,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>      ret = vfio_get_region_info(vbasedev,
>>                                 VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
>>      if (ret) {
>> -        error_report("vfio: Error getting config info: %m");
>> -        goto error;
>> +        error_setg_errno(errp, ret, "failed to get config info");
>> +        return;
>>      }
>>  
>>      trace_vfio_populate_device_config(vdev->vbasedev.name,
>> @@ -2259,9 +2259,9 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>      if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
>>          ret = vfio_populate_vga(vdev);
>>          if (ret) {
>> -            error_report(
>> -                "vfio: Device does not support requested feature x-vga");
>> -            goto error;
>> +            error_setg_errno(errp, ret,
>> +                "device does not support requested feature x-vga");
>> +            return;
>>          }
>>      }
>>  
>> @@ -2271,17 +2271,11 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>      if (ret) {
>>          /* This can fail for an old kernel or legacy PCI dev */
>>          trace_vfio_populate_device_get_irq_info_failure();
>> -        ret = 0;
>>      } else if (irq_info.count == 1) {
>>          vdev->pci_aer = true;
>>      } else {
>> -        error_report("vfio: %s "
>> -                     "Could not enable error recovery for the device",
>> -                     vbasedev->name);
>> +        error_setg_errno(errp, ret, "could not enable error recovery");
> 
> This isn't an error before this patch (ret stays zero).  Your patch
> turns it into one.  Intentional?
Hum no thank you for spotting this bug!

Thanks

Eric
> 
>>      }
>> -
>> -error:
>> -    return ret;
>>  }
>>  
>>  static void vfio_put_device(VFIOPCIDevice *vdev)
>> @@ -2491,6 +2485,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>      VFIODevice *vbasedev_iter;
>>      VFIOGroup *group;
>>      char *tmp, group_path[PATH_MAX], *group_name;
>> +    Error *err = NULL;
>>      ssize_t len;
>>      struct stat st;
>>      int groupid;
>> @@ -2554,9 +2549,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>          goto error;
>>      }
>>  
>> -    ret = vfio_populate_device(vdev);
>> -    if (ret) {
>> -        error_setg_errno(errp, ret, "failed to populate the device");
>> +    vfio_populate_device(vdev, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>>          goto error;
>>      }
>
Markus Armbruster Sept. 12, 2016, 3:50 p.m. UTC | #3
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.

[...]
Auger Eric Sept. 12, 2016, 3:52 p.m. UTC | #4
Hi Markus,

On 12/09/2016 17:50, Markus Armbruster wrote:
> 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.

OK Thanks!

Eric
> 
> [...]
>

Patch
diff mbox

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;
@@ -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;
     }
 
     if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
-        error_report("vfio: unexpected number of io regions %u",
-                     vbasedev->num_regions);
-        goto error;
+        error_setg(errp, "unexpected number of io regions %u",
+                   vbasedev->num_regions);
+        return;
     }
 
     if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
-        error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs);
-        goto error;
+        error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
+        return;
     }
 
     for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) {
@@ -2229,8 +2229,8 @@  static int vfio_populate_device(VFIOPCIDevice *vdev)
         g_free(name);
 
         if (ret) {
-            error_report("vfio: Error getting region %d info: %m", i);
-            goto error;
+            error_setg_errno(errp, ret, "failed to get region %d info", i);
+            return;
         }
 
         QLIST_INIT(&vdev->bars[i].quirks);
@@ -2239,8 +2239,8 @@  static int vfio_populate_device(VFIOPCIDevice *vdev)
     ret = vfio_get_region_info(vbasedev,
                                VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
     if (ret) {
-        error_report("vfio: Error getting config info: %m");
-        goto error;
+        error_setg_errno(errp, ret, "failed to get config info");
+        return;
     }
 
     trace_vfio_populate_device_config(vdev->vbasedev.name,
@@ -2259,9 +2259,9 @@  static int vfio_populate_device(VFIOPCIDevice *vdev)
     if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
         ret = vfio_populate_vga(vdev);
         if (ret) {
-            error_report(
-                "vfio: Device does not support requested feature x-vga");
-            goto error;
+            error_setg_errno(errp, ret,
+                "device does not support requested feature x-vga");
+            return;
         }
     }
 
@@ -2271,17 +2271,11 @@  static int vfio_populate_device(VFIOPCIDevice *vdev)
     if (ret) {
         /* This can fail for an old kernel or legacy PCI dev */
         trace_vfio_populate_device_get_irq_info_failure();
-        ret = 0;
     } else if (irq_info.count == 1) {
         vdev->pci_aer = true;
     } else {
-        error_report("vfio: %s "
-                     "Could not enable error recovery for the device",
-                     vbasedev->name);
+        error_setg_errno(errp, ret, "could not enable error recovery");
     }
-
-error:
-    return ret;
 }
 
 static void vfio_put_device(VFIOPCIDevice *vdev)
@@ -2491,6 +2485,7 @@  static void vfio_realize(PCIDevice *pdev, Error **errp)
     VFIODevice *vbasedev_iter;
     VFIOGroup *group;
     char *tmp, group_path[PATH_MAX], *group_name;
+    Error *err = NULL;
     ssize_t len;
     struct stat st;
     int groupid;
@@ -2554,9 +2549,9 @@  static void vfio_realize(PCIDevice *pdev, Error **errp)
         goto error;
     }
 
-    ret = vfio_populate_device(vdev);
-    if (ret) {
-        error_setg_errno(errp, ret, "failed to populate the device");
+    vfio_populate_device(vdev, &err);
+    if (err) {
+        error_propagate(errp, err);
         goto error;
     }