[Qemu-devel] [PATCH v4 0/5] Xen PCI passthru: Convert to realize()

[Qemu-devel] [PATCH v4 0/5] Xen PCI passthru: Convert to realize()

[Cao jin]

v4 changelog:
1. patch 1/5 is new, according to Eric`s comment, using qemu_strtoul().
2. change xen_host_pci_sysfs_path() to void, use assert inside.
3. fix all Error object memory leak risk, via error_report_err()
4. change 'local_err' to 'err'.
5. fix to all the format-related issue.

Cao jin (5):
  Use qemu_strtoul instead of strtol
  Add Error **errp for xen_host_pci_device_get()
  Add Error **errp for xen_pt_setup_vga()
  Add Error **errp for xen_pt_config_init()
  Xen PCI passthru: convert to realize()

 hw/xen/xen-host-pci-device.c | 145 ++++++++++++++++++++++---------------------
 hw/xen/xen-host-pci-device.h |   5 +-
 hw/xen/xen_pt.c              |  77 +++++++++++++----------
 hw/xen/xen_pt.h              |   5 +-
 hw/xen/xen_pt_config_init.c  |  51 ++++++++-------
 hw/xen/xen_pt_graphics.c     |  11 ++--
 6 files changed, 156 insertions(+), 138 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v4 1/5] Use qemu_strtoul instead of strtol

[Cao jin]

strtol() don`t guarantee errno to be ERANGE on overflow.
This wrapper returns either -EINVAL or the errno set by strtol()
function (e.g -ERANGE).

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen-host-pci-device.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 7d8a023..351b61a 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -148,7 +148,7 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
     char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
     int fd, rc;
     unsigned long value;
-    char *endptr;
+    const char *endptr;
 
     rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
     if (rc) {
@@ -167,13 +167,8 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
         }
     } while (rc < 0);
     buf[rc] = 0;
-    value = strtol(buf, &endptr, base);
-    if (endptr == buf || *endptr != '\n') {
-        rc = -1;
-    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
-        rc = -errno;
-    } else {
-        rc = 0;
+    rc = qemu_strtoul(buf, &endptr, base, &value);
+    if (!rc) {
         *pvalue = value;
     }
 out:
-- 
2.1.0

Re: [Qemu-devel] [PATCH v4 1/5] Use qemu_strtoul instead of strtol

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

On 01/08/2016 01:37 AM, Cao jin wrote:
> strtol() don`t guarantee errno to be ERANGE on overflow.

I stand slightly corrected: C99 requires ERANGE on overflow, but not
EINVAL; it is POSIX that adds EINVAL, but does not properly require it.
 At any rate, my main point was that errno is not always properly set by
all strtol implementations, and furthermore that you can't rely on it
being set to a sane value if you didn't pre-set it to 0.

> This wrapper returns either -EINVAL or the errno set by strtol()
> function (e.g -ERANGE).

The subject line doesn't start with a topic.  Maybe a better commit
message would be:

xen: Use qemu_strtoul instead of strtol

No need to roll our own (with slightly incorrect handling of errno),
when we can use the common version.

> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen-host-pci-device.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)

>      buf[rc] = 0;
> -    value = strtol(buf, &endptr, base);
> -    if (endptr == buf || *endptr != '\n') {
> -        rc = -1;
> -    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> -        rc = -errno;
> -    } else {
> -        rc = 0;
> +    rc = qemu_strtoul(buf, &endptr, base, &value);

Why did you switch from strtol() to qemu_strtoul()?  Was signed parsing
incorrect, and unsigned parsing a bug fix?  If so, please mention it in
the commit message as intentional.  Otherwise, use qemu_strtol() (and
adjust the commit message accordingly).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 1/5] Use qemu_strtoul instead of strtol

[Cao jin]

On 01/09/2016 01:35 AM, Eric Blake wrote:
> On 01/08/2016 01:37 AM, Cao jin wrote:
>> strtol() don`t guarantee errno to be ERANGE on overflow.
>
> I stand slightly corrected: C99 requires ERANGE on overflow, but not
> EINVAL; it is POSIX that adds EINVAL, but does not properly require it.
>   At any rate, my main point was that errno is not always properly set by
> all strtol implementations, and furthermore that you can't rely on it
> being set to a sane value if you didn't pre-set it to 0.
>

Got you:)

>> This wrapper returns either -EINVAL or the errno set by strtol()
>> function (e.g -ERANGE).
>
> The subject line doesn't start with a topic.  Maybe a better commit
> message would be:
>
> xen: Use qemu_strtoul instead of strtol
>
> No need to roll our own (with slightly incorrect handling of errno),
> when we can use the common version.
>

ok, looks good.

>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/xen/xen-host-pci-device.c | 11 +++--------
>>   1 file changed, 3 insertions(+), 8 deletions(-)
>
>>       buf[rc] = 0;
>> -    value = strtol(buf, &endptr, base);
>> -    if (endptr == buf || *endptr != '\n') {
>> -        rc = -1;
>> -    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
>> -        rc = -errno;
>> -    } else {
>> -        rc = 0;
>> +    rc = qemu_strtoul(buf, &endptr, base, &value);
>
> Why did you switch from strtol() to qemu_strtoul()?  Was signed parsing
> incorrect, and unsigned parsing a bug fix?  If so, please mention it in
> the commit message as intentional.  Otherwise, use qemu_strtol() (and
> adjust the commit message accordingly).
>

Yes, I should mention it, sorry for forgetting to explain it. Because 
what it want to read from host are values in pci device config space, 
which are non-negative, like "vendor ID", "device ID", "class code", 
etc. so unsigned parsing seems more suitable in this case. we can tell 
it also from the local variable "unsigned long value;"

-- 
Yours Sincerely,

Cao jin

[Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

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 | 134 ++++++++++++++++++++++---------------------
 hw/xen/xen-host-pci-device.h |   5 +-
 hw/xen/xen_pt.c              |  13 +++--
 3 files changed, 81 insertions(+), 71 deletions(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 351b61a..b8ec016 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -31,8 +31,8 @@
 #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;
 
@@ -40,16 +40,16 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
                   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;
+        /* The output is truncated, or some other error was encountered.
+         * Assert here since user can do nothing in case of failure */
+        assert(0);
     }
-    return 0;
 }
 
 
 /* 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 +58,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,20 +126,19 @@ 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];
@@ -150,44 +146,50 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
     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;
+    } else if (rc == -EINVAL) {
+        error_setg(errp, "strtoul: Invalid argument");
+    } else {
+        error_setg_errno(errp, errno, "strtoul err");
     }
+
 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)
@@ -195,26 +197,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 +333,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 +346,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..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..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;
     }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4844 bytes --]

On 01/08/2016 01:37 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 | 134 ++++++++++++++++++++++---------------------
>  hw/xen/xen-host-pci-device.h |   5 +-
>  hw/xen/xen_pt.c              |  13 +++--
>  3 files changed, 81 insertions(+), 71 deletions(-)
> 

> @@ -40,16 +40,16 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>                    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;
> +        /* The output is truncated, or some other error was encountered.
> +         * Assert here since user can do nothing in case of failure */
> +        assert(0);
>      }

Might be shorter to drop the 'if' block, and just write:

assert(rc >= 0 && rc < size);

where you then don't need a comment, because the body of the assert() is
then more specific on the caller's responsibility for passing in a
decent size argument.


>      buf[rc] = 0;
>      rc = qemu_strtoul(buf, &endptr, base, &value);

Do you still need a local 'value' variable, or can you just reuse pvalue
here?

>      if (!rc) {
>          *pvalue = value;
> +    } else if (rc == -EINVAL) {
> +        error_setg(errp, "strtoul: Invalid argument");
> +    } else {
> +        error_setg_errno(errp, errno, "strtoul err");

Still not quite right - you are not guaranteed that 'errno' is sane
after qemu_strtoul(), only that -rc is sane.  And feels repetitive.
Better might be:

rc = qemu_strtoul(buf, &endptr, base, pvalue);
if (rc) {
    error_setg_errno(errp, -rc, "failed to parse value '%s'", buf);
}

> -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)

Indentation is off.

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

and again.


> -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)

and again.


> +++ 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);

and again

> @@ -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");

Markus may have an opinion on whether his new error_prepend code is a
better fit (error_append_hint lists _after_ the original failure, but it
sounds like you want "failed to open the real pci device: low level
details").

But looks like you're getting closer.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

On 01/09/2016 06:50 AM, Eric Blake wrote:
> On 01/08/2016 01:37 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 | 134 ++++++++++++++++++++++---------------------
>>   hw/xen/xen-host-pci-device.h |   5 +-
>>   hw/xen/xen_pt.c              |  13 +++--
>>   3 files changed, 81 insertions(+), 71 deletions(-)
>>
>
>> @@ -40,16 +40,16 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>>                     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;
>> +        /* The output is truncated, or some other error was encountered.
>> +         * Assert here since user can do nothing in case of failure */
>> +        assert(0);
>>       }
>
> Might be shorter to drop the 'if' block, and just write:
>
> assert(rc >= 0 && rc < size);
>
> where you then don't need a comment, because the body of the assert() is
> then more specific on the caller's responsibility for passing in a
> decent size argument.
>

Yes, seems better

>
>>       buf[rc] = 0;
>>       rc = qemu_strtoul(buf, &endptr, base, &value);
>
> Do you still need a local 'value' variable, or can you just reuse pvalue
> here?
>
>>       if (!rc) {
>>           *pvalue = value;
>> +    } else if (rc == -EINVAL) {
>> +        error_setg(errp, "strtoul: Invalid argument");
>> +    } else {
>> +        error_setg_errno(errp, errno, "strtoul err");
>
> Still not quite right - you are not guaranteed that 'errno' is sane
> after qemu_strtoul(), only that -rc is sane.  And feels repetitive.
> Better might be:
>
> rc = qemu_strtoul(buf, &endptr, base, pvalue);
> if (rc) {
>      error_setg_errno(errp, -rc, "failed to parse value '%s'", buf);
> }
>

yes, shorter and better.

But you said before: the only correct way to safely check errno after 
strtol() is to first prime it to 0 prior to calling strtol. Seems 
qemu_strtoul() did it as you said(first prime it to 0), so the errno is 
sane? Then I guess my patch can achieve the same result as yours? 
because return value of qemu_strtoul() is as following:
   success: 0
   failure: -EINVAL or -errno

[...]
>> -    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");
>
> Markus may have an opinion on whether his new error_prepend code is a
> better fit (error_append_hint lists _after_ the original failure, but it
> sounds like you want "failed to open the real pci device: low level
> details").
>
> But looks like you're getting closer.
>

greped the code, seems error_prepend doesn`t exist in upstream? So I 
guess maybe we can use append hint for now?

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

On 01/09/2016 06:50 AM, Eric Blake wrote:
> On 01/08/2016 01:37 AM, Cao jin wrote:

>
>
>>       buf[rc] = 0;
>>       rc = qemu_strtoul(buf, &endptr, base, &value);
>
> Do you still need a local 'value' variable, or can you just reuse pvalue
> here?
>

I guess so, or else it won`t compile, because, *pvalue* is int*, *value* is
unsigned long*, and qemu_strtoul() require a unsigned long* for the last 
param.
And I guess that is why author use a local 'value'. I just want to make 
the patch
small for the reviewer, so I reuse most of original code.

but yes, maybe it can be:
rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue);

-- 
Yours Sincerely,

Cao jin

[Qemu-devel] [PATCH v4 3/5] Add Error **errp for xen_pt_setup_vga()

[Cao jin]

To catch the error msg. Also modify the caller

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen_pt.c          |  7 +++++--
 hw/xen/xen_pt.h          |  3 ++-
 hw/xen/xen_pt_graphics.c | 11 ++++++-----
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 53b5bca..07bfcec 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -808,8 +808,11 @@ static int xen_pt_initfn(PCIDevice *d)
             return -1;
         }
 
-        if (xen_pt_setup_vga(s, &s->real_device) < 0) {
-            XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+        xen_pt_setup_vga(s, &s->real_device, &err);
+        if (err) {
+            error_append_hint(&err, "Setup VGA BIOS of passthrough"
+                    " GFX failed");
+            error_report_err(err);
             xen_host_pci_device_put(&s->real_device);
             return -1;
         }
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index c545280..dc74d3e 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -328,5 +328,6 @@ static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
 }
 int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
 int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);
-int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev);
+void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
+                     Error **errp);
 #endif /* !XEN_PT_H */
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index df6069b..25b6126 100644
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -161,7 +161,8 @@ struct pci_data {
     uint16_t reserved;
 } __attribute__((packed));
 
-int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
+void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
+                     Error **errp)
 {
     unsigned char *bios = NULL;
     struct rom_header *rom;
@@ -172,13 +173,14 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
     struct pci_data *pd = NULL;
 
     if (!is_igd_vga_passthrough(dev)) {
-        return -1;
+        error_setg(errp, "Need to enable igd-passthrough");
+        return;
     }
 
     bios = get_vgabios(s, &bios_size, dev);
     if (!bios) {
-        XEN_PT_ERR(&s->dev, "VGA: Can't getting VBIOS!\n");
-        return -1;
+        error_setg(errp, "VGA: Can't getting VBIOS");
+        return;
     }
 
     /* Currently we fixed this address as a primary. */
@@ -203,7 +205,6 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
 
     /* Currently we fixed this address as a primary for legacy BIOS. */
     cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
-    return 0;
 }
 
 uint32_t igd_read_opregion(XenPCIPassthroughState *s)
-- 
2.1.0

[Qemu-devel] [PATCH v4 4/5] Add Error **errp for xen_pt_config_init()

[Cao jin]

To catch the error msg. Also modify the caller

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen_pt.c             |  8 ++++---
 hw/xen/xen_pt.h             |  2 +-
 hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++---------------------
 3 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 07bfcec..9eef3df 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -825,9 +825,11 @@ static int xen_pt_initfn(PCIDevice *d)
     xen_pt_register_regions(s, &cmd);
 
     /* reinitialize each config register to be emulated */
-    rc = xen_pt_config_init(s);
-    if (rc) {
-        XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
+    xen_pt_config_init(s, &err);
+    if (err) {
+        error_append_hint(&err, "PCI Config space initialisation failed");
+        error_report_err(err);
+        rc = -1;
         goto err_out;
     }
 
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index dc74d3e..466525f 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -228,7 +228,7 @@ struct XenPCIPassthroughState {
     bool listener_set;
 };
 
-int xen_pt_config_init(XenPCIPassthroughState *s);
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp);
 void xen_pt_config_delete(XenPCIPassthroughState *s);
 XenPTRegGroup *xen_pt_find_reg_grp(XenPCIPassthroughState *s, uint32_t address);
 XenPTReg *xen_pt_find_reg(XenPTRegGroup *reg_grp, uint32_t address);
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 3d8686d..128ccba 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1899,8 +1899,9 @@ static uint8_t find_cap_offset(XenPCIPassthroughState *s, uint8_t cap)
     return 0;
 }
 
-static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
-                                  XenPTRegGroup *reg_grp, XenPTRegInfo *reg)
+static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
+                                   XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
+                                   Error **errp)
 {
     XenPTReg *reg_entry;
     uint32_t data = 0;
@@ -1919,12 +1920,13 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
                        reg_grp->base_offset + reg->offset, &data);
         if (rc < 0) {
             g_free(reg_entry);
-            return rc;
+            error_setg(errp, "Init emulate register fail");
+            return;
         }
         if (data == XEN_PT_INVALID_REG) {
             /* free unused BAR register entry */
             g_free(reg_entry);
-            return 0;
+            return;
         }
         /* Sync up the data to dev.config */
         offset = reg_grp->base_offset + reg->offset;
@@ -1942,7 +1944,8 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
         if (rc) {
             /* Serious issues when we cannot read the host values! */
             g_free(reg_entry);
-            return rc;
+            error_setg(errp, "Cannot read host values");
+            return;
         }
         /* Set bits in emu_mask are the ones we emulate. The dev.config shall
          * contain the emulated view of the guest - therefore we flip the mask
@@ -1967,10 +1970,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
             val = data;
 
         if (val & ~size_mask) {
-            XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
-                       offset, val, reg->size);
+            error_setg(errp, "Offset 0x%04x:0x%04x expands past"
+                    " register size (%d)", offset, val, reg->size);
             g_free(reg_entry);
-            return -ENXIO;
+            return;
         }
         /* This could be just pci_set_long as we don't modify the bits
          * past reg->size, but in case this routine is run in parallel or the
@@ -1990,13 +1993,12 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
     }
     /* list add register entry */
     QLIST_INSERT_HEAD(&reg_grp->reg_tbl_list, reg_entry, entries);
-
-    return 0;
 }
 
-int xen_pt_config_init(XenPCIPassthroughState *s)
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
 {
     int i, rc;
+    Error *err = NULL;
 
     QLIST_INIT(&s->reg_grps);
 
@@ -2039,11 +2041,12 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
                                                   reg_grp_offset,
                                                   &reg_grp_entry->size);
             if (rc < 0) {
-                XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
-                           i, ARRAY_SIZE(xen_pt_emu_reg_grps),
+                error_setg(&err, "Failed to initialize %d/%zu, type = 0x%x,"
+                           " rc: %d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
                            xen_pt_emu_reg_grps[i].grp_type, rc);
+                error_propagate(errp, err);
                 xen_pt_config_delete(s);
-                return rc;
+                return;
             }
         }
 
@@ -2051,24 +2054,24 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
             if (xen_pt_emu_reg_grps[i].emu_regs) {
                 int j = 0;
                 XenPTRegInfo *regs = xen_pt_emu_reg_grps[i].emu_regs;
+
                 /* initialize capability register */
                 for (j = 0; regs->size != 0; j++, regs++) {
-                    /* initialize capability register */
-                    rc = xen_pt_config_reg_init(s, reg_grp_entry, regs);
-                    if (rc < 0) {
-                        XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
-                                   j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
-                                   regs->offset, xen_pt_emu_reg_grps[i].grp_type,
-                                   i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
+                    xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
+                    if (err) {
+                        error_append_hint(&err, "Failed to initialize %d/%zu"
+                                " reg 0x%x in grp_type = 0x%x (%d/%zu)",
+                                j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
+                                regs->offset, xen_pt_emu_reg_grps[i].grp_type,
+                                i, ARRAY_SIZE(xen_pt_emu_reg_grps));
+                        error_propagate(errp, err);
                         xen_pt_config_delete(s);
-                        return rc;
+                        return;
                     }
                 }
             }
         }
     }
-
-    return 0;
 }
 
 /* delete all emulate register */
-- 
2.1.0

[Qemu-devel] [PATCH v4 5/5] Xen PCI passthru: convert to realize()

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen_pt.c | 53 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 9eef3df..d33221b 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -760,10 +760,10 @@ static void xen_pt_destroy(PCIDevice *d) {
 }
 /* init */
 
-static int xen_pt_initfn(PCIDevice *d)
+static void xen_pt_realize(PCIDevice *d, Error **errp)
 {
     XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
-    int rc = 0;
+    int i, rc = 0;
     uint8_t machine_irq = 0, scratch;
     uint16_t cmd = 0;
     int pirq = XEN_PT_UNASSIGNED_PIRQ;
@@ -781,8 +781,8 @@ static int xen_pt_initfn(PCIDevice *d)
                             &err);
     if (err) {
         error_append_hint(&err, "Failed to \"open\" the real pci device");
-        error_report_err(err);
-        return -1;
+        error_propagate(errp, err);
+        return;
     }
 
     s->is_virtfn = s->real_device.is_virtfn;
@@ -802,19 +802,19 @@ static int xen_pt_initfn(PCIDevice *d)
     if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
         (s->real_device.dev == 2) && (s->real_device.func == 0)) {
         if (!is_igd_vga_passthrough(&s->real_device)) {
-            XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
-                       " to passthrough IGD GFX.\n");
+            error_setg(errp, "Need to enable igd-passthru if you're trying"
+                    " to passthrough IGD GFX");
             xen_host_pci_device_put(&s->real_device);
-            return -1;
+            return;
         }
 
         xen_pt_setup_vga(s, &s->real_device, &err);
         if (err) {
             error_append_hint(&err, "Setup VGA BIOS of passthrough"
                     " GFX failed");
-            error_report_err(err);
+            error_propagate(errp, err);
             xen_host_pci_device_put(&s->real_device);
-            return -1;
+            return;
         }
 
         /* Register ISA bridge for passthrough GFX. */
@@ -836,20 +836,19 @@ static int xen_pt_initfn(PCIDevice *d)
     /* Bind interrupt */
     rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
     if (rc) {
-        XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
+        error_setg_errno(errp, errno, "Failed to read PCI_INTERRUPT_PIN");
         goto err_out;
     }
     if (!scratch) {
-        XEN_PT_LOG(d, "no pin interrupt\n");
+        error_setg(errp, "no pin interrupt");
         goto out;
     }
 
     machine_irq = s->real_device.irq;
     rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
-
     if (rc < 0) {
-        XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
-                   machine_irq, pirq, errno);
+        error_setg_errno(errp, errno, "Mapping machine irq %u to"
+                         " pirq %i failed", machine_irq, pirq);
 
         /* Disable PCI intx assertion (turn on bit10 of devctl) */
         cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -870,8 +869,8 @@ static int xen_pt_initfn(PCIDevice *d)
                                        PCI_SLOT(d->devfn),
                                        e_intx);
         if (rc < 0) {
-            XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
-                       e_intx, errno);
+            error_setg_errno(errp, errno, "Binding of interrupt %u failed",
+                             e_intx);
 
             /* Disable PCI intx assertion (turn on bit10 of devctl) */
             cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -879,8 +878,8 @@ static int xen_pt_initfn(PCIDevice *d)
 
             if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
                 if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
-                    XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
-                               " (err: %d)\n", machine_irq, errno);
+                    error_setg_errno(errp, errno, "Unmapping of machine"
+                            " interrupt %u failed", machine_irq);
                 }
             }
             s->machine_irq = 0;
@@ -893,14 +892,14 @@ out:
 
         rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
         if (rc) {
-            XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
+            error_setg_errno(errp, errno, "Failed to read PCI_COMMAND");
             goto err_out;
         } else {
             val |= cmd;
             rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
             if (rc) {
-                XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
-                           val, rc);
+                error_setg_errno(errp, errno, "Failed to write PCI_COMMAND"
+                                 " val = 0x%x", val);
                 goto err_out;
             }
         }
@@ -910,15 +909,19 @@ out:
     memory_listener_register(&s->io_listener, &address_space_io);
     s->listener_set = true;
     XEN_PT_LOG(d,
-               "Real physical device %02x:%02x.%d registered successfully!\n",
+               "Real physical device %02x:%02x.%d registered successfully\n",
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
 
-    return 0;
+    return;
 
 err_out:
+    for (i = 0; i < PCI_ROM_SLOT; i++) {
+        object_unparent(OBJECT(&s->bar[i]));
+    }
+    object_unparent(OBJECT(&s->rom));
+
     xen_pt_destroy(d);
     assert(rc);
-    return rc;
 }
 
 static void xen_pt_unregister_device(PCIDevice *d)
@@ -937,7 +940,7 @@ static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = xen_pt_initfn;
+    k->realize = xen_pt_realize;
     k->exit = xen_pt_unregister_device;
     k->config_read = xen_pt_pci_read_config;
     k->config_write = xen_pt_pci_write_config;
-- 
2.1.0