diff mbox

[v5,2/5] Add Error **errp for xen_host_pci_device_get()

Message ID 1452689507-8188-3-git-send-email-caoj.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Cao jin Jan. 13, 2016, 12:51 p.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 | 142 +++++++++++++++++++++----------------------
 hw/xen/xen-host-pci-device.h |   5 +-
 hw/xen/xen_pt.c              |  13 ++--
 3 files changed, 80 insertions(+), 80 deletions(-)

Comments

Eric Blake Jan. 14, 2016, 10:29 p.m. UTC | #1 
On 01/13/2016 05:51 AM, 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 | 142 +++++++++++++++++++++----------------------
>  hw/xen/xen-host-pci-device.h |   5 +-
>  hw/xen/xen_pt.c              |  13 ++--
>  3 files changed, 80 insertions(+), 80 deletions(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 351b61a..3e22de8 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -31,25 +31,20 @@
>  #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
>  #define IORESOURCE_MEM_64       0x00100000
>  
> -static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> -                                   const char *name, char *buf, ssize_t size)
> +static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> +                                    const char *name, char *buf, ssize_t size)

Changing xen_host_pci_sysfs_path() to return void, by assert()ing on
caller error, is not mentioned in the commit message; and if I were
doing the series, I probably would have done it as a separate commit.

>  /* 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)
>  {

>      buf[rc] = 0;
> -    rc = qemu_strtoul(buf, &endptr, base, &value);
> -    if (!rc) {
> -        *pvalue = value;
> +    rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue);

Ouch. Casting unsigned int * to unsigned long * and then dereferencing
it is bogus (you end up having qemu_strtoul() write beyond bounds on
platforms where long is larger than int).  You'll need to revert this
part of the patch, and stick with *pvalue = value (and possibly even add
a bounds check that value <= UINT_MAX).

Otherwise looks okay.
Cao jin Jan. 15, 2016, 3:11 a.m. UTC | #2 
On 01/15/2016 06:29 AM, Eric Blake wrote:
> On 01/13/2016 05:51 AM, 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 | 142 +++++++++++++++++++++----------------------
>>   hw/xen/xen-host-pci-device.h |   5 +-
>>   hw/xen/xen_pt.c              |  13 ++--
>>   3 files changed, 80 insertions(+), 80 deletions(-)
>>
>> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>> index 351b61a..3e22de8 100644
>> --- a/hw/xen/xen-host-pci-device.c
>> +++ b/hw/xen/xen-host-pci-device.c
>> @@ -31,25 +31,20 @@
>>   #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
>>   #define IORESOURCE_MEM_64       0x00100000
>>
>> -static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>> -                                   const char *name, char *buf, ssize_t size)
>> +static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>> +                                    const char *name, char *buf, ssize_t size)
>
> Changing xen_host_pci_sysfs_path() to return void, by assert()ing on
> caller error, is not mentioned in the commit message; and if I were
> doing the series, I probably would have done it as a separate commit.
>

Thanks for the suggestion, will split it out.

>>   /* 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)
>>   {
>
>>       buf[rc] = 0;
>> -    rc = qemu_strtoul(buf, &endptr, base, &value);
>> -    if (!rc) {
>> -        *pvalue = value;
>> +    rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue);
>
> Ouch. Casting unsigned int * to unsigned long * and then dereferencing
> it is bogus (you end up having qemu_strtoul() write beyond bounds on
> platforms where long is larger than int).

Yes, I considered this issue a little. Because the current condition is: 
the value it want to get won`t exceed 4 byte (vendor/device ID, etc). So 
I guess even if on x86_64(length of int != long), it won`t break things.
So, compared with following, which style do you prefer?

> You'll need to revert this
> part of the patch, and stick with *pvalue = value (and possibly even add
> a bounds check that value <= UINT_MAX).
>
> Otherwise looks okay.
>
Eric Blake Jan. 15, 2016, 4:41 p.m. UTC | #3 
On 01/14/2016 08:11 PM, Cao jin wrote:

>>>       buf[rc] = 0;
>>> -    rc = qemu_strtoul(buf, &endptr, base, &value);
>>> -    if (!rc) {
>>> -        *pvalue = value;
>>> +    rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue);
>>
>> Ouch. Casting unsigned int * to unsigned long * and then dereferencing
>> it is bogus (you end up having qemu_strtoul() write beyond bounds on
>> platforms where long is larger than int).
> 
> Yes, I considered this issue a little. Because the current condition is:
> the value it want to get won`t exceed 4 byte (vendor/device ID, etc). So
> I guess even if on x86_64(length of int != long), it won`t break things.
> So, compared with following, which style do you prefer?

Maybe:

rc = qemu_strtoul(buf, &endptr, base, &value);
if (rc) {
    assert(value < UINT_MAX);
    *pvalue = value;
} else {
    report error ...
}

And maybe some of it should even be done as part of the conversion to
qemu_strtoul() in 1/5.
Cao jin Jan. 17, 2016, 10:34 a.m. UTC | #4 
On 01/16/2016 12:41 AM, Eric Blake wrote:
> On 01/14/2016 08:11 PM, Cao jin wrote:
>
>>>>        buf[rc] = 0;
>>>> -    rc = qemu_strtoul(buf, &endptr, base, &value);
>>>> -    if (!rc) {
>>>> -        *pvalue = value;
>>>> +    rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue);
>>>
>>> Ouch. Casting unsigned int * to unsigned long * and then dereferencing
>>> it is bogus (you end up having qemu_strtoul() write beyond bounds on
>>> platforms where long is larger than int).
>>
>> Yes, I considered this issue a little. Because the current condition is:
>> the value it want to get won`t exceed 4 byte (vendor/device ID, etc). So
>> I guess even if on x86_64(length of int != long), it won`t break things.
>> So, compared with following, which style do you prefer?
>
> Maybe:
>
> rc = qemu_strtoul(buf, &endptr, base, &value);
> if (rc) {
>      assert(value < UINT_MAX);
>      *pvalue = value;
> } else {
>      report error ...
> }
>
> And maybe some of it should even be done as part of the conversion to
> qemu_strtoul() in 1/5.
>

Thanks for the example, will give v6 soon.
diff mbox

Patch

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 351b61a..3e22de8 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -31,25 +31,20 @@ 
 #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
 #define IORESOURCE_MEM_64       0x00100000
 
-static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
-                                   const char *name, char *buf, ssize_t size)
+static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
+                                    const char *name, char *buf, ssize_t size)
 {
     int rc;
 
     rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
                   d->domain, d->bus, d->dev, d->func, name);
-
-    if (rc >= size || rc < 0) {
-        /* The output is truncated, or some other error was encountered */
-        return -ENODEV;
-    }
-    return 0;
+    assert(rc >= 0 && rc < size);
 }
 
 
 /* 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];
@@ -58,25 +53,22 @@  static int xen_host_pci_get_resource(XenHostPCIDevice *d)
     char *endptr, *s;
     uint8_t type;
 
-    rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
-    if (rc) {
-        return rc;
-    }
+    xen_host_pci_sysfs_path(d, "resource", path, sizeof(path));
+
     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_file_open(errp, errno, path);
+        return;
     }
 
     do {
-        rc = read(fd, &buf, sizeof (buf) - 1);
+        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,65 +121,65 @@  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 too 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];
     int fd, rc;
-    unsigned long value;
     const char *endptr;
 
-    rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
-    if (rc) {
-        return rc;
-    }
+    xen_host_pci_sysfs_path(d, name, path, sizeof(path));
+
     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_file_open(errp, errno, path);
+        return;
     }
+
     do {
-        rc = read(fd, &buf, sizeof (buf) - 1);
+        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 = qemu_strtoul(buf, &endptr, base, &value);
-    if (!rc) {
-        *pvalue = value;
+    rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue);
+    if (rc) {
+        error_setg_errno(errp, -rc, "failed to parse value '%s'", buf);
     }
+
 out:
     close(fd);
-    return rc;
 }
 
-static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
-                                             const char *name,
-                                             unsigned int *pvalue)
+static inline void xen_host_pci_get_hex_value(XenHostPCIDevice *d,
+                                              const char *name,
+                                              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,
-                                             const char *name,
-                                             unsigned int *pvalue)
+static inline void xen_host_pci_get_dec_value(XenHostPCIDevice *d,
+                                              const char *name,
+                                              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)
@@ -195,26 +187,21 @@  static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
     char path[PATH_MAX];
     struct stat buf;
 
-    if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path))) {
-        return false;
-    }
+    xen_host_pci_sysfs_path(d, "physfn", path, sizeof(path));
+
     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;
-    }
+    xen_host_pci_sysfs_path(d, "config", path, sizeof(path));
+
     d->config_fd = open(path, O_RDWR);
-    if (d->config_fd < 0) {
-        return -errno;
+    if (d->config_fd == -1) {
+        error_setg_file_open(errp, errno, path);
     }
-    return 0;
 }
 
 static int xen_host_pci_config_read(XenHostPCIDevice *d,
@@ -336,11 +323,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 *err = NULL;
 
     d->config_fd = -1;
     d->domain = domain;
@@ -348,43 +336,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, &err);
+    if (err) {
         goto error;
     }
-    rc = xen_host_pci_get_resource(d);
-    if (rc) {
+
+    xen_host_pci_get_resource(d, &err);
+    if (err) {
         goto error;
     }
-    rc = xen_host_pci_get_hex_value(d, "vendor", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "vendor", &v, &err);
+    if (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, &err);
+    if (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, &err);
+    if (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, &err);
+    if (err) {
         goto error;
     }
     d->class_code = v;
+
     d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
 
-    return 0;
+    return;
+
 error:
+    error_propagate(errp, 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..6acf36e 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..53b5bca 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 *err = NULL;
 
     /* register real device */
     XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
@@ -774,11 +775,13 @@  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,
+                            &err);
+    if (err) {
+        error_append_hint(&err, "Failed to \"open\" the real pci device");
+        error_report_err(err);
         return -1;
     }