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

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

[Cao jin]

v5 changelog:
1. tweaked the commit message of patch 1/5 as sugguested.
2. xen_host_pci_sysfs_path() modification as sugguested.
3. remove local 'value' variable in xen_host_pci_get_value()
4. Indentation fix in patch 2/5.
5. Still use error_append_hint() because error_prepend() doesn`t exist.

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

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

-- 
2.1.0

[Qemu-devel] [PATCH v5 1/5] Xen: use qemu_strtoul instead of strtol

[Cao jin]

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

Change signed parsing to unsigned, because what it read are values in
PCI config space, which are non-negative.

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

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

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

[Cao jin]

To catch the error msg. Also modify the caller

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

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 351b61a..3e22de8 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -31,25 +31,20 @@
 #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
 #define IORESOURCE_MEM_64       0x00100000
 
-static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
-                                   const char *name, char *buf, ssize_t size)
+static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
+                                    const char *name, char *buf, ssize_t size)
 {
     int rc;
 
     rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
                   d->domain, d->bus, d->dev, d->func, name);
-
-    if (rc >= size || rc < 0) {
-        /* The output is truncated, or some other error was encountered */
-        return -ENODEV;
-    }
-    return 0;
+    assert(rc >= 0 && rc < size);
 }
 
 
 /* This size should be enough to read the first 7 lines of a resource file */
 #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
-static int xen_host_pci_get_resource(XenHostPCIDevice *d)
+static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
 {
     int i, rc, fd;
     char path[PATH_MAX];
@@ -58,25 +53,22 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
     char *endptr, *s;
     uint8_t type;
 
-    rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
-    if (rc) {
-        return rc;
-    }
+    xen_host_pci_sysfs_path(d, "resource", path, sizeof(path));
+
     fd = open(path, O_RDONLY);
     if (fd == -1) {
-        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
-        return -errno;
+        error_setg_file_open(errp, errno, path);
+        return;
     }
 
     do {
-        rc = read(fd, &buf, sizeof (buf) - 1);
+        rc = read(fd, &buf, sizeof(buf) - 1);
         if (rc < 0 && errno != EINTR) {
-            rc = -errno;
+            error_setg_errno(errp, errno, "read err");
             goto out;
         }
     } while (rc < 0);
     buf[rc] = 0;
-    rc = 0;
 
     s = buf;
     for (i = 0; i < PCI_NUM_REGIONS; i++) {
@@ -129,65 +121,65 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
             d->rom.bus_flags = flags & IORESOURCE_BITS;
         }
     }
+
     if (i != PCI_NUM_REGIONS) {
-        /* Invalid format or input to short */
-        rc = -ENODEV;
+        error_setg(errp, "Invalid format or input too short: %s", buf);
     }
 
 out:
     close(fd);
-    return rc;
 }
 
 /* This size should be enough to read a long from a file */
 #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
-static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
-                                  unsigned int *pvalue, int base)
+static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
+                                   unsigned int *pvalue, int base, Error **errp)
 {
     char path[PATH_MAX];
     char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
     int fd, rc;
-    unsigned long value;
     const char *endptr;
 
-    rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
-    if (rc) {
-        return rc;
-    }
+    xen_host_pci_sysfs_path(d, name, path, sizeof(path));
+
     fd = open(path, O_RDONLY);
     if (fd == -1) {
-        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
-        return -errno;
+        error_setg_file_open(errp, errno, path);
+        return;
     }
+
     do {
-        rc = read(fd, &buf, sizeof (buf) - 1);
+        rc = read(fd, &buf, sizeof(buf) - 1);
         if (rc < 0 && errno != EINTR) {
-            rc = -errno;
+            error_setg_errno(errp, errno, "read err");
             goto out;
         }
     } while (rc < 0);
+
     buf[rc] = 0;
-    rc = qemu_strtoul(buf, &endptr, base, &value);
-    if (!rc) {
-        *pvalue = value;
+    rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue);
+    if (rc) {
+        error_setg_errno(errp, -rc, "failed to parse value '%s'", buf);
     }
+
 out:
     close(fd);
-    return rc;
 }
 
-static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
-                                             const char *name,
-                                             unsigned int *pvalue)
+static inline void xen_host_pci_get_hex_value(XenHostPCIDevice *d,
+                                              const char *name,
+                                              unsigned int *pvalue,
+                                              Error **errp)
 {
-    return xen_host_pci_get_value(d, name, pvalue, 16);
+    xen_host_pci_get_value(d, name, pvalue, 16, errp);
 }
 
-static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
-                                             const char *name,
-                                             unsigned int *pvalue)
+static inline void xen_host_pci_get_dec_value(XenHostPCIDevice *d,
+                                              const char *name,
+                                              unsigned int *pvalue,
+                                              Error **errp)
 {
-    return xen_host_pci_get_value(d, name, pvalue, 10);
+    xen_host_pci_get_value(d, name, pvalue, 10, errp);
 }
 
 static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
@@ -195,26 +187,21 @@ static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
     char path[PATH_MAX];
     struct stat buf;
 
-    if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path))) {
-        return false;
-    }
+    xen_host_pci_sysfs_path(d, "physfn", path, sizeof(path));
+
     return !stat(path, &buf);
 }
 
-static int xen_host_pci_config_open(XenHostPCIDevice *d)
+static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
 {
     char path[PATH_MAX];
-    int rc;
 
-    rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
-    if (rc) {
-        return rc;
-    }
+    xen_host_pci_sysfs_path(d, "config", path, sizeof(path));
+
     d->config_fd = open(path, O_RDWR);
-    if (d->config_fd < 0) {
-        return -errno;
+    if (d->config_fd == -1) {
+        error_setg_file_open(errp, errno, path);
     }
-    return 0;
 }
 
 static int xen_host_pci_config_read(XenHostPCIDevice *d,
@@ -336,11 +323,12 @@ int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
     return -1;
 }
 
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
-                            uint8_t bus, uint8_t dev, uint8_t func)
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+                             uint8_t bus, uint8_t dev, uint8_t func,
+                             Error **errp)
 {
     unsigned int v;
-    int rc = 0;
+    Error *err = NULL;
 
     d->config_fd = -1;
     d->domain = domain;
@@ -348,43 +336,51 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
     d->dev = dev;
     d->func = func;
 
-    rc = xen_host_pci_config_open(d);
-    if (rc) {
+    xen_host_pci_config_open(d, &err);
+    if (err) {
         goto error;
     }
-    rc = xen_host_pci_get_resource(d);
-    if (rc) {
+
+    xen_host_pci_get_resource(d, &err);
+    if (err) {
         goto error;
     }
-    rc = xen_host_pci_get_hex_value(d, "vendor", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "vendor", &v, &err);
+    if (err) {
         goto error;
     }
     d->vendor_id = v;
-    rc = xen_host_pci_get_hex_value(d, "device", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "device", &v, &err);
+    if (err) {
         goto error;
     }
     d->device_id = v;
-    rc = xen_host_pci_get_dec_value(d, "irq", &v);
-    if (rc) {
+
+    xen_host_pci_get_dec_value(d, "irq", &v, &err);
+    if (err) {
         goto error;
     }
     d->irq = v;
-    rc = xen_host_pci_get_hex_value(d, "class", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "class", &v, &err);
+    if (err) {
         goto error;
     }
     d->class_code = v;
+
     d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
 
-    return 0;
+    return;
+
 error:
+    error_propagate(errp, err);
+
     if (d->config_fd >= 0) {
         close(d->config_fd);
         d->config_fd = -1;
     }
-    return rc;
 }
 
 bool xen_host_pci_device_closed(XenHostPCIDevice *d)
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index 3d44e04..6acf36e 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -36,8 +36,9 @@ typedef struct XenHostPCIDevice {
     int config_fd;
 } XenHostPCIDevice;
 
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
-                            uint8_t bus, uint8_t dev, uint8_t func);
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+                             uint8_t bus, uint8_t dev, uint8_t func,
+                             Error **errp);
 void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
 bool xen_host_pci_device_closed(XenHostPCIDevice *d);
 
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index aa96288..53b5bca 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -767,6 +767,7 @@ static int xen_pt_initfn(PCIDevice *d)
     uint8_t machine_irq = 0, scratch;
     uint16_t cmd = 0;
     int pirq = XEN_PT_UNASSIGNED_PIRQ;
+    Error *err = NULL;
 
     /* register real device */
     XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
@@ -774,11 +775,13 @@ static int xen_pt_initfn(PCIDevice *d)
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
                s->dev.devfn);
 
-    rc = xen_host_pci_device_get(&s->real_device,
-                                 s->hostaddr.domain, s->hostaddr.bus,
-                                 s->hostaddr.slot, s->hostaddr.function);
-    if (rc) {
-        XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
+    xen_host_pci_device_get(&s->real_device,
+                            s->hostaddr.domain, s->hostaddr.bus,
+                            s->hostaddr.slot, s->hostaddr.function,
+                            &err);
+    if (err) {
+        error_append_hint(&err, "Failed to \"open\" the real pci device");
+        error_report_err(err);
         return -1;
     }
 
-- 
2.1.0

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

[Cao jin]

To catch the error msg. Also modify the caller

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

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

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

[Cao jin]

To catch the error msg. Also modify the caller

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

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

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

[Cao jin]

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

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

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

[Stefano Stabellini]

Eric,

I'll wait for your reviewed-by on the whole series before committing.

Thanks,

Stefano

On Wed, 13 Jan 2016, Cao jin wrote:
> v5 changelog:
> 1. tweaked the commit message of patch 1/5 as sugguested.
> 2. xen_host_pci_sysfs_path() modification as sugguested.
> 3. remove local 'value' variable in xen_host_pci_get_value()
> 4. Indentation fix in patch 2/5.
> 5. Still use error_append_hint() because error_prepend() doesn`t exist.
> 
> Cao jin (5):
>   Xen: use qemu_strtoul instead of strtol
>   Add Error **errp for xen_host_pci_device_get()
>   Add Error **errp for xen_pt_setup_vga()
>   Add Error **errp for xen_pt_config_init()
>   Xen PCI passthru: convert to realize()
> 
>  hw/xen/xen-host-pci-device.c | 149 ++++++++++++++++++++-----------------------
>  hw/xen/xen-host-pci-device.h |   5 +-
>  hw/xen/xen_pt.c              |  77 ++++++++++++----------
>  hw/xen/xen_pt.h              |   5 +-
>  hw/xen/xen_pt_config_init.c  |  51 ++++++++-------
>  hw/xen/xen_pt_graphics.c     |  11 ++--
>  6 files changed, 153 insertions(+), 145 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
> 

Re: [Qemu-devel] [PATCH v5 1/5] Xen: use qemu_strtoul instead of strtol

[Eric Blake]

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

On 01/13/2016 05:51 AM, Cao jin wrote:
> No need to roll our own (with slightly incorrect handling of errno),
> when we can use the common version.
> 
> Change signed parsing to unsigned, because what it read are values in
> PCI config space, which are non-negative.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen-host-pci-device.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v5 2/5] Add Error **errp for xen_host_pci_device_get()

[Eric Blake]

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

On 01/13/2016 05:51 AM, Cao jin wrote:
> To catch the error msg. Also modify the caller
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen-host-pci-device.c | 142 +++++++++++++++++++++----------------------
>  hw/xen/xen-host-pci-device.h |   5 +-
>  hw/xen/xen_pt.c              |  13 ++--
>  3 files changed, 80 insertions(+), 80 deletions(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 351b61a..3e22de8 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -31,25 +31,20 @@
>  #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
>  #define IORESOURCE_MEM_64       0x00100000
>  
> -static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> -                                   const char *name, char *buf, ssize_t size)
> +static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> +                                    const char *name, char *buf, ssize_t size)

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

>  /* This size should be enough to read a long from a file */
>  #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
> -static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> -                                  unsigned int *pvalue, int base)
> +static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> +                                   unsigned int *pvalue, int base, Error **errp)
>  {

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

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

Otherwise looks okay.

-- 
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 v5 3/5] Add Error **errp for xen_pt_setup_vga()

[Eric Blake]

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

On 01/13/2016 05:51 AM, Cao jin wrote:
> To catch the error msg. Also modify the caller
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen_pt.c          |  7 +++++--
>  hw/xen/xen_pt.h          |  3 ++-
>  hw/xen/xen_pt_graphics.c | 11 ++++++-----
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 

> +++ b/hw/xen/xen_pt_graphics.c

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

As long as you are touching this, fix the grammar:

"VGA: Can't get VBIOS"

With that tweak,
Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Eric Blake]

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

On 01/13/2016 05:51 AM, Cao jin wrote:
> To catch the error msg. Also modify the caller

Doesn't hurt to spell out 'message' instead of 'msg' (here and in other
patches), but not a show-stopper.

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

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Eric Blake]

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

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

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v5 0/5] Xen PCI passthru: Convert to realize()

[Eric Blake]

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

On 01/14/2016 09:50 AM, Stefano Stabellini wrote:
> Eric,
> 
> I'll wait for your reviewed-by on the whole series before committing.

Found a bug in 2/5, up to you if you want to fix that or wait for a v6.

> On Wed, 13 Jan 2016, Cao jin wrote:
>> v5 changelog:
>> 1. tweaked the commit message of patch 1/5 as sugguested.
>> 2. xen_host_pci_sysfs_path() modification as sugguested.
>> 3. remove local 'value' variable in xen_host_pci_get_value()

Basically, this change from v4 was not correct, after all.

>> 4. Indentation fix in patch 2/5.
>> 5. Still use error_append_hint() because error_prepend() doesn`t exist.

But it does now: commit 8277d2aa.

-- 
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 v5 2/5] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

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

Thanks for the suggestion, will split it out.

>>   /* This size should be enough to read a long from a file */
>>   #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
>> -static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>> -                                  unsigned int *pvalue, int base)
>> +static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>> +                                   unsigned int *pvalue, int base, Error **errp)
>>   {
>
>>       buf[rc] = 0;
>> -    rc = qemu_strtoul(buf, &endptr, base, &value);
>> -    if (!rc) {
>> -        *pvalue = value;
>> +    rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue);
>
> Ouch. Casting unsigned int * to unsigned long * and then dereferencing
> it is bogus (you end up having qemu_strtoul() write beyond bounds on
> platforms where long is larger than int).

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

> You'll need to revert this
> part of the patch, and stick with *pvalue = value (and possibly even add
> a bounds check that value <= UINT_MAX).
>
> Otherwise looks okay.
>

-- 
Yours Sincerely,

Cao jin

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

[Stefano Stabellini]

On Thu, 14 Jan 2016, Eric Blake wrote:
> On 01/14/2016 09:50 AM, Stefano Stabellini wrote:
> > Eric,
> > 
> > I'll wait for your reviewed-by on the whole series before committing.
> 
> Found a bug in 2/5, up to you if you want to fix that or wait for a v6.

If Cao is happy to update and resend, I'll wait for v6.


> > On Wed, 13 Jan 2016, Cao jin wrote:
> >> v5 changelog:
> >> 1. tweaked the commit message of patch 1/5 as sugguested.
> >> 2. xen_host_pci_sysfs_path() modification as sugguested.
> >> 3. remove local 'value' variable in xen_host_pci_get_value()
> 
> Basically, this change from v4 was not correct, after all.
> 
> >> 4. Indentation fix in patch 2/5.
> >> 5. Still use error_append_hint() because error_prepend() doesn`t exist.
> 
> But it does now: commit 8277d2aa.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 

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

[Eric Blake]

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

On 01/14/2016 08:11 PM, Cao jin wrote:

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

Maybe:

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

And maybe some of it should even be done as part of the conversion to
qemu_strtoul() in 1/5.

-- 
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 v5 0/5] Xen PCI passthru: Convert to realize()

[Cao jin]

On 01/15/2016 10:16 PM, Stefano Stabellini wrote:
> On Thu, 14 Jan 2016, Eric Blake wrote:
>> On 01/14/2016 09:50 AM, Stefano Stabellini wrote:
>>> Eric,
>>>
>>> I'll wait for your reviewed-by on the whole series before committing.
>>
>> Found a bug in 2/5, up to you if you want to fix that or wait for a v6.
>
> If Cao is happy to update and resend, I'll wait for v6.
>

fine by me, will update it:) But I guess that bug maybe not a show-stopper:)

-- 
Yours Sincerely,

Cao jin

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

[Cao jin]

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

Thanks for the example, will give v6 soon.
-- 
Yours Sincerely,

Cao jin