diff mbox

[v3,1/4] Add Error **errp for xen_host_pci_device_get()

Message ID 1452047951-17429-2-git-send-email-caoj.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Cao jin Jan. 6, 2016, 2:39 a.m. UTC
To catch the error msg. Also modify the caller

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
 hw/xen/xen-host-pci-device.h |   5 +-
 hw/xen/xen_pt.c              |  12 +++--
 3 files changed, 71 insertions(+), 52 deletions(-)

Comments

Eric Blake Jan. 6, 2016, 3:51 p.m. UTC | #1
On 01/05/2016 07:39 PM, Cao jin wrote:
> To catch the error msg. Also modify the caller
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
>  hw/xen/xen-host-pci-device.h |   5 +-
>  hw/xen/xen_pt.c              |  12 +++--
>  3 files changed, 71 insertions(+), 52 deletions(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 7d8a023..952cae0 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>  
>  /* This size should be enough to read the first 7 lines of a resource file */
>  #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
> -static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> +static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
>  {
>      int i, rc, fd;
>      char path[PATH_MAX];
> @@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>  
>      rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
>      if (rc) {
> -        return rc;
> +        error_setg_errno(errp, errno, "snprintf err");

Are you sure that errno is relevant?  And "snprintf err" doesn't seem to
be the correct message, as there is no snprintf in the line above.

> +        return;
>      }
> +
>      fd = open(path, O_RDONLY);
>      if (fd == -1) {
> -        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> -        return -errno;
> +        error_setg_errno(errp, errno, "open %s err", path);

Please use error_setg_file_open() for reporting open() failures.

> @@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>              d->rom.bus_flags = flags & IORESOURCE_BITS;
>          }
>      }
> +
>      if (i != PCI_NUM_REGIONS) {
>          /* Invalid format or input to short */
> -        rc = -ENODEV;
> +        error_setg(errp, "Invalid format or input to short: %s", buf);

s/to/too/ (and might as well fix the same typo in the comment while at it)


> @@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>  
>      rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
>      if (rc) {
> -        return rc;
> +        error_setg_errno(errp, errno, "snprintf err");
> +        return;
>      }
> +
>      fd = open(path, O_RDONLY);
>      if (fd == -1) {
> -        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> -        return -errno;
> +        error_setg_errno(errp, errno, "open %s err", path);

Same comments as above.

> +        return;
>      }
> +
>      do {
>          rc = read(fd, &buf, sizeof (buf) - 1);
>          if (rc < 0 && errno != EINTR) {
> -            rc = -errno;
> +            error_setg_errno(errp, errno, "read err");
>              goto out;
>          }
>      } while (rc < 0);
> +
>      buf[rc] = 0;
>      value = strtol(buf, &endptr, base);
>      if (endptr == buf || *endptr != '\n') {
> -        rc = -1;
> +        error_setg(errp, "format invalid: %s", buf);
>      } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> -        rc = -errno;
> +        error_setg_errno(errp, errno, "strtol err");

This is pre-existing invalid use of strtol (the value of errno is not
guaranteed to be ERANGE on overflow; and the only correct way to safely
check errno after strtol() is to first prime it to 0 prior to calling
strtol).  Better would be to use qemu_strtol() (preferably as a separate
patch), so that you don't even have to worry about using strtol()
incorrectly.

> +static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
>  {
>      char path[PATH_MAX];
>      int rc;
>  
>      rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));

May want to delete the space before ( while touching the code in this
vicinity.

>      if (rc) {
> -        return rc;
> +        error_setg_errno(errp, errno, "snprintf err");

Another suspect message.


> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> +                            uint8_t bus, uint8_t dev, uint8_t func,
> +                            Error **errp)
>  {
>      unsigned int v;
> -    int rc = 0;
> +    Error *local_err = NULL;

These days, naming the local variable 'err' is more common than 'local_err'.

> @@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
>                 s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
>                 s->dev.devfn);
>  
> -    rc = xen_host_pci_device_get(&s->real_device,
> -                                 s->hostaddr.domain, s->hostaddr.bus,
> -                                 s->hostaddr.slot, s->hostaddr.function);
> -    if (rc) {
> -        XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
> +    xen_host_pci_device_get(&s->real_device,
> +                            s->hostaddr.domain, s->hostaddr.bus,
> +                            s->hostaddr.slot, s->hostaddr.function,
> +                            &local_err);
> +    if (local_err) {
> +        XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");

Leaks local_err.
Cao jin Jan. 7, 2016, 3:13 a.m. UTC | #2
On 01/06/2016 11:51 PM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
>> To catch the error msg. Also modify the caller
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
>>   hw/xen/xen-host-pci-device.h |   5 +-
>>   hw/xen/xen_pt.c              |  12 +++--
>>   3 files changed, 71 insertions(+), 52 deletions(-)
>>
>> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>> index 7d8a023..952cae0 100644
>> --- a/hw/xen/xen-host-pci-device.c
>> +++ b/hw/xen/xen-host-pci-device.c
>> @@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>>
>>   /* This size should be enough to read the first 7 lines of a resource file */
>>   #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
>> -static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>> +static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
>>   {
>>       int i, rc, fd;
>>       char path[PATH_MAX];
>> @@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>>
>>       rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
>>       if (rc) {
>> -        return rc;
>> +        error_setg_errno(errp, errno, "snprintf err");
>
> Are you sure that errno is relevant?  And "snprintf err" doesn't seem to
> be the correct message, as there is no snprintf in the line above.
>

snprintf() is called in xen_host_pci_sysfs_path() above, and is the only 
possible error source, so I guess the errno is relevant?

Or, replace the error_setg_errno() to assert(0)? because if snprintf 
goes wrong, user seems can do nothing.

>> +        return;
>>       }
>> +
>>       fd = open(path, O_RDONLY);
>>       if (fd == -1) {
>> -        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
>> -        return -errno;
>> +        error_setg_errno(errp, errno, "open %s err", path);
>
> Please use error_setg_file_open() for reporting open() failures.
>
>> @@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>>               d->rom.bus_flags = flags & IORESOURCE_BITS;
>>           }
>>       }
>> +
>>       if (i != PCI_NUM_REGIONS) {
>>           /* Invalid format or input to short */
>> -        rc = -ENODEV;
>> +        error_setg(errp, "Invalid format or input to short: %s", buf);
>
> s/to/too/ (and might as well fix the same typo in the comment while at it)
>
>
>> @@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>>
>>       rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
>>       if (rc) {
>> -        return rc;
>> +        error_setg_errno(errp, errno, "snprintf err");
>> +        return;
>>       }
>> +
>>       fd = open(path, O_RDONLY);
>>       if (fd == -1) {
>> -        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
>> -        return -errno;
>> +        error_setg_errno(errp, errno, "open %s err", path);
>
> Same comments as above.
>
>> +        return;
>>       }
>> +
>>       do {
>>           rc = read(fd, &buf, sizeof (buf) - 1);
>>           if (rc < 0 && errno != EINTR) {
>> -            rc = -errno;
>> +            error_setg_errno(errp, errno, "read err");
>>               goto out;
>>           }
>>       } while (rc < 0);
>> +
>>       buf[rc] = 0;
>>       value = strtol(buf, &endptr, base);
>>       if (endptr == buf || *endptr != '\n') {
>> -        rc = -1;
>> +        error_setg(errp, "format invalid: %s", buf);
>>       } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
>> -        rc = -errno;
>> +        error_setg_errno(errp, errno, "strtol err");
>
> This is pre-existing invalid use of strtol (the value of errno is not
> guaranteed to be ERANGE on overflow; and the only correct way to safely
> check errno after strtol() is to first prime it to 0 prior to calling
> strtol).  Better would be to use qemu_strtol() (preferably as a separate
> patch), so that you don't even have to worry about using strtol()
> incorrectly.
>

Ok, will split it into a separate patch

>> +static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
>>   {
>>       char path[PATH_MAX];
>>       int rc;
>>
>>       rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
>
> May want to delete the space before ( while touching the code in this
> vicinity.
>
>>       if (rc) {
>> -        return rc;
>> +        error_setg_errno(errp, errno, "snprintf err");
>
> Another suspect message.
>
>
>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> +                            uint8_t bus, uint8_t dev, uint8_t func,
>> +                            Error **errp)
>>   {
>>       unsigned int v;
>> -    int rc = 0;
>> +    Error *local_err = NULL;
>
> These days, naming the local variable 'err' is more common than 'local_err'.
>

agree. I guess maybe "local_err" is a better mnemonic for newbie. and 
when I am gradually familiar with error report, I also prefer "err". 
Actually I considered to change this name, I just don`t want to bother 
the reviewer to review it again, especially when the patch got a 
Review-by and new version just changes some names. Will fix it.

>> @@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
>>                  s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
>>                  s->dev.devfn);
>>
>> -    rc = xen_host_pci_device_get(&s->real_device,
>> -                                 s->hostaddr.domain, s->hostaddr.bus,
>> -                                 s->hostaddr.slot, s->hostaddr.function);
>> -    if (rc) {
>> -        XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
>> +    xen_host_pci_device_get(&s->real_device,
>> +                            s->hostaddr.domain, s->hostaddr.bus,
>> +                            s->hostaddr.slot, s->hostaddr.function,
>> +                            &local_err);
>> +    if (local_err) {
>> +        XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
>
> Leaks local_err.
>

Yes, but this will be fixed with error_propagate when patch "4/4 convert 
to realize" comes, so is it ok to let it be here?

I originally want to make these patch independent from each other with 
the most necessary modification.
Eric Blake Jan. 7, 2016, 4:47 p.m. UTC | #3
On 01/06/2016 08:13 PM, Cao jin wrote:
> 
> 
> On 01/06/2016 11:51 PM, Eric Blake wrote:
>> On 01/05/2016 07:39 PM, Cao jin wrote:
>>> To catch the error msg. Also modify the caller
>>>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>> ---

>>> xen_host_pci_get_resource(XenHostPCIDevice *d)
>>>
>>>       rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
>>>       if (rc) {
>>> -        return rc;
>>> +        error_setg_errno(errp, errno, "snprintf err");
>>
>> Are you sure that errno is relevant?  And "snprintf err" doesn't seem to
>> be the correct message, as there is no snprintf in the line above.
>>
> 
> snprintf() is called in xen_host_pci_sysfs_path() above, and is the only
> possible error source, so I guess the errno is relevant?

Then maybe it's better to pass Error **errp through
xen_host_pci_sysfs_path(), so that the error message is closer to the
real error.  Especially since you have multiple callers, all identically
affected (each caller shouldn't have to repeat the same common
error-handling logic, if you can push it down lower in the callstack).

For that matter, the only failure of xen_host_pci_sysfs_path is to
return -ENODEV, but it does NOT set 'errno = ENODEV' on that error path,
so you most likely ARE printing the wrong errno value.

Another option is to at least reword the message to make sense locally,
as in:

if (xen_host_pci_sysfs_path(...)) {
    error_setg(errp, "failed to determine device path for %s", name);
    return;
}

> 
> Or, replace the error_setg_errno() to assert(0)? because if snprintf
> goes wrong, user seems can do nothing.

If you want to assert, then do that in xen_host_pci_sysfs_path(), not in
all callers; at which point xen_host_pci_sysfs_path() should be fixed to
return void.


>>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>>> +                            uint8_t bus, uint8_t dev, uint8_t func,
>>> +                            Error **errp)
>>>   {
>>>       unsigned int v;
>>> -    int rc = 0;
>>> +    Error *local_err = NULL;
>>
>> These days, naming the local variable 'err' is more common than
>> 'local_err'.
>>
> 
> agree. I guess maybe "local_err" is a better mnemonic for newbie. and
> when I am gradually familiar with error report, I also prefer "err".
> Actually I considered to change this name, I just don`t want to bother
> the reviewer to review it again, especially when the patch got a
> Review-by and new version just changes some names. Will fix it.

We don't mind re-reviewing a patch, but it does help if your cover
letter and/or text after the --- divider explains how v4 differs from
v3, to help us focus on the changes.

>>> +    xen_host_pci_device_get(&s->real_device,
>>> +                            s->hostaddr.domain, s->hostaddr.bus,
>>> +                            s->hostaddr.slot, s->hostaddr.function,
>>> +                            &local_err);
>>> +    if (local_err) {
>>> +        XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
>>
>> Leaks local_err.
>>
> 
> Yes, but this will be fixed with error_propagate when patch "4/4 convert
> to realize" comes, so is it ok to let it be here?

No. Every patch should be stand-alone correct, or else strongly document
why it is taking a shortcut that a later patch will clean up.  At a bare
minimum, if you don't fix the leak here, you should have a FIXME comment
both here and in the commit message; but it's probably easier to just
avoid the leak in the first place.

> 
> I originally want to make these patch independent from each other with
> the most necessary modification.

Splitting patches can be an art form.  But your comment about the patch
being independent _is_ the reason why the patch must not leak - if a
backporter uses this patch but not the rest of the series, they should
not be introducing a leak in their backport.
diff mbox

Patch

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 7d8a023..952cae0 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -49,7 +49,7 @@  static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
 
 /* This size should be enough to read the first 7 lines of a resource file */
 #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
-static int xen_host_pci_get_resource(XenHostPCIDevice *d)
+static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
 {
     int i, rc, fd;
     char path[PATH_MAX];
@@ -60,23 +60,24 @@  static int xen_host_pci_get_resource(XenHostPCIDevice *d)
 
     rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
     if (rc) {
-        return rc;
+        error_setg_errno(errp, errno, "snprintf err");
+        return;
     }
+
     fd = open(path, O_RDONLY);
     if (fd == -1) {
-        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
-        return -errno;
+        error_setg_errno(errp, errno, "open %s err", path);
+        return;
     }
 
     do {
         rc = read(fd, &buf, sizeof (buf) - 1);
         if (rc < 0 && errno != EINTR) {
-            rc = -errno;
+            error_setg_errno(errp, errno, "read err");
             goto out;
         }
     } while (rc < 0);
     buf[rc] = 0;
-    rc = 0;
 
     s = buf;
     for (i = 0; i < PCI_NUM_REGIONS; i++) {
@@ -129,20 +130,20 @@  static int xen_host_pci_get_resource(XenHostPCIDevice *d)
             d->rom.bus_flags = flags & IORESOURCE_BITS;
         }
     }
+
     if (i != PCI_NUM_REGIONS) {
         /* Invalid format or input to short */
-        rc = -ENODEV;
+        error_setg(errp, "Invalid format or input to short: %s", buf);
     }
 
 out:
     close(fd);
-    return rc;
 }
 
 /* This size should be enough to read a long from a file */
 #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
-static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
-                                  unsigned int *pvalue, int base)
+static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
+                                  unsigned int *pvalue, int base, Error **errp)
 {
     char path[PATH_MAX];
     char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
@@ -152,47 +153,52 @@  static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
 
     rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
     if (rc) {
-        return rc;
+        error_setg_errno(errp, errno, "snprintf err");
+        return;
     }
+
     fd = open(path, O_RDONLY);
     if (fd == -1) {
-        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
-        return -errno;
+        error_setg_errno(errp, errno, "open %s err", path);
+        return;
     }
+
     do {
         rc = read(fd, &buf, sizeof (buf) - 1);
         if (rc < 0 && errno != EINTR) {
-            rc = -errno;
+            error_setg_errno(errp, errno, "read err");
             goto out;
         }
     } while (rc < 0);
+
     buf[rc] = 0;
     value = strtol(buf, &endptr, base);
     if (endptr == buf || *endptr != '\n') {
-        rc = -1;
+        error_setg(errp, "format invalid: %s", buf);
     } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
-        rc = -errno;
+        error_setg_errno(errp, errno, "strtol err");
     } else {
-        rc = 0;
         *pvalue = value;
     }
+
 out:
     close(fd);
-    return rc;
 }
 
-static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
+static inline void xen_host_pci_get_hex_value(XenHostPCIDevice *d,
                                              const char *name,
-                                             unsigned int *pvalue)
+                                             unsigned int *pvalue,
+                                             Error **errp)
 {
-    return xen_host_pci_get_value(d, name, pvalue, 16);
+    xen_host_pci_get_value(d, name, pvalue, 16, errp);
 }
 
-static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
+static inline void xen_host_pci_get_dec_value(XenHostPCIDevice *d,
                                              const char *name,
-                                             unsigned int *pvalue)
+                                             unsigned int *pvalue,
+                                             Error **errp)
 {
-    return xen_host_pci_get_value(d, name, pvalue, 10);
+    xen_host_pci_get_value(d, name, pvalue, 10, errp);
 }
 
 static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
@@ -206,20 +212,21 @@  static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
     return !stat(path, &buf);
 }
 
-static int xen_host_pci_config_open(XenHostPCIDevice *d)
+static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
 {
     char path[PATH_MAX];
     int rc;
 
     rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
     if (rc) {
-        return rc;
+        error_setg_errno(errp, errno, "snprintf err");
+        return;
     }
+
     d->config_fd = open(path, O_RDWR);
     if (d->config_fd < 0) {
-        return -errno;
+        error_setg_errno(errp, errno, "open %s err", path);
     }
-    return 0;
 }
 
 static int xen_host_pci_config_read(XenHostPCIDevice *d,
@@ -341,11 +348,12 @@  int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
     return -1;
 }
 
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
-                            uint8_t bus, uint8_t dev, uint8_t func)
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+                            uint8_t bus, uint8_t dev, uint8_t func,
+                            Error **errp)
 {
     unsigned int v;
-    int rc = 0;
+    Error *local_err = NULL;
 
     d->config_fd = -1;
     d->domain = domain;
@@ -353,43 +361,51 @@  int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
     d->dev = dev;
     d->func = func;
 
-    rc = xen_host_pci_config_open(d);
-    if (rc) {
+    xen_host_pci_config_open(d, &local_err);
+    if (local_err) {
         goto error;
     }
-    rc = xen_host_pci_get_resource(d);
-    if (rc) {
+
+    xen_host_pci_get_resource(d, &local_err);
+    if (local_err) {
         goto error;
     }
-    rc = xen_host_pci_get_hex_value(d, "vendor", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "vendor", &v, &local_err);
+    if (local_err) {
         goto error;
     }
     d->vendor_id = v;
-    rc = xen_host_pci_get_hex_value(d, "device", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "device", &v, &local_err);
+    if (local_err) {
         goto error;
     }
     d->device_id = v;
-    rc = xen_host_pci_get_dec_value(d, "irq", &v);
-    if (rc) {
+
+    xen_host_pci_get_dec_value(d, "irq", &v, &local_err);
+    if (local_err) {
         goto error;
     }
     d->irq = v;
-    rc = xen_host_pci_get_hex_value(d, "class", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "class", &v, &local_err);
+    if (local_err) {
         goto error;
     }
     d->class_code = v;
+
     d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
 
-    return 0;
+    return;
+
 error:
+    error_propagate(errp, local_err);
+
     if (d->config_fd >= 0) {
         close(d->config_fd);
         d->config_fd = -1;
     }
-    return rc;
 }
 
 bool xen_host_pci_device_closed(XenHostPCIDevice *d)
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index 3d44e04..42b9138 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -36,8 +36,9 @@  typedef struct XenHostPCIDevice {
     int config_fd;
 } XenHostPCIDevice;
 
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
-                            uint8_t bus, uint8_t dev, uint8_t func);
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+                            uint8_t bus, uint8_t dev, uint8_t func,
+                            Error **errp);
 void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
 bool xen_host_pci_device_closed(XenHostPCIDevice *d);
 
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index aa96288..1bd4109 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -767,6 +767,7 @@  static int xen_pt_initfn(PCIDevice *d)
     uint8_t machine_irq = 0, scratch;
     uint16_t cmd = 0;
     int pirq = XEN_PT_UNASSIGNED_PIRQ;
+    Error *local_err = NULL;
 
     /* register real device */
     XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
@@ -774,11 +775,12 @@  static int xen_pt_initfn(PCIDevice *d)
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
                s->dev.devfn);
 
-    rc = xen_host_pci_device_get(&s->real_device,
-                                 s->hostaddr.domain, s->hostaddr.bus,
-                                 s->hostaddr.slot, s->hostaddr.function);
-    if (rc) {
-        XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
+    xen_host_pci_device_get(&s->real_device,
+                            s->hostaddr.domain, s->hostaddr.bus,
+                            s->hostaddr.slot, s->hostaddr.function,
+                            &local_err);
+    if (local_err) {
+        XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
         return -1;
     }