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

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

[Cao jin]

v2 changelog:
1. separate every supporting function into a single patch
2. Change supporting function type to void, for consistency
3. other fixes as per Stefano`s review

@Stefano: the last patch build failure you feedback:

qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’:
qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used uninitialized in this func

I admit it is careless misktake(fixed), but weird thing is, I confirmed
again, it indeed can compile in my environment, the evidence is:
1. every time I make after touching xen_pt_config_init.c, I can see the log:
    CC    x86_64-softmmu/hw/xen/xen_pt_config_init.o
    LINK  x86_64-softmmu/qemu-system-x86_64
2. $ readelf -s qemu-system-x86_64 | grep xen_pt_config_init , can got the
   symbol

Will the different compiler make the difference?(I am using
gcc (GCC) 4.9.2 20150212 (Red Hat 4.9.2-6))

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 | 102 ++++++++++++++++++++++++-------------------
 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  |  48 ++++++++++----------
 hw/xen/xen_pt_graphics.c     |  11 ++---
 6 files changed, 134 insertions(+), 110 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 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 | 102 ++++++++++++++++++++++++-------------------
 hw/xen/xen-host-pci-device.h |   5 ++-
 hw/xen/xen_pt.c              |  12 ++---
 3 files changed, 67 insertions(+), 52 deletions(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 7d8a023..3d22095 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,11 @@ 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;
 
     d->config_fd = -1;
     d->domain = domain;
@@ -353,43 +360,48 @@ 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, errp);
+    if (*errp) {
         goto error;
     }
-    rc = xen_host_pci_get_resource(d);
-    if (rc) {
+
+    xen_host_pci_get_resource(d, errp);
+    if (*errp) {
         goto error;
     }
-    rc = xen_host_pci_get_hex_value(d, "vendor", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "vendor", &v, errp);
+    if (*errp) {
         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, errp);
+    if (*errp) {
         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, errp);
+    if (*errp) {
         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, errp);
+    if (*errp) {
         goto error;
     }
     d->class_code = v;
+
     d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
 
-    return 0;
+    return;
 error:
     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 v2 1/4] Add Error **errp for xen_host_pci_device_get()

[Stefano Stabellini]

On Sun, 27 Dec 2015, Cao jin wrote:
> To catch the error msg. Also modify the caller
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>

This looks much better, thanks.


>  hw/xen/xen-host-pci-device.c | 102 ++++++++++++++++++++++++-------------------
>  hw/xen/xen-host-pci-device.h |   5 ++-
>  hw/xen/xen_pt.c              |  12 ++---
>  3 files changed, 67 insertions(+), 52 deletions(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 7d8a023..3d22095 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,11 @@ 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;
>  
>      d->config_fd = -1;
>      d->domain = domain;
> @@ -353,43 +360,48 @@ 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, errp);
> +    if (*errp) {

I think that errp could be NULL, therefore the right way to do this is:

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

see the comment at the beginning of include/qapi/error.h.


>          goto error;
>      }
> -    rc = xen_host_pci_get_resource(d);
> -    if (rc) {
> +
> +    xen_host_pci_get_resource(d, errp);
> +    if (*errp) {
>          goto error;
>      }
> -    rc = xen_host_pci_get_hex_value(d, "vendor", &v);
> -    if (rc) {
> +
> +    xen_host_pci_get_hex_value(d, "vendor", &v, errp);
> +    if (*errp) {
>          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, errp);
> +    if (*errp) {
>          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, errp);
> +    if (*errp) {
>          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, errp);
> +    if (*errp) {
>          goto error;
>      }
>      d->class_code = v;
> +
>      d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
>  
> -    return 0;
> +    return;
>  error:
>      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 v2 1/4] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

On 01/04/2016 11:15 PM, Stefano Stabellini wrote:
> On Sun, 27 Dec 2015, Cao jin wrote:
>> To catch the error msg. Also modify the caller
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>
> This looks much better, thanks.
>
>
[...]
>>
>> -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;
>>
>>       d->config_fd = -1;
>>       d->domain = domain;
>> @@ -353,43 +360,48 @@ 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, errp);
>> +    if (*errp) {
>
> I think that errp could be NULL, therefore the right way to do this is:
>
>      Error *err = NULL;
>      foo(arg, &err);
>      if (err) {
>          handle the error...
>          error_propagate(errp, err);
>      }
>
> see the comment at the beginning of include/qapi/error.h.
>

Thanks for reminding, I didn`t see the comment of error.h before, now I 
am aware why lots of people like the style you mentioned. Will fix it in 
next version, also the comments in other patch.

[...]

-- 
Yours Sincerely,

Cao jin

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

[Cao jin]

On 01/04/2016 11:15 PM, Stefano Stabellini wrote:
> On Sun, 27 Dec 2015, Cao jin wrote:
>> To catch the error msg. Also modify the caller
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>
> This looks much better, thanks.
>
>
[...]
>>
>> -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;
>>
>>       d->config_fd = -1;
>>       d->domain = domain;
>> @@ -353,43 +360,48 @@ 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, errp);
>> +    if (*errp) {
>
> I think that errp could be NULL, therefore the right way to do this is:
>
>      Error *err = NULL;
>      foo(arg, &err);
>      if (err) {
>          handle the error...
>          error_propagate(errp, err);
>      }
>
> see the comment at the beginning of include/qapi/error.h.
>

Hi stefano,

     I read that comment, and find something maybe new:

"errp could be NULL", I think it is saying, if we are in a .realize() 
function, yes, *errp* maybe NULL, but reality is, here is the callee of 
.realize(), and we defined a local variable: Error *local_err = NULL in 
.realize() and passed it to all the callee, so, theoretically *errp* 
won`t be NULL. so the way you said above is suitable in .realize() IMHO, 
and I also did it in that way.

comment also says:

  * Receive an error and pass it on to the caller:
  *     Error *err = NULL;
  *     foo(arg, &err);
  *     if (err) {
  *         handle the error...
  *         error_propagate(errp, err);
  *     }
  * where Error **errp is a parameter, by convention the last one.

If I understand the last sentence well, the Error **errp in .realize() 
prototype is *the last one*, so we could call error_propagate(errp, err) 
only in .realize()

The comment also says:

  * But when all you do with the error is pass it on, please use
  *     foo(arg, errp);
  * for readability."

We just pass error on in all the callees, so I guess I also did as 
comment suggest?

How do you think?

[...]
-- 
Yours Sincerely,

Cao Jin

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

[Stefano Stabellini]

On Tue, 5 Jan 2016, Cao jin wrote:
> On 01/04/2016 11:15 PM, Stefano Stabellini wrote:
> > On Sun, 27 Dec 2015, Cao jin wrote:
> > > To catch the error msg. Also modify the caller
> > > 
> > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> > 
> > This looks much better, thanks.
> > 
> > 
> [...]
> > > 
> > > -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;
> > > 
> > >       d->config_fd = -1;
> > >       d->domain = domain;
> > > @@ -353,43 +360,48 @@ 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, errp);
> > > +    if (*errp) {
> > 
> > I think that errp could be NULL, therefore the right way to do this is:
> > 
> >      Error *err = NULL;
> >      foo(arg, &err);
> >      if (err) {
> >          handle the error...
> >          error_propagate(errp, err);
> >      }
> > 
> > see the comment at the beginning of include/qapi/error.h.
> > 
> 
> Hi stefano,
> 
>     I read that comment, and find something maybe new:
> 
> "errp could be NULL", I think it is saying, if we are in a .realize()
> function, yes, *errp* maybe NULL, but reality is, here is the callee of
> .realize(), and we defined a local variable: Error *local_err = NULL in
> .realize() and passed it to all the callee, so, theoretically *errp* won`t be
> NULL.

This is true, however I think that relying on it is error prone: in a
couple of years from now somebody might change the call sequence without
updating the error handling (easy to forget), causing QEMU to crash on
error. I think it is safer not to rely on errp != NULL.


> so the way you said above is suitable in .realize() IMHO, and I also did
> it in that way.
> 
> comment also says:
> 
>  * Receive an error and pass it on to the caller:
>  *     Error *err = NULL;
>  *     foo(arg, &err);
>  *     if (err) {
>  *         handle the error...
>  *         error_propagate(errp, err);
>  *     }
>  * where Error **errp is a parameter, by convention the last one.
> 
> If I understand the last sentence well, the Error **errp in .realize()
> prototype is *the last one*, so we could call error_propagate(errp, err) only
> in .realize()
> 
> The comment also says:
> 
>  * But when all you do with the error is pass it on, please use
>  *     foo(arg, errp);
>  * for readability."
> 
> We just pass error on in all the callees, so I guess I also did as comment
> suggest?
> 
> How do you think?
 
I think we only need to use a local Error variable when we want to check
for the returned error, in cases such as:

if (*errp) {

In other cases, when we are not interested in *errp, we can simply
propagate the error, like you have done in your patches.

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

[Cao jin]

On 01/05/2016 06:40 PM, Stefano Stabellini wrote:
> On Tue, 5 Jan 2016, Cao jin wrote:
[...]
>
> This is true, however I think that relying on it is error prone: in a
> couple of years from now somebody might change the call sequence without
> updating the error handling (easy to forget), causing QEMU to crash on
> error. I think it is safer not to rely on errp != NULL.
>

I see, sounds reasonable

>
[...]

> I think we only need to use a local Error variable when we want to check
> for the returned error, in cases such as:
>
> if (*errp) {
>
> In other cases, when we are not interested in *errp, we can simply
> propagate the error, like you have done in your patches.
>

Ok, will fix it

> .
>

-- 
Yours Sincerely,

Cao Jin

[Qemu-devel] [PATCH v2 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>
---
 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 v2 2/4] Add Error **errp for xen_pt_setup_vga()

[Stefano Stabellini]

On Sun, 27 Dec 2015, 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!");
>              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
> 
> 
> 

[Qemu-devel] [PATCH v2 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 | 48 ++++++++++++++++++++++-----------------------
 3 files changed, 29 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..096f204 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,11 +1993,9 @@ 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;
 
@@ -2039,11 +2040,11 @@ 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(errp, "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);
                 xen_pt_config_delete(s);
-                return rc;
+                return;
             }
         }
 
@@ -2051,24 +2052,23 @@ 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, errp);
+                    if (*errp) {
+                        error_append_hint(errp, "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));
                         xen_pt_config_delete(s);
-                        return rc;
+                        return;
                     }
                 }
             }
         }
     }
-
-    return 0;
 }
 
 /* delete all emulate register */
-- 
2.1.0

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

[Stefano Stabellini]

On Sun, 27 Dec 2015, 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 | 48 ++++++++++++++++++++++-----------------------
>  3 files changed, 29 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..096f204 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,11 +1993,9 @@ 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;
>  
> @@ -2039,11 +2040,11 @@ 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(errp, "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);
>                  xen_pt_config_delete(s);
> -                return rc;
> +                return;
>              }
>          }
>  
> @@ -2051,24 +2052,23 @@ 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, errp);
> +                    if (*errp) {

errp could theoretically be NULL


> +                        error_append_hint(errp, "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));
>                          xen_pt_config_delete(s);
> -                        return rc;
> +                        return;
>                      }
>                  }
>              }
>          }
>      }
> -
> -    return 0;
>  }
>  
>  /* delete all emulate register */
> -- 
> 2.1.0
> 
> 
> 

[Qemu-devel] [PATCH v2 4/4] 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 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 v2 4/4] Xen PCI passthru: convert to realize()

[Stefano Stabellini]

On Sun, 27 Dec 2015, 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(-)
> 
> 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
> 
> 
>