[v2,01/12] vfio/pci: Use local error object in vfio_initfn
diff mbox

Message ID 1474404352-28958-2-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Auger Eric Sept. 20, 2016, 8:45 p.m. UTC
To prepare for migration to realize, let's use a local error
object in vfio_initfn. Also let's use the same error prefix for all
error messages.

On top of the 1-1 conversion, we start using a common error prefix for
all error messages. We also introduce a similar warning prefix which will
be used later on.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---

v2: creation
---
 hw/vfio/pci.c                 | 73 +++++++++++++++++++++++++------------------
 include/hw/vfio/vfio-common.h |  3 ++
 2 files changed, 46 insertions(+), 30 deletions(-)

Comments

Markus Armbruster Sept. 22, 2016, 3:42 p.m. UTC | #1
Eric Auger <eric.auger@redhat.com> writes:

> To prepare for migration to realize, let's use a local error
> object in vfio_initfn. Also let's use the same error prefix for all
> error messages.
>
> On top of the 1-1 conversion, we start using a common error prefix for
> all error messages. We also introduce a similar warning prefix which will
> be used later on.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>
> v2: creation
> ---
>  hw/vfio/pci.c                 | 73 +++++++++++++++++++++++++------------------
>  include/hw/vfio/vfio-common.h |  3 ++
>  2 files changed, 46 insertions(+), 30 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index a5a620a..ca0293d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2493,6 +2493,7 @@ static int vfio_initfn(PCIDevice *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;
> @@ -2506,9 +2507,9 @@ static int vfio_initfn(PCIDevice *pdev)
>      }
>  
>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
> -        error_report("vfio: error: no such host device: %s",
> -                     vdev->vbasedev.sysfsdev);
> -        return -errno;
> +        error_setg(&err, "no such host device");

Any particular reason you don't use error_setg_errno() here?

> +        ret = -errno;
> +        goto error;
>      }
>  
>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
> @@ -2520,40 +2521,44 @@ 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(&err, len < 0 ? errno : ENAMETOOLONG,
> +                         "no iommu_group found");
> +        ret = len < 0 ? -errno : -ENAMETOOLONG;

Suggest

           ret = len < 0 ? -errno : -ENAMETOOLONG;
           error_setg_errno(&err, -ret, "no iommu_group found");

> +        goto error;
>      }
>  
>      group_path[len] = 0;
>  
>      group_name = basename(group_path);
>      if (sscanf(group_name, "%d", &groupid) != 1) {
> -        error_report("vfio: error reading %s: %m", group_path);
> -        return -errno;
> +        error_setg_errno(&err, errno, "failed to read %s", group_path);
> +        ret = -errno;
> +        goto error;
>      }
>  
>      trace_vfio_initfn(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(&err, "failed to get group %d", groupid);
> +        ret = -ENOENT;
> +        goto error;
>      }
>  
>      QLIST_FOREACH(vbasedev_iter, &group->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(&err, "device is already attached");
>              vfio_put_group(group);
> -            return -EBUSY;
> +            ret = -EBUSY;
> +            goto error;
>          }
>      }
>  
>      ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
>      if (ret) {
> -        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
> +        error_setg_errno(&err, -ret, "failed to get device");
>          vfio_put_group(group);
> -        return ret;
> +        goto error;
>      }
>  
>      ret = vfio_populate_device(vdev);
> @@ -2567,8 +2572,8 @@ static int vfio_initfn(PCIDevice *pdev)
>                  vdev->config_offset);
>      if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
>          ret = ret < 0 ? -errno : -EFAULT;
> -        error_report("vfio: Failed to read device config space");
> -        return ret;
> +        error_setg_errno(&err, -ret, "failed to read device config space");
> +        goto error;
>      }
>  
>      /* vfio emulates a lot for us, but some bits need extra love */
> @@ -2584,8 +2589,9 @@ static int vfio_initfn(PCIDevice *pdev)
>       */
>      if (vdev->vendor_id != PCI_ANY_ID) {
>          if (vdev->vendor_id >= 0xffff) {
> -            error_report("vfio: Invalid PCI vendor ID provided");
> -            return -EINVAL;
> +            error_setg(&err, "invalid PCI vendor ID provided");
> +            ret = -EINVAL;
> +            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);
> @@ -2595,8 +2601,9 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->device_id != PCI_ANY_ID) {
>          if (vdev->device_id > 0xffff) {
> -            error_report("vfio: Invalid PCI device ID provided");
> -            return -EINVAL;
> +            error_setg(&err, "invalid PCI device ID provided");
> +            ret = -EINVAL;
> +            goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
>          trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id);
> @@ -2606,8 +2613,9 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->sub_vendor_id != PCI_ANY_ID) {
>          if (vdev->sub_vendor_id > 0xffff) {
> -            error_report("vfio: Invalid PCI subsystem vendor ID provided");
> -            return -EINVAL;
> +            error_setg(&err, "invalid PCI subsystem vendor ID provided");
> +            ret = -EINVAL;
> +            goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
>                                 vdev->sub_vendor_id, ~0);
> @@ -2617,8 +2625,9 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->sub_device_id != PCI_ANY_ID) {
>          if (vdev->sub_device_id > 0xffff) {
> -            error_report("vfio: Invalid PCI subsystem device ID provided");
> -            return -EINVAL;
> +            error_setg(&err, "invalid PCI subsystem device ID provided");
> +            ret = -EINVAL;
> +            goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
>          trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name,
> @@ -2671,8 +2680,9 @@ static int vfio_initfn(PCIDevice *pdev)
>          struct vfio_region_info *opregion;
>  
>          if (vdev->pdev.qdev.hotplugged) {
> -            error_report("Cannot support IGD OpRegion feature on hotplugged "
> -                         "device %s", vdev->vbasedev.name);
> +            error_setg(&err,
> +                       "cannot support IGD OpRegion feature on hotplugged "
> +                       "device");
>              ret = -EINVAL;
>              goto out_teardown;
>          }
> @@ -2681,16 +2691,15 @@ static int vfio_initfn(PCIDevice *pdev)
>                          VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>                          VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
>          if (ret) {
> -            error_report("Device %s does not support requested IGD OpRegion "
> -                         "feature", vdev->vbasedev.name);
> +            error_setg_errno(&err, -ret,
> +                             "does not support requested IGD OpRegion feature");
>              goto out_teardown;
>          }
>  
>          ret = vfio_pci_igd_opregion_init(vdev, opregion);
>          g_free(opregion);
>          if (ret) {
> -            error_report("Device %s IGD OpRegion initialization failed",
> -                         vdev->vbasedev.name);
> +            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
>              goto out_teardown;
>          }
>      }
> @@ -2726,6 +2735,10 @@ out_teardown:
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>      vfio_teardown_msi(vdev);
>      vfio_bars_exit(vdev);
> +error:
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    }
>      return ret;
>  }

The value assigned to ret doesn't matter as long as it's negative, which
makes me wonder why we bother with different values here.  Peeking
ahead, I see you simplify this already as part of your conversion to
realize().  Okay, that works for me.

>  
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 94dfae3..fd19880 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -30,6 +30,9 @@
>  #include <linux/vfio.h>
>  #endif
>  
> +#define ERR_PREFIX "vfio error: %s: "
> +#define WARN_PREFIX "vfio warning: %s: "
> +
>  /*#define DEBUG_VFIO*/
>  #ifdef DEBUG_VFIO
>  #define DPRINTF(fmt, ...) \

We already discussed error message prefixes and errno strings.  I got my
taste, you got yours.  Since this is your code, yours wins :)
Auger Eric Sept. 30, 2016, 1:35 p.m. UTC | #2
Hi Markus,

On 22/09/2016 17:42, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> To prepare for migration to realize, let's use a local error
>> object in vfio_initfn. Also let's use the same error prefix for all
>> error messages.
>>
>> On top of the 1-1 conversion, we start using a common error prefix for
>> all error messages. We also introduce a similar warning prefix which will
>> be used later on.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>
>> v2: creation
>> ---
>>  hw/vfio/pci.c                 | 73 +++++++++++++++++++++++++------------------
>>  include/hw/vfio/vfio-common.h |  3 ++
>>  2 files changed, 46 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index a5a620a..ca0293d 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2493,6 +2493,7 @@ static int vfio_initfn(PCIDevice *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;
>> @@ -2506,9 +2507,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>      }
>>  
>>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
>> -        error_report("vfio: error: no such host device: %s",
>> -                     vdev->vbasedev.sysfsdev);
>> -        return -errno;
>> +        error_setg(&err, "no such host device");
> 
> Any particular reason you don't use error_setg_errno() here?
I hesitated and eventually removed into thinking it was not bringing
much value. But not strong opinion here, I will restore it.
> 
>> +        ret = -errno;
>> +        goto error;
>>      }
>>  
>>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
>> @@ -2520,40 +2521,44 @@ 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(&err, len < 0 ? errno : ENAMETOOLONG,
>> +                         "no iommu_group found");
>> +        ret = len < 0 ? -errno : -ENAMETOOLONG;
> 
> Suggest
> 
>            ret = len < 0 ? -errno : -ENAMETOOLONG;
>            error_setg_errno(&err, -ret, "no iommu_group found");
Sure. Eventually ret will disappear so we will come back to
error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
>> +                         "no iommu_group found");
> 
>> +        goto error;
>>      }
>>  
>>      group_path[len] = 0;
>>  
>>      group_name = basename(group_path);
>>      if (sscanf(group_name, "%d", &groupid) != 1) {
>> -        error_report("vfio: error reading %s: %m", group_path);
>> -        return -errno;
>> +        error_setg_errno(&err, errno, "failed to read %s", group_path);
>> +        ret = -errno;
>> +        goto error;
>>      }
>>  
>>      trace_vfio_initfn(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(&err, "failed to get group %d", groupid);
>> +        ret = -ENOENT;
>> +        goto error;
>>      }
>>  
>>      QLIST_FOREACH(vbasedev_iter, &group->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(&err, "device is already attached");
>>              vfio_put_group(group);
>> -            return -EBUSY;
>> +            ret = -EBUSY;
>> +            goto error;
>>          }
>>      }
>>  
>>      ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
>>      if (ret) {
>> -        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
>> +        error_setg_errno(&err, -ret, "failed to get device");
>>          vfio_put_group(group);
>> -        return ret;
>> +        goto error;
>>      }
>>  
>>      ret = vfio_populate_device(vdev);
>> @@ -2567,8 +2572,8 @@ static int vfio_initfn(PCIDevice *pdev)
>>                  vdev->config_offset);
>>      if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
>>          ret = ret < 0 ? -errno : -EFAULT;
>> -        error_report("vfio: Failed to read device config space");
>> -        return ret;
>> +        error_setg_errno(&err, -ret, "failed to read device config space");
>> +        goto error;
>>      }
>>  
>>      /* vfio emulates a lot for us, but some bits need extra love */
>> @@ -2584,8 +2589,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>       */
>>      if (vdev->vendor_id != PCI_ANY_ID) {
>>          if (vdev->vendor_id >= 0xffff) {
>> -            error_report("vfio: Invalid PCI vendor ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI vendor ID provided");
>> +            ret = -EINVAL;
>> +            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);
>> @@ -2595,8 +2601,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->device_id != PCI_ANY_ID) {
>>          if (vdev->device_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI device ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI device ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
>>          trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id);
>> @@ -2606,8 +2613,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->sub_vendor_id != PCI_ANY_ID) {
>>          if (vdev->sub_vendor_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI subsystem vendor ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI subsystem vendor ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
>>                                 vdev->sub_vendor_id, ~0);
>> @@ -2617,8 +2625,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->sub_device_id != PCI_ANY_ID) {
>>          if (vdev->sub_device_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI subsystem device ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI subsystem device ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
>>          trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name,
>> @@ -2671,8 +2680,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>          struct vfio_region_info *opregion;
>>  
>>          if (vdev->pdev.qdev.hotplugged) {
>> -            error_report("Cannot support IGD OpRegion feature on hotplugged "
>> -                         "device %s", vdev->vbasedev.name);
>> +            error_setg(&err,
>> +                       "cannot support IGD OpRegion feature on hotplugged "
>> +                       "device");
>>              ret = -EINVAL;
>>              goto out_teardown;
>>          }
>> @@ -2681,16 +2691,15 @@ static int vfio_initfn(PCIDevice *pdev)
>>                          VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>>                          VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
>>          if (ret) {
>> -            error_report("Device %s does not support requested IGD OpRegion "
>> -                         "feature", vdev->vbasedev.name);
>> +            error_setg_errno(&err, -ret,
>> +                             "does not support requested IGD OpRegion feature");
>>              goto out_teardown;
>>          }
>>  
>>          ret = vfio_pci_igd_opregion_init(vdev, opregion);
>>          g_free(opregion);
>>          if (ret) {
>> -            error_report("Device %s IGD OpRegion initialization failed",
>> -                         vdev->vbasedev.name);
>> +            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
>>              goto out_teardown;
>>          }
>>      }
>> @@ -2726,6 +2735,10 @@ out_teardown:
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>      vfio_teardown_msi(vdev);
>>      vfio_bars_exit(vdev);
>> +error:
>> +    if (err) {
>> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>> +    }
>>      return ret;
>>  }
> 
> The value assigned to ret doesn't matter as long as it's negative, which
> makes me wonder why we bother with different values here.  Peeking
> ahead, I see you simplify this already as part of your conversion to
> realize().  Okay, that works for me.
At that stage it was a 1-1 conversion. Indeed this eventually disappears.

> 
>>  
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 94dfae3..fd19880 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -30,6 +30,9 @@
>>  #include <linux/vfio.h>
>>  #endif
>>  
>> +#define ERR_PREFIX "vfio error: %s: "
>> +#define WARN_PREFIX "vfio warning: %s: "
>> +
>>  /*#define DEBUG_VFIO*/
>>  #ifdef DEBUG_VFIO
>>  #define DPRINTF(fmt, ...) \
> 
> We already discussed error message prefixes and errno strings.  I got my
> taste, you got yours.  Since this is your code, yours wins :)
> 

Here is a sample output. Using qmp interactively you easily get to know
which device BDF produced the error. Also qemu prints the -device option
which lead to the error.

(QEMU) device_add driver=vfio-pci host=0000:01:10.0 bus=head.2 addr=0x14
{"error": {"class": "GenericError", "desc": "vfio error: 0000:01:10.0:
error opening /dev/vfio/6: No such file or directory"}}

qemu-system-aarch64: -device vfio-pci,host=0000:01:10.0: vfio error:
0000:01:10.0: error opening /dev/vfio/6: No such file or directory

However in case of more complex usage (qmp scripts?), might be helpful
to get the BDF directly in the error output?

Thanks

Eric

Patch
diff mbox

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a5a620a..ca0293d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2493,6 +2493,7 @@  static int vfio_initfn(PCIDevice *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;
@@ -2506,9 +2507,9 @@  static int vfio_initfn(PCIDevice *pdev)
     }
 
     if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
-        error_report("vfio: error: no such host device: %s",
-                     vdev->vbasedev.sysfsdev);
-        return -errno;
+        error_setg(&err, "no such host device");
+        ret = -errno;
+        goto error;
     }
 
     vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
@@ -2520,40 +2521,44 @@  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(&err, len < 0 ? errno : ENAMETOOLONG,
+                         "no iommu_group found");
+        ret = len < 0 ? -errno : -ENAMETOOLONG;
+        goto error;
     }
 
     group_path[len] = 0;
 
     group_name = basename(group_path);
     if (sscanf(group_name, "%d", &groupid) != 1) {
-        error_report("vfio: error reading %s: %m", group_path);
-        return -errno;
+        error_setg_errno(&err, errno, "failed to read %s", group_path);
+        ret = -errno;
+        goto error;
     }
 
     trace_vfio_initfn(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(&err, "failed to get group %d", groupid);
+        ret = -ENOENT;
+        goto error;
     }
 
     QLIST_FOREACH(vbasedev_iter, &group->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(&err, "device is already attached");
             vfio_put_group(group);
-            return -EBUSY;
+            ret = -EBUSY;
+            goto error;
         }
     }
 
     ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
     if (ret) {
-        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
+        error_setg_errno(&err, -ret, "failed to get device");
         vfio_put_group(group);
-        return ret;
+        goto error;
     }
 
     ret = vfio_populate_device(vdev);
@@ -2567,8 +2572,8 @@  static int vfio_initfn(PCIDevice *pdev)
                 vdev->config_offset);
     if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
         ret = ret < 0 ? -errno : -EFAULT;
-        error_report("vfio: Failed to read device config space");
-        return ret;
+        error_setg_errno(&err, -ret, "failed to read device config space");
+        goto error;
     }
 
     /* vfio emulates a lot for us, but some bits need extra love */
@@ -2584,8 +2589,9 @@  static int vfio_initfn(PCIDevice *pdev)
      */
     if (vdev->vendor_id != PCI_ANY_ID) {
         if (vdev->vendor_id >= 0xffff) {
-            error_report("vfio: Invalid PCI vendor ID provided");
-            return -EINVAL;
+            error_setg(&err, "invalid PCI vendor ID provided");
+            ret = -EINVAL;
+            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);
@@ -2595,8 +2601,9 @@  static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->device_id != PCI_ANY_ID) {
         if (vdev->device_id > 0xffff) {
-            error_report("vfio: Invalid PCI device ID provided");
-            return -EINVAL;
+            error_setg(&err, "invalid PCI device ID provided");
+            ret = -EINVAL;
+            goto error;
         }
         vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
         trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id);
@@ -2606,8 +2613,9 @@  static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->sub_vendor_id != PCI_ANY_ID) {
         if (vdev->sub_vendor_id > 0xffff) {
-            error_report("vfio: Invalid PCI subsystem vendor ID provided");
-            return -EINVAL;
+            error_setg(&err, "invalid PCI subsystem vendor ID provided");
+            ret = -EINVAL;
+            goto error;
         }
         vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
                                vdev->sub_vendor_id, ~0);
@@ -2617,8 +2625,9 @@  static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->sub_device_id != PCI_ANY_ID) {
         if (vdev->sub_device_id > 0xffff) {
-            error_report("vfio: Invalid PCI subsystem device ID provided");
-            return -EINVAL;
+            error_setg(&err, "invalid PCI subsystem device ID provided");
+            ret = -EINVAL;
+            goto error;
         }
         vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
         trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name,
@@ -2671,8 +2680,9 @@  static int vfio_initfn(PCIDevice *pdev)
         struct vfio_region_info *opregion;
 
         if (vdev->pdev.qdev.hotplugged) {
-            error_report("Cannot support IGD OpRegion feature on hotplugged "
-                         "device %s", vdev->vbasedev.name);
+            error_setg(&err,
+                       "cannot support IGD OpRegion feature on hotplugged "
+                       "device");
             ret = -EINVAL;
             goto out_teardown;
         }
@@ -2681,16 +2691,15 @@  static int vfio_initfn(PCIDevice *pdev)
                         VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
                         VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
         if (ret) {
-            error_report("Device %s does not support requested IGD OpRegion "
-                         "feature", vdev->vbasedev.name);
+            error_setg_errno(&err, -ret,
+                             "does not support requested IGD OpRegion feature");
             goto out_teardown;
         }
 
         ret = vfio_pci_igd_opregion_init(vdev, opregion);
         g_free(opregion);
         if (ret) {
-            error_report("Device %s IGD OpRegion initialization failed",
-                         vdev->vbasedev.name);
+            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
             goto out_teardown;
         }
     }
@@ -2726,6 +2735,10 @@  out_teardown:
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_teardown_msi(vdev);
     vfio_bars_exit(vdev);
+error:
+    if (err) {
+        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+    }
     return ret;
 }
 
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 94dfae3..fd19880 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -30,6 +30,9 @@ 
 #include <linux/vfio.h>
 #endif
 
+#define ERR_PREFIX "vfio error: %s: "
+#define WARN_PREFIX "vfio warning: %s: "
+
 /*#define DEBUG_VFIO*/
 #ifdef DEBUG_VFIO
 #define DPRINTF(fmt, ...) \