[Qemu-devel] [PATCH v3 0/4] Convert to realize()

[Qemu-devel] [PATCH v3 0/4] Convert to realize()

[Cao jin]

v3 changelog:
1. use following style when we want to check the returned error

     Error *err = NULL;
     foo(arg, &err);
     if (err) {
         handle the error...
         error_propagate(errp, err);
     }

Cao jin (4):
  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 | 106 +++++++++++++++++++++++++------------------
 hw/xen/xen-host-pci-device.h |   5 +-
 hw/xen/xen_pt.c              |  73 ++++++++++++++++-------------
 hw/xen/xen_pt.h              |   5 +-
 hw/xen/xen_pt_config_init.c  |  51 +++++++++++----------
 hw/xen/xen_pt_graphics.c     |  11 +++--
 6 files changed, 141 insertions(+), 110 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v3 1/4] 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 | 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");
+        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;
     }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get()

[Eric Blake]

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

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.

-- 
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 v3 1/4] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

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.

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get()

[Eric Blake]

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

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.

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

[Qemu-devel] [PATCH v3 2/4] 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>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_pt.c          |  5 ++++-
 hw/xen/xen_pt.h          |  3 ++-
 hw/xen/xen_pt_graphics.c | 11 ++++++-----
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 1bd4109..fbce55c 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -807,7 +807,10 @@ static int xen_pt_initfn(PCIDevice *d)
             return -1;
         }
 
-        if (xen_pt_setup_vga(s, &s->real_device) < 0) {
+        xen_pt_setup_vga(s, &s->real_device, &local_err);
+        if (local_err) {
+            error_append_hint(&local_err, "Setup VGA BIOS of passthrough"
+                    " GFX failed!");
             XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
             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..a0a7e9c 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

Re: [Qemu-devel] [PATCH v3 2/4] Add Error **errp for xen_pt_setup_vga()

[Eric Blake]

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

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>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/xen/xen_pt.c          |  5 ++++-
>  hw/xen/xen_pt.h          |  3 ++-
>  hw/xen/xen_pt_graphics.c | 11 ++++++-----
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 1bd4109..fbce55c 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -807,7 +807,10 @@ static int xen_pt_initfn(PCIDevice *d)
>              return -1;
>          }
>  
> -        if (xen_pt_setup_vga(s, &s->real_device) < 0) {
> +        xen_pt_setup_vga(s, &s->real_device, &local_err);
> +        if (local_err) {
> +            error_append_hint(&local_err, "Setup VGA BIOS of passthrough"
> +                    " GFX failed!");

Please no '!' in error messages.  We aren't shouting at the user.

>              XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");

Do we still need the XEN_PT_ERR() alongside setting the local error?

>              xen_host_pci_device_put(&s->real_device);
>              return -1;

This leaks local_err.


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

Please drop the trailing '!' while touching this

-- 
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 v3 2/4] Add Error **errp for xen_pt_setup_vga()

[Cao jin]

On 01/06/2016 11:53 PM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>
> Please no '!' in error messages.  We aren't shouting at the user.

Interesting, I didn`t think of this kind of questions before. will fix

>
>>               XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
>
> Do we still need the XEN_PT_ERR() alongside setting the local error?
>

this is fixed in "4/4 convert to realize" patch, as I said in last mail, 
I try to make every patch as small as possible

>>               xen_host_pci_device_put(&s->real_device);
>>               return -1;
>
> This leaks local_err.
>
>
[...]
-- 
Yours Sincerely,

Cao jin

[Qemu-devel] [PATCH v3 3/4] 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             |  7 ++++---
 hw/xen/xen_pt.h             |  2 +-
 hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++---------------------
 3 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index fbce55c..3787c26 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -824,9 +824,10 @@ 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, &local_err);
+    if (local_err) {
+        error_append_hint(&local_err, "PCI Config space initialisation failed");
+        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..72b10dd 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 *local_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(&local_err, "Failed to initialize %d/%ld, 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, local_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, &local_err);
+                    if (local_err) {
+                        error_append_hint(&local_err, "Failed to initialize %d/%ld"
+                                " reg 0x%x in grp_type=0x%x (%d/%ld)",
+                                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, local_err);
                         xen_pt_config_delete(s);
-                        return rc;
+                        return;
                     }
                 }
             }
         }
     }
-
-    return 0;
 }
 
 /* delete all emulate register */
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init()

[Eric Blake]

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

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_pt.c             |  7 ++++---
>  hw/xen/xen_pt.h             |  2 +-
>  hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++---------------------
>  3 files changed, 32 insertions(+), 28 deletions(-)
> 

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

Indentation is now off.


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

Drop the trailing !.  Also, while touching this, it's better to have a
space before ( in English.


> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
>  {
>      int i, rc;
> +    Error *local_err = NULL;

Same comments as earlier in the series about using the shorter 'err'
instead of 'local_err'.

>  
>      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(&local_err, "Failed to initialize %d/%ld, type=0x%x,"
> +                           " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),

This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
compile on 32-bit platforms (where size_t is not necessarily long).  Fix
it to %zd while touching it.

-- 
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 v3 3/4] Add Error **errp for xen_pt_config_init()

[Cao jin]

On 01/07/2016 12:02 AM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>> +static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
>> +                                  XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
>> +                                  Error **errp)
>
> Indentation is now off.
>

Sharp eyes;)

>> @@ -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);
>
> Drop the trailing !.  Also, while touching this, it's better to have a
> space before ( in English.
>

Ok

>
>> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
>>   {
>>       int i, rc;
>> +    Error *local_err = NULL;
>
> Same comments as earlier in the series about using the shorter 'err'
> instead of 'local_err'.
>
>>
>>       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(&local_err, "Failed to initialize %d/%ld, type=0x%x,"
>> +                           " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
>
> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
> compile on 32-bit platforms (where size_t is not necessarily long).  Fix
> it to %zd while touching it.
>

a question:
1. Is %zu more suitable for size_t? since size_t is unsigned integer.

and a personal question after digging into size_t:
2. Does the size of size_t always equal to the word length[*] of computer

[*] https://en.wikipedia.org/wiki/Word_%28computer_architecture%29

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init()

[Eric Blake]

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

On 01/07/2016 01:12 AM, Cao jin wrote:

>>>               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(&local_err, "Failed to initialize %d/%ld,
>>> type=0x%x,"
>>> +                           " rc:%d", i,
>>> ARRAY_SIZE(xen_pt_emu_reg_grps),
>>
>> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
>> compile on 32-bit platforms (where size_t is not necessarily long).  Fix
>> it to %zd while touching it.
>>
> 
> a question:
> 1. Is %zu more suitable for size_t? since size_t is unsigned integer.

Yes, you're right on that one.

> 
> and a personal question after digging into size_t:
> 2. Does the size of size_t always equal to the word length[*] of computer

No.  It equals the maximum size the program can use.  But the x32 ABI
project is a good example of a 32-bit size_t while still taking full
advantage of the 64-bit word size registers, in the name of memory
efficiencies.  See https://en.wikipedia.org/wiki/X32_ABI.

-- 
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 v3 3/4] Add Error **errp for xen_pt_config_init()

[Cao jin]

On 01/08/2016 12:51 AM, Eric Blake wrote:
> On 01/07/2016 01:12 AM, Cao jin wrote:
>
>>>>                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(&local_err, "Failed to initialize %d/%ld,
>>>> type=0x%x,"
>>>> +                           " rc:%d", i,
>>>> ARRAY_SIZE(xen_pt_emu_reg_grps),
>>>
>>> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
>>> compile on 32-bit platforms (where size_t is not necessarily long).  Fix
>>> it to %zd while touching it.
>>>
>>
>> a question:
>> 1. Is %zu more suitable for size_t? since size_t is unsigned integer.
>
> Yes, you're right on that one.
>
>>
>> and a personal question after digging into size_t:
>> 2. Does the size of size_t always equal to the word length[*] of computer
>
> No.  It equals the maximum size the program can use.  But the x32 ABI
> project is a good example of a 32-bit size_t while still taking full
> advantage of the 64-bit word size registers, in the name of memory
> efficiencies.  See https://en.wikipedia.org/wiki/X32_ABI.
>

Thanks very much. Have send v4 version, hope I don`t miss any comment:)

-- 
Yours Sincerely,

Cao jin

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

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.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 3787c26..b058f61 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;
@@ -780,8 +780,8 @@ static int xen_pt_initfn(PCIDevice *d)
                             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;
+        error_propagate(errp, local_err);
+        return;
     }
 
     s->is_virtfn = s->real_device.is_virtfn;
@@ -801,19 +801,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, &local_err);
         if (local_err) {
             error_append_hint(&local_err, "Setup VGA BIOS of passthrough"
                     " GFX failed!");
-            XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+            error_propagate(errp, local_err);
             xen_host_pci_device_put(&s->real_device);
-            return -1;
+            return;
         }
 
         /* Register ISA bridge for passthrough GFX. */
@@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
     xen_pt_config_init(s, &local_err);
     if (local_err) {
         error_append_hint(&local_err, "PCI Config space initialisation failed");
-        rc = -1;
+        error_propagate(errp, local_err);
         goto err_out;
     }
 
     /* 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;
@@ -868,8 +867,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 %i failed!",
+                             e_intx);
 
             /* Disable PCI intx assertion (turn on bit10 of devctl) */
             cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -877,8 +876,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 %i failed!", machine_irq);
                 }
             }
             s->machine_irq = 0;
@@ -891,14 +890,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;
             }
         }
@@ -911,12 +910,16 @@ out:
                "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)
@@ -935,7 +938,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

Re: [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize()

[Eric Blake]

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

On 01/05/2016 07:39 PM, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/xen/xen_pt.c | 53 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 25 deletions(-)
> 

> @@ -801,19 +801,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.");

No trailing '.' in error_setg() messages.


> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>      xen_pt_config_init(s, &local_err);
>      if (local_err) {
>          error_append_hint(&local_err, "PCI Config space initialisation failed");
> -        rc = -1;
> +        error_propagate(errp, local_err);
>          goto err_out;
>      }

Looks like this fixes a memory leak in an earlier patch; maybe you need
to shuffle hunks around?

>  
>      /* 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!");

No trailing '!'


> @@ -891,14 +890,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!");

and again


> @@ -911,12 +910,16 @@ out:
>                 "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;

Is the assertion still needed?

-- 
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 v3 4/4] Xen PCI passthru: convert to realize()

[Cao jin]

On 01/07/2016 12:07 AM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>
>> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>>       xen_pt_config_init(s, &local_err);
>>       if (local_err) {
>>           error_append_hint(&local_err, "PCI Config space initialisation failed");
>> -        rc = -1;
>> +        error_propagate(errp, local_err);
>>           goto err_out;
>>       }
>
> Looks like this fixes a memory leak in an earlier patch; maybe you need
> to shuffle hunks around?
>

Sorry, don`t quite undertand what "shuffle hunks around" means, could 
you detail it?

[...]
>>
>>   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;
>
> Is the assertion still needed?
>

Actually I think in the original code, the assertion isn`t necessary 
too, but I guess it is a kind of defensive coding? You can see, not very 
if (rc) equals TRUE will goto err_out. So I prefer not to touch it when 
I am not sure about the author`s intent.

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize()

[Eric Blake]

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

On 01/07/2016 12:16 AM, Cao jin wrote:
> 
> 
> On 01/07/2016 12:07 AM, Eric Blake wrote:
>> On 01/05/2016 07:39 PM, Cao jin wrote:
> [...]
>>
>>> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>>>       xen_pt_config_init(s, &local_err);
>>>       if (local_err) {
>>>           error_append_hint(&local_err, "PCI Config space
>>> initialisation failed");
>>> -        rc = -1;
>>> +        error_propagate(errp, local_err);
>>>           goto err_out;
>>>       }
>>
>> Looks like this fixes a memory leak in an earlier patch; maybe you need
>> to shuffle hunks around?
>>
> 
> Sorry, don`t quite undertand what "shuffle hunks around" means, could
> you detail it?

It means this change should occur in an earlier patch (probably 2/4?).
I shuffle hunks by using 'git rebase -i'.

-- 
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 v3 0/4] Convert to realize()

[Stefano Stabellini]

On Wed, 6 Jan 2016, Cao jin wrote:
> v3 changelog:
> 1. use following style when we want to check the returned error
> 
>      Error *err = NULL;
>      foo(arg, &err);
>      if (err) {
>          handle the error...
>          error_propagate(errp, err);
>      }
> 
> Cao jin (4):
>   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 | 106 +++++++++++++++++++++++++------------------
>  hw/xen/xen-host-pci-device.h |   5 +-
>  hw/xen/xen_pt.c              |  73 ++++++++++++++++-------------
>  hw/xen/xen_pt.h              |   5 +-
>  hw/xen/xen_pt_config_init.c  |  51 +++++++++++----------
>  hw/xen/xen_pt_graphics.c     |  11 +++--
>  6 files changed, 141 insertions(+), 110 deletions(-)

Thanks Cao, I applied the whole series to my next branch.

Re: [Qemu-devel] [PATCH v3 0/4] Convert to realize()

[Eric Blake]

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

On 01/06/2016 04:08 AM, Stefano Stabellini wrote:
> On Wed, 6 Jan 2016, Cao jin wrote:
>> v3 changelog:
>> 1. use following style when we want to check the returned error
>>
>>      Error *err = NULL;
>>      foo(arg, &err);
>>      if (err) {
>>          handle the error...
>>          error_propagate(errp, err);
>>      }
>>
>> Cao jin (4):
>>   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 | 106 +++++++++++++++++++++++++------------------
>>  hw/xen/xen-host-pci-device.h |   5 +-
>>  hw/xen/xen_pt.c              |  73 ++++++++++++++++-------------
>>  hw/xen/xen_pt.h              |   5 +-
>>  hw/xen/xen_pt_config_init.c  |  51 +++++++++++----------
>>  hw/xen/xen_pt_graphics.c     |  11 +++--
>>  6 files changed, 141 insertions(+), 110 deletions(-)
> 
> Thanks Cao, I applied the whole series to my next branch.

I found some issues while reviewing; maybe you want to wait for a v4.

-- 
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 v3 0/4] Convert to realize()

[Stefano Stabellini]

On Wed, 6 Jan 2016, Eric Blake wrote:
> On 01/06/2016 04:08 AM, Stefano Stabellini wrote:
> > On Wed, 6 Jan 2016, Cao jin wrote:
> >> v3 changelog:
> >> 1. use following style when we want to check the returned error
> >>
> >>      Error *err = NULL;
> >>      foo(arg, &err);
> >>      if (err) {
> >>          handle the error...
> >>          error_propagate(errp, err);
> >>      }
> >>
> >> Cao jin (4):
> >>   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 | 106 +++++++++++++++++++++++++------------------
> >>  hw/xen/xen-host-pci-device.h |   5 +-
> >>  hw/xen/xen_pt.c              |  73 ++++++++++++++++-------------
> >>  hw/xen/xen_pt.h              |   5 +-
> >>  hw/xen/xen_pt_config_init.c  |  51 +++++++++++----------
> >>  hw/xen/xen_pt_graphics.c     |  11 +++--
> >>  6 files changed, 141 insertions(+), 110 deletions(-)
> > 
> > Thanks Cao, I applied the whole series to my next branch.
> 
> I found some issues while reviewing; maybe you want to wait for a v4.

Sure, thanks for reviewing.