* [Qemu-devel] [PATCH v3 0/4] Convert to realize()
@ 2016-01-06 2:39 Cao jin
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
` (4 more replies)
0 siblings, 5 replies; 20+ messages in thread
From: Cao jin @ 2016-01-06 2:39 UTC (permalink / raw)
To: qemu-devel; +Cc: stefano.stabellini
v3 changelog:
1. use following style when we want to check the returned error
Error *err = NULL;
foo(arg, &err);
if (err) {
handle the error...
error_propagate(errp, err);
}
Cao jin (4):
Add Error **errp for xen_host_pci_device_get()
Add Error **errp for xen_pt_setup_vga()
Add Error **errp for xen_pt_config_init()
Xen PCI passthru: convert to realize()
hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
hw/xen/xen-host-pci-device.h | 5 +-
hw/xen/xen_pt.c | 73 ++++++++++++++++-------------
hw/xen/xen_pt.h | 5 +-
hw/xen/xen_pt_config_init.c | 51 +++++++++++----------
hw/xen/xen_pt_graphics.c | 11 +++--
6 files changed, 141 insertions(+), 110 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get()
2016-01-06 2:39 [Qemu-devel] [PATCH v3 0/4] Convert to realize() Cao jin
@ 2016-01-06 2:39 ` Cao jin
2016-01-06 15:51 ` Eric Blake
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 2/4] Add Error **errp for xen_pt_setup_vga() Cao jin
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Cao jin @ 2016-01-06 2:39 UTC (permalink / raw)
To: qemu-devel; +Cc: stefano.stabellini
To catch the error msg. Also modify the caller
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
hw/xen/xen-host-pci-device.h | 5 +-
hw/xen/xen_pt.c | 12 +++--
3 files changed, 71 insertions(+), 52 deletions(-)
diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 7d8a023..952cae0 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
/* This size should be enough to read the first 7 lines of a resource file */
#define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
-static int xen_host_pci_get_resource(XenHostPCIDevice *d)
+static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
{
int i, rc, fd;
char path[PATH_MAX];
@@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
if (rc) {
- return rc;
+ error_setg_errno(errp, errno, "snprintf err");
+ return;
}
+
fd = open(path, O_RDONLY);
if (fd == -1) {
- XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
- return -errno;
+ error_setg_errno(errp, errno, "open %s err", path);
+ return;
}
do {
rc = read(fd, &buf, sizeof (buf) - 1);
if (rc < 0 && errno != EINTR) {
- rc = -errno;
+ error_setg_errno(errp, errno, "read err");
goto out;
}
} while (rc < 0);
buf[rc] = 0;
- rc = 0;
s = buf;
for (i = 0; i < PCI_NUM_REGIONS; i++) {
@@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
d->rom.bus_flags = flags & IORESOURCE_BITS;
}
}
+
if (i != PCI_NUM_REGIONS) {
/* Invalid format or input to short */
- rc = -ENODEV;
+ error_setg(errp, "Invalid format or input to short: %s", buf);
}
out:
close(fd);
- return rc;
}
/* This size should be enough to read a long from a file */
#define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
-static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
- unsigned int *pvalue, int base)
+static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
+ unsigned int *pvalue, int base, Error **errp)
{
char path[PATH_MAX];
char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
@@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
if (rc) {
- return rc;
+ error_setg_errno(errp, errno, "snprintf err");
+ return;
}
+
fd = open(path, O_RDONLY);
if (fd == -1) {
- XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
- return -errno;
+ error_setg_errno(errp, errno, "open %s err", path);
+ return;
}
+
do {
rc = read(fd, &buf, sizeof (buf) - 1);
if (rc < 0 && errno != EINTR) {
- rc = -errno;
+ error_setg_errno(errp, errno, "read err");
goto out;
}
} while (rc < 0);
+
buf[rc] = 0;
value = strtol(buf, &endptr, base);
if (endptr == buf || *endptr != '\n') {
- rc = -1;
+ error_setg(errp, "format invalid: %s", buf);
} else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
- rc = -errno;
+ error_setg_errno(errp, errno, "strtol err");
} else {
- rc = 0;
*pvalue = value;
}
+
out:
close(fd);
- return rc;
}
-static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
+static inline void xen_host_pci_get_hex_value(XenHostPCIDevice *d,
const char *name,
- unsigned int *pvalue)
+ unsigned int *pvalue,
+ Error **errp)
{
- return xen_host_pci_get_value(d, name, pvalue, 16);
+ xen_host_pci_get_value(d, name, pvalue, 16, errp);
}
-static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
+static inline void xen_host_pci_get_dec_value(XenHostPCIDevice *d,
const char *name,
- unsigned int *pvalue)
+ unsigned int *pvalue,
+ Error **errp)
{
- return xen_host_pci_get_value(d, name, pvalue, 10);
+ xen_host_pci_get_value(d, name, pvalue, 10, errp);
}
static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
@@ -206,20 +212,21 @@ static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
return !stat(path, &buf);
}
-static int xen_host_pci_config_open(XenHostPCIDevice *d)
+static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
{
char path[PATH_MAX];
int rc;
rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
if (rc) {
- return rc;
+ error_setg_errno(errp, errno, "snprintf err");
+ return;
}
+
d->config_fd = open(path, O_RDWR);
if (d->config_fd < 0) {
- return -errno;
+ error_setg_errno(errp, errno, "open %s err", path);
}
- return 0;
}
static int xen_host_pci_config_read(XenHostPCIDevice *d,
@@ -341,11 +348,12 @@ int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
return -1;
}
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
- uint8_t bus, uint8_t dev, uint8_t func)
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+ uint8_t bus, uint8_t dev, uint8_t func,
+ Error **errp)
{
unsigned int v;
- int rc = 0;
+ Error *local_err = NULL;
d->config_fd = -1;
d->domain = domain;
@@ -353,43 +361,51 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
d->dev = dev;
d->func = func;
- rc = xen_host_pci_config_open(d);
- if (rc) {
+ xen_host_pci_config_open(d, &local_err);
+ if (local_err) {
goto error;
}
- rc = xen_host_pci_get_resource(d);
- if (rc) {
+
+ xen_host_pci_get_resource(d, &local_err);
+ if (local_err) {
goto error;
}
- rc = xen_host_pci_get_hex_value(d, "vendor", &v);
- if (rc) {
+
+ xen_host_pci_get_hex_value(d, "vendor", &v, &local_err);
+ if (local_err) {
goto error;
}
d->vendor_id = v;
- rc = xen_host_pci_get_hex_value(d, "device", &v);
- if (rc) {
+
+ xen_host_pci_get_hex_value(d, "device", &v, &local_err);
+ if (local_err) {
goto error;
}
d->device_id = v;
- rc = xen_host_pci_get_dec_value(d, "irq", &v);
- if (rc) {
+
+ xen_host_pci_get_dec_value(d, "irq", &v, &local_err);
+ if (local_err) {
goto error;
}
d->irq = v;
- rc = xen_host_pci_get_hex_value(d, "class", &v);
- if (rc) {
+
+ xen_host_pci_get_hex_value(d, "class", &v, &local_err);
+ if (local_err) {
goto error;
}
d->class_code = v;
+
d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
- return 0;
+ return;
+
error:
+ error_propagate(errp, local_err);
+
if (d->config_fd >= 0) {
close(d->config_fd);
d->config_fd = -1;
}
- return rc;
}
bool xen_host_pci_device_closed(XenHostPCIDevice *d)
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index 3d44e04..42b9138 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -36,8 +36,9 @@ typedef struct XenHostPCIDevice {
int config_fd;
} XenHostPCIDevice;
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
- uint8_t bus, uint8_t dev, uint8_t func);
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+ uint8_t bus, uint8_t dev, uint8_t func,
+ Error **errp);
void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
bool xen_host_pci_device_closed(XenHostPCIDevice *d);
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index aa96288..1bd4109 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -767,6 +767,7 @@ static int xen_pt_initfn(PCIDevice *d)
uint8_t machine_irq = 0, scratch;
uint16_t cmd = 0;
int pirq = XEN_PT_UNASSIGNED_PIRQ;
+ Error *local_err = NULL;
/* register real device */
XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
@@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
s->dev.devfn);
- rc = xen_host_pci_device_get(&s->real_device,
- s->hostaddr.domain, s->hostaddr.bus,
- s->hostaddr.slot, s->hostaddr.function);
- if (rc) {
- XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
+ xen_host_pci_device_get(&s->real_device,
+ s->hostaddr.domain, s->hostaddr.bus,
+ s->hostaddr.slot, s->hostaddr.function,
+ &local_err);
+ if (local_err) {
+ XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
return -1;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v3 2/4] Add Error **errp for xen_pt_setup_vga()
2016-01-06 2:39 [Qemu-devel] [PATCH v3 0/4] Convert to realize() Cao jin
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
@ 2016-01-06 2:39 ` Cao jin
2016-01-06 15:53 ` Eric Blake
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init() Cao jin
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Cao jin @ 2016-01-06 2:39 UTC (permalink / raw)
To: qemu-devel; +Cc: stefano.stabellini
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init()
2016-01-06 2:39 [Qemu-devel] [PATCH v3 0/4] Convert to realize() Cao jin
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 2/4] Add Error **errp for xen_pt_setup_vga() Cao jin
@ 2016-01-06 2:39 ` Cao jin
2016-01-06 16:02 ` Eric Blake
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize() Cao jin
2016-01-06 11:08 ` [Qemu-devel] [PATCH v3 0/4] Convert " Stefano Stabellini
4 siblings, 1 reply; 20+ messages in thread
From: Cao jin @ 2016-01-06 2:39 UTC (permalink / raw)
To: qemu-devel; +Cc: stefano.stabellini
To catch the error msg. Also modify the caller
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/xen/xen_pt.c | 7 ++++---
hw/xen/xen_pt.h | 2 +-
hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++---------------------
3 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index fbce55c..3787c26 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -824,9 +824,10 @@ static int xen_pt_initfn(PCIDevice *d)
xen_pt_register_regions(s, &cmd);
/* reinitialize each config register to be emulated */
- rc = xen_pt_config_init(s);
- if (rc) {
- XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
+ xen_pt_config_init(s, &local_err);
+ if (local_err) {
+ error_append_hint(&local_err, "PCI Config space initialisation failed");
+ rc = -1;
goto err_out;
}
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index dc74d3e..466525f 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -228,7 +228,7 @@ struct XenPCIPassthroughState {
bool listener_set;
};
-int xen_pt_config_init(XenPCIPassthroughState *s);
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp);
void xen_pt_config_delete(XenPCIPassthroughState *s);
XenPTRegGroup *xen_pt_find_reg_grp(XenPCIPassthroughState *s, uint32_t address);
XenPTReg *xen_pt_find_reg(XenPTRegGroup *reg_grp, uint32_t address);
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 3d8686d..72b10dd 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1899,8 +1899,9 @@ static uint8_t find_cap_offset(XenPCIPassthroughState *s, uint8_t cap)
return 0;
}
-static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
- XenPTRegGroup *reg_grp, XenPTRegInfo *reg)
+static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
+ XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
+ Error **errp)
{
XenPTReg *reg_entry;
uint32_t data = 0;
@@ -1919,12 +1920,13 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
reg_grp->base_offset + reg->offset, &data);
if (rc < 0) {
g_free(reg_entry);
- return rc;
+ error_setg(errp, "Init emulate register fail");
+ return;
}
if (data == XEN_PT_INVALID_REG) {
/* free unused BAR register entry */
g_free(reg_entry);
- return 0;
+ return;
}
/* Sync up the data to dev.config */
offset = reg_grp->base_offset + reg->offset;
@@ -1942,7 +1944,8 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
if (rc) {
/* Serious issues when we cannot read the host values! */
g_free(reg_entry);
- return rc;
+ error_setg(errp, "Cannot read host values");
+ return;
}
/* Set bits in emu_mask are the ones we emulate. The dev.config shall
* contain the emulated view of the guest - therefore we flip the mask
@@ -1967,10 +1970,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
val = data;
if (val & ~size_mask) {
- XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
- offset, val, reg->size);
+ error_setg(errp, "Offset 0x%04x:0x%04x expands past"
+ " register size(%d)!", offset, val, reg->size);
g_free(reg_entry);
- return -ENXIO;
+ return;
}
/* This could be just pci_set_long as we don't modify the bits
* past reg->size, but in case this routine is run in parallel or the
@@ -1990,13 +1993,12 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
}
/* list add register entry */
QLIST_INSERT_HEAD(®_grp->reg_tbl_list, reg_entry, entries);
-
- return 0;
}
-int xen_pt_config_init(XenPCIPassthroughState *s)
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
{
int i, rc;
+ Error *local_err = NULL;
QLIST_INIT(&s->reg_grps);
@@ -2039,11 +2041,12 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
reg_grp_offset,
®_grp_entry->size);
if (rc < 0) {
- XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
- i, ARRAY_SIZE(xen_pt_emu_reg_grps),
+ error_setg(&local_err, "Failed to initialize %d/%ld, type=0x%x,"
+ " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
xen_pt_emu_reg_grps[i].grp_type, rc);
+ error_propagate(errp, local_err);
xen_pt_config_delete(s);
- return rc;
+ return;
}
}
@@ -2051,24 +2054,24 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
if (xen_pt_emu_reg_grps[i].emu_regs) {
int j = 0;
XenPTRegInfo *regs = xen_pt_emu_reg_grps[i].emu_regs;
+
/* initialize capability register */
for (j = 0; regs->size != 0; j++, regs++) {
- /* initialize capability register */
- rc = xen_pt_config_reg_init(s, reg_grp_entry, regs);
- if (rc < 0) {
- XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
- j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
- regs->offset, xen_pt_emu_reg_grps[i].grp_type,
- i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
+ xen_pt_config_reg_init(s, reg_grp_entry, regs, &local_err);
+ if (local_err) {
+ error_append_hint(&local_err, "Failed to initialize %d/%ld"
+ " reg 0x%x in grp_type=0x%x (%d/%ld)",
+ j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
+ regs->offset, xen_pt_emu_reg_grps[i].grp_type,
+ i, ARRAY_SIZE(xen_pt_emu_reg_grps));
+ error_propagate(errp, local_err);
xen_pt_config_delete(s);
- return rc;
+ return;
}
}
}
}
}
-
- return 0;
}
/* delete all emulate register */
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize()
2016-01-06 2:39 [Qemu-devel] [PATCH v3 0/4] Convert to realize() Cao jin
` (2 preceding siblings ...)
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init() Cao jin
@ 2016-01-06 2:39 ` Cao jin
2016-01-06 16:07 ` Eric Blake
2016-01-06 11:08 ` [Qemu-devel] [PATCH v3 0/4] Convert " Stefano Stabellini
4 siblings, 1 reply; 20+ messages in thread
From: Cao jin @ 2016-01-06 2:39 UTC (permalink / raw)
To: qemu-devel; +Cc: stefano.stabellini
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] Convert to realize()
2016-01-06 2:39 [Qemu-devel] [PATCH v3 0/4] Convert to realize() Cao jin
` (3 preceding siblings ...)
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize() Cao jin
@ 2016-01-06 11:08 ` Stefano Stabellini
2016-01-06 15:51 ` Eric Blake
4 siblings, 1 reply; 20+ messages in thread
From: Stefano Stabellini @ 2016-01-06 11:08 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, stefano.stabellini
On Wed, 6 Jan 2016, Cao jin wrote:
> v3 changelog:
> 1. use following style when we want to check the returned error
>
> Error *err = NULL;
> foo(arg, &err);
> if (err) {
> handle the error...
> error_propagate(errp, err);
> }
>
> Cao jin (4):
> Add Error **errp for xen_host_pci_device_get()
> Add Error **errp for xen_pt_setup_vga()
> Add Error **errp for xen_pt_config_init()
> Xen PCI passthru: convert to realize()
>
> hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
> hw/xen/xen-host-pci-device.h | 5 +-
> hw/xen/xen_pt.c | 73 ++++++++++++++++-------------
> hw/xen/xen_pt.h | 5 +-
> hw/xen/xen_pt_config_init.c | 51 +++++++++++----------
> hw/xen/xen_pt_graphics.c | 11 +++--
> 6 files changed, 141 insertions(+), 110 deletions(-)
Thanks Cao, I applied the whole series to my next branch.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get()
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
@ 2016-01-06 15:51 ` Eric Blake
2016-01-07 3:13 ` Cao jin
0 siblings, 1 reply; 20+ messages in thread
From: Eric Blake @ 2016-01-06 15:51 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: stefano.stabellini
[-- Attachment #1: Type: text/plain, Size: 5495 bytes --]
On 01/05/2016 07:39 PM, Cao jin wrote:
> To catch the error msg. Also modify the caller
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
> hw/xen/xen-host-pci-device.h | 5 +-
> hw/xen/xen_pt.c | 12 +++--
> 3 files changed, 71 insertions(+), 52 deletions(-)
>
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 7d8a023..952cae0 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>
> /* This size should be enough to read the first 7 lines of a resource file */
> #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
> -static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> +static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
> {
> int i, rc, fd;
> char path[PATH_MAX];
> @@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>
> rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
Are you sure that errno is relevant? And "snprintf err" doesn't seem to
be the correct message, as there is no snprintf in the line above.
> + return;
> }
> +
> fd = open(path, O_RDONLY);
> if (fd == -1) {
> - XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> - return -errno;
> + error_setg_errno(errp, errno, "open %s err", path);
Please use error_setg_file_open() for reporting open() failures.
> @@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> d->rom.bus_flags = flags & IORESOURCE_BITS;
> }
> }
> +
> if (i != PCI_NUM_REGIONS) {
> /* Invalid format or input to short */
> - rc = -ENODEV;
> + error_setg(errp, "Invalid format or input to short: %s", buf);
s/to/too/ (and might as well fix the same typo in the comment while at it)
> @@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>
> rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
> + return;
> }
> +
> fd = open(path, O_RDONLY);
> if (fd == -1) {
> - XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> - return -errno;
> + error_setg_errno(errp, errno, "open %s err", path);
Same comments as above.
> + return;
> }
> +
> do {
> rc = read(fd, &buf, sizeof (buf) - 1);
> if (rc < 0 && errno != EINTR) {
> - rc = -errno;
> + error_setg_errno(errp, errno, "read err");
> goto out;
> }
> } while (rc < 0);
> +
> buf[rc] = 0;
> value = strtol(buf, &endptr, base);
> if (endptr == buf || *endptr != '\n') {
> - rc = -1;
> + error_setg(errp, "format invalid: %s", buf);
> } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> - rc = -errno;
> + error_setg_errno(errp, errno, "strtol err");
This is pre-existing invalid use of strtol (the value of errno is not
guaranteed to be ERANGE on overflow; and the only correct way to safely
check errno after strtol() is to first prime it to 0 prior to calling
strtol). Better would be to use qemu_strtol() (preferably as a separate
patch), so that you don't even have to worry about using strtol()
incorrectly.
> +static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
> {
> char path[PATH_MAX];
> int rc;
>
> rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
May want to delete the space before ( while touching the code in this
vicinity.
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
Another suspect message.
> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> + uint8_t bus, uint8_t dev, uint8_t func,
> + Error **errp)
> {
> unsigned int v;
> - int rc = 0;
> + Error *local_err = NULL;
These days, naming the local variable 'err' is more common than 'local_err'.
> @@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
> s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
> s->dev.devfn);
>
> - rc = xen_host_pci_device_get(&s->real_device,
> - s->hostaddr.domain, s->hostaddr.bus,
> - s->hostaddr.slot, s->hostaddr.function);
> - if (rc) {
> - XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
> + xen_host_pci_device_get(&s->real_device,
> + s->hostaddr.domain, s->hostaddr.bus,
> + s->hostaddr.slot, s->hostaddr.function,
> + &local_err);
> + if (local_err) {
> + XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
Leaks local_err.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] Convert to realize()
2016-01-06 11:08 ` [Qemu-devel] [PATCH v3 0/4] Convert " Stefano Stabellini
@ 2016-01-06 15:51 ` Eric Blake
2016-01-06 16:02 ` Stefano Stabellini
0 siblings, 1 reply; 20+ messages in thread
From: Eric Blake @ 2016-01-06 15:51 UTC (permalink / raw)
To: Stefano Stabellini, Cao jin; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]
On 01/06/2016 04:08 AM, Stefano Stabellini wrote:
> On Wed, 6 Jan 2016, Cao jin wrote:
>> v3 changelog:
>> 1. use following style when we want to check the returned error
>>
>> Error *err = NULL;
>> foo(arg, &err);
>> if (err) {
>> handle the error...
>> error_propagate(errp, err);
>> }
>>
>> Cao jin (4):
>> Add Error **errp for xen_host_pci_device_get()
>> Add Error **errp for xen_pt_setup_vga()
>> Add Error **errp for xen_pt_config_init()
>> Xen PCI passthru: convert to realize()
>>
>> hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
>> hw/xen/xen-host-pci-device.h | 5 +-
>> hw/xen/xen_pt.c | 73 ++++++++++++++++-------------
>> hw/xen/xen_pt.h | 5 +-
>> hw/xen/xen_pt_config_init.c | 51 +++++++++++----------
>> hw/xen/xen_pt_graphics.c | 11 +++--
>> 6 files changed, 141 insertions(+), 110 deletions(-)
>
> Thanks Cao, I applied the whole series to my next branch.
I found some issues while reviewing; maybe you want to wait for a v4.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] Add Error **errp for xen_pt_setup_vga()
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 2/4] Add Error **errp for xen_pt_setup_vga() Cao jin
@ 2016-01-06 15:53 ` Eric Blake
2016-01-07 3:26 ` Cao jin
0 siblings, 1 reply; 20+ messages in thread
From: Eric Blake @ 2016-01-06 15:53 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: stefano.stabellini
[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]
On 01/05/2016 07:39 PM, Cao jin wrote:
> To catch the error msg. Also modify the caller
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> hw/xen/xen_pt.c | 5 ++++-
> hw/xen/xen_pt.h | 3 ++-
> hw/xen/xen_pt_graphics.c | 11 ++++++-----
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 1bd4109..fbce55c 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -807,7 +807,10 @@ static int xen_pt_initfn(PCIDevice *d)
> return -1;
> }
>
> - if (xen_pt_setup_vga(s, &s->real_device) < 0) {
> + xen_pt_setup_vga(s, &s->real_device, &local_err);
> + if (local_err) {
> + error_append_hint(&local_err, "Setup VGA BIOS of passthrough"
> + " GFX failed!");
Please no '!' in error messages. We aren't shouting at the user.
> XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
Do we still need the XEN_PT_ERR() alongside setting the local error?
> xen_host_pci_device_put(&s->real_device);
> return -1;
This leaks local_err.
> @@ -172,13 +173,14 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
> struct pci_data *pd = NULL;
>
> if (!is_igd_vga_passthrough(dev)) {
> - return -1;
> + error_setg(errp, "Need to enable igd-passthrough");
> + return;
> }
>
> bios = get_vgabios(s, &bios_size, dev);
> if (!bios) {
> - XEN_PT_ERR(&s->dev, "VGA: Can't getting VBIOS!\n");
> - return -1;
> + error_setg(errp, "VGA: Can't getting VBIOS!");
> + return;
Please drop the trailing '!' while touching this
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] Convert to realize()
2016-01-06 15:51 ` Eric Blake
@ 2016-01-06 16:02 ` Stefano Stabellini
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Stabellini @ 2016-01-06 16:02 UTC (permalink / raw)
To: Eric Blake; +Cc: Cao jin, qemu-devel, Stefano Stabellini
On Wed, 6 Jan 2016, Eric Blake wrote:
> On 01/06/2016 04:08 AM, Stefano Stabellini wrote:
> > On Wed, 6 Jan 2016, Cao jin wrote:
> >> v3 changelog:
> >> 1. use following style when we want to check the returned error
> >>
> >> Error *err = NULL;
> >> foo(arg, &err);
> >> if (err) {
> >> handle the error...
> >> error_propagate(errp, err);
> >> }
> >>
> >> Cao jin (4):
> >> Add Error **errp for xen_host_pci_device_get()
> >> Add Error **errp for xen_pt_setup_vga()
> >> Add Error **errp for xen_pt_config_init()
> >> Xen PCI passthru: convert to realize()
> >>
> >> hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
> >> hw/xen/xen-host-pci-device.h | 5 +-
> >> hw/xen/xen_pt.c | 73 ++++++++++++++++-------------
> >> hw/xen/xen_pt.h | 5 +-
> >> hw/xen/xen_pt_config_init.c | 51 +++++++++++----------
> >> hw/xen/xen_pt_graphics.c | 11 +++--
> >> 6 files changed, 141 insertions(+), 110 deletions(-)
> >
> > Thanks Cao, I applied the whole series to my next branch.
>
> I found some issues while reviewing; maybe you want to wait for a v4.
Sure, thanks for reviewing.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init()
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init() Cao jin
@ 2016-01-06 16:02 ` Eric Blake
2016-01-07 8:12 ` Cao jin
0 siblings, 1 reply; 20+ messages in thread
From: Eric Blake @ 2016-01-06 16:02 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: stefano.stabellini
[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]
On 01/05/2016 07:39 PM, Cao jin wrote:
> To catch the error msg. Also modify the caller
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/xen/xen_pt.c | 7 ++++---
> hw/xen/xen_pt.h | 2 +-
> hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++---------------------
> 3 files changed, 32 insertions(+), 28 deletions(-)
>
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1899,8 +1899,9 @@ static uint8_t find_cap_offset(XenPCIPassthroughState *s, uint8_t cap)
> return 0;
> }
>
> -static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> - XenPTRegGroup *reg_grp, XenPTRegInfo *reg)
> +static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
> + XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
> + Error **errp)
Indentation is now off.
> @@ -1967,10 +1970,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> val = data;
>
> if (val & ~size_mask) {
> - XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
> - offset, val, reg->size);
> + error_setg(errp, "Offset 0x%04x:0x%04x expands past"
> + " register size(%d)!", offset, val, reg->size);
Drop the trailing !. Also, while touching this, it's better to have a
space before ( in English.
> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
> {
> int i, rc;
> + Error *local_err = NULL;
Same comments as earlier in the series about using the shorter 'err'
instead of 'local_err'.
>
> QLIST_INIT(&s->reg_grps);
>
> @@ -2039,11 +2041,12 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
> reg_grp_offset,
> ®_grp_entry->size);
> if (rc < 0) {
> - XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
> - i, ARRAY_SIZE(xen_pt_emu_reg_grps),
> + error_setg(&local_err, "Failed to initialize %d/%ld, type=0x%x,"
> + " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
compile on 32-bit platforms (where size_t is not necessarily long). Fix
it to %zd while touching it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize()
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize() Cao jin
@ 2016-01-06 16:07 ` Eric Blake
2016-01-07 7:16 ` Cao jin
0 siblings, 1 reply; 20+ messages in thread
From: Eric Blake @ 2016-01-06 16:07 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: stefano.stabellini
[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]
On 01/05/2016 07:39 PM, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> hw/xen/xen_pt.c | 53 ++++++++++++++++++++++++++++-------------------------
> 1 file changed, 28 insertions(+), 25 deletions(-)
>
> @@ -801,19 +801,19 @@ static int xen_pt_initfn(PCIDevice *d)
> if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
> (s->real_device.dev == 2) && (s->real_device.func == 0)) {
> if (!is_igd_vga_passthrough(&s->real_device)) {
> - XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
> - " to passthrough IGD GFX.\n");
> + error_setg(errp, "Need to enable igd-passthru if you're trying"
> + " to passthrough IGD GFX.");
No trailing '.' in error_setg() messages.
> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
> xen_pt_config_init(s, &local_err);
> if (local_err) {
> error_append_hint(&local_err, "PCI Config space initialisation failed");
> - rc = -1;
> + error_propagate(errp, local_err);
> goto err_out;
> }
Looks like this fixes a memory leak in an earlier patch; maybe you need
to shuffle hunks around?
>
> /* Bind interrupt */
> rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
> if (rc) {
> - XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
> + error_setg_errno(errp, errno, "Failed to read PCI_INTERRUPT_PIN!");
No trailing '!'
> @@ -891,14 +890,14 @@ out:
>
> rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
> if (rc) {
> - XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
> + error_setg_errno(errp, errno, "Failed to read PCI_COMMAND!");
and again
> @@ -911,12 +910,16 @@ out:
> "Real physical device %02x:%02x.%d registered successfully!\n",
> s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
>
> - return 0;
> + return;
>
> err_out:
> + for (i = 0; i < PCI_ROM_SLOT; i++) {
> + object_unparent(OBJECT(&s->bar[i]));
> + }
> + object_unparent(OBJECT(&s->rom));
> +
> xen_pt_destroy(d);
> assert(rc);
> - return rc;
Is the assertion still needed?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get()
2016-01-06 15:51 ` Eric Blake
@ 2016-01-07 3:13 ` Cao jin
2016-01-07 16:47 ` Eric Blake
0 siblings, 1 reply; 20+ messages in thread
From: Cao jin @ 2016-01-07 3:13 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: stefano.stabellini
On 01/06/2016 11:51 PM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
>> To catch the error msg. Also modify the caller
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> hw/xen/xen-host-pci-device.c | 106 +++++++++++++++++++++++++------------------
>> hw/xen/xen-host-pci-device.h | 5 +-
>> hw/xen/xen_pt.c | 12 +++--
>> 3 files changed, 71 insertions(+), 52 deletions(-)
>>
>> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>> index 7d8a023..952cae0 100644
>> --- a/hw/xen/xen-host-pci-device.c
>> +++ b/hw/xen/xen-host-pci-device.c
>> @@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>>
>> /* This size should be enough to read the first 7 lines of a resource file */
>> #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
>> -static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>> +static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
>> {
>> int i, rc, fd;
>> char path[PATH_MAX];
>> @@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>>
>> rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
>> if (rc) {
>> - return rc;
>> + error_setg_errno(errp, errno, "snprintf err");
>
> Are you sure that errno is relevant? And "snprintf err" doesn't seem to
> be the correct message, as there is no snprintf in the line above.
>
snprintf() is called in xen_host_pci_sysfs_path() above, and is the only
possible error source, so I guess the errno is relevant?
Or, replace the error_setg_errno() to assert(0)? because if snprintf
goes wrong, user seems can do nothing.
>> + return;
>> }
>> +
>> fd = open(path, O_RDONLY);
>> if (fd == -1) {
>> - XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
>> - return -errno;
>> + error_setg_errno(errp, errno, "open %s err", path);
>
> Please use error_setg_file_open() for reporting open() failures.
>
>> @@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>> d->rom.bus_flags = flags & IORESOURCE_BITS;
>> }
>> }
>> +
>> if (i != PCI_NUM_REGIONS) {
>> /* Invalid format or input to short */
>> - rc = -ENODEV;
>> + error_setg(errp, "Invalid format or input to short: %s", buf);
>
> s/to/too/ (and might as well fix the same typo in the comment while at it)
>
>
>> @@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>>
>> rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
>> if (rc) {
>> - return rc;
>> + error_setg_errno(errp, errno, "snprintf err");
>> + return;
>> }
>> +
>> fd = open(path, O_RDONLY);
>> if (fd == -1) {
>> - XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
>> - return -errno;
>> + error_setg_errno(errp, errno, "open %s err", path);
>
> Same comments as above.
>
>> + return;
>> }
>> +
>> do {
>> rc = read(fd, &buf, sizeof (buf) - 1);
>> if (rc < 0 && errno != EINTR) {
>> - rc = -errno;
>> + error_setg_errno(errp, errno, "read err");
>> goto out;
>> }
>> } while (rc < 0);
>> +
>> buf[rc] = 0;
>> value = strtol(buf, &endptr, base);
>> if (endptr == buf || *endptr != '\n') {
>> - rc = -1;
>> + error_setg(errp, "format invalid: %s", buf);
>> } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
>> - rc = -errno;
>> + error_setg_errno(errp, errno, "strtol err");
>
> This is pre-existing invalid use of strtol (the value of errno is not
> guaranteed to be ERANGE on overflow; and the only correct way to safely
> check errno after strtol() is to first prime it to 0 prior to calling
> strtol). Better would be to use qemu_strtol() (preferably as a separate
> patch), so that you don't even have to worry about using strtol()
> incorrectly.
>
Ok, will split it into a separate patch
>> +static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
>> {
>> char path[PATH_MAX];
>> int rc;
>>
>> rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
>
> May want to delete the space before ( while touching the code in this
> vicinity.
>
>> if (rc) {
>> - return rc;
>> + error_setg_errno(errp, errno, "snprintf err");
>
> Another suspect message.
>
>
>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> + uint8_t bus, uint8_t dev, uint8_t func,
>> + Error **errp)
>> {
>> unsigned int v;
>> - int rc = 0;
>> + Error *local_err = NULL;
>
> These days, naming the local variable 'err' is more common than 'local_err'.
>
agree. I guess maybe "local_err" is a better mnemonic for newbie. and
when I am gradually familiar with error report, I also prefer "err".
Actually I considered to change this name, I just don`t want to bother
the reviewer to review it again, especially when the patch got a
Review-by and new version just changes some names. Will fix it.
>> @@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
>> s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
>> s->dev.devfn);
>>
>> - rc = xen_host_pci_device_get(&s->real_device,
>> - s->hostaddr.domain, s->hostaddr.bus,
>> - s->hostaddr.slot, s->hostaddr.function);
>> - if (rc) {
>> - XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
>> + xen_host_pci_device_get(&s->real_device,
>> + s->hostaddr.domain, s->hostaddr.bus,
>> + s->hostaddr.slot, s->hostaddr.function,
>> + &local_err);
>> + if (local_err) {
>> + XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
>
> Leaks local_err.
>
Yes, but this will be fixed with error_propagate when patch "4/4 convert
to realize" comes, so is it ok to let it be here?
I originally want to make these patch independent from each other with
the most necessary modification.
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] Add Error **errp for xen_pt_setup_vga()
2016-01-06 15:53 ` Eric Blake
@ 2016-01-07 3:26 ` Cao jin
0 siblings, 0 replies; 20+ messages in thread
From: Cao jin @ 2016-01-07 3:26 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: stefano.stabellini
On 01/06/2016 11:53 PM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>
> Please no '!' in error messages. We aren't shouting at the user.
Interesting, I didn`t think of this kind of questions before. will fix
>
>> XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
>
> Do we still need the XEN_PT_ERR() alongside setting the local error?
>
this is fixed in "4/4 convert to realize" patch, as I said in last mail,
I try to make every patch as small as possible
>> xen_host_pci_device_put(&s->real_device);
>> return -1;
>
> This leaks local_err.
>
>
[...]
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize()
2016-01-06 16:07 ` Eric Blake
@ 2016-01-07 7:16 ` Cao jin
2016-01-07 16:53 ` Eric Blake
0 siblings, 1 reply; 20+ messages in thread
From: Cao jin @ 2016-01-07 7:16 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: stefano.stabellini
On 01/07/2016 12:07 AM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>
>> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>> xen_pt_config_init(s, &local_err);
>> if (local_err) {
>> error_append_hint(&local_err, "PCI Config space initialisation failed");
>> - rc = -1;
>> + error_propagate(errp, local_err);
>> goto err_out;
>> }
>
> Looks like this fixes a memory leak in an earlier patch; maybe you need
> to shuffle hunks around?
>
Sorry, don`t quite undertand what "shuffle hunks around" means, could
you detail it?
[...]
>>
>> err_out:
>> + for (i = 0; i < PCI_ROM_SLOT; i++) {
>> + object_unparent(OBJECT(&s->bar[i]));
>> + }
>> + object_unparent(OBJECT(&s->rom));
>> +
>> xen_pt_destroy(d);
>> assert(rc);
>> - return rc;
>
> Is the assertion still needed?
>
Actually I think in the original code, the assertion isn`t necessary
too, but I guess it is a kind of defensive coding? You can see, not very
if (rc) equals TRUE will goto err_out. So I prefer not to touch it when
I am not sure about the author`s intent.
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init()
2016-01-06 16:02 ` Eric Blake
@ 2016-01-07 8:12 ` Cao jin
2016-01-07 16:51 ` Eric Blake
0 siblings, 1 reply; 20+ messages in thread
From: Cao jin @ 2016-01-07 8:12 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: stefano.stabellini
On 01/07/2016 12:02 AM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>> +static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
>> + XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
>> + Error **errp)
>
> Indentation is now off.
>
Sharp eyes;)
>> @@ -1967,10 +1970,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
>> val = data;
>>
>> if (val & ~size_mask) {
>> - XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
>> - offset, val, reg->size);
>> + error_setg(errp, "Offset 0x%04x:0x%04x expands past"
>> + " register size(%d)!", offset, val, reg->size);
>
> Drop the trailing !. Also, while touching this, it's better to have a
> space before ( in English.
>
Ok
>
>> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
>> {
>> int i, rc;
>> + Error *local_err = NULL;
>
> Same comments as earlier in the series about using the shorter 'err'
> instead of 'local_err'.
>
>>
>> QLIST_INIT(&s->reg_grps);
>>
>> @@ -2039,11 +2041,12 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
>> reg_grp_offset,
>> ®_grp_entry->size);
>> if (rc < 0) {
>> - XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
>> - i, ARRAY_SIZE(xen_pt_emu_reg_grps),
>> + error_setg(&local_err, "Failed to initialize %d/%ld, type=0x%x,"
>> + " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
>
> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
> compile on 32-bit platforms (where size_t is not necessarily long). Fix
> it to %zd while touching it.
>
a question:
1. Is %zu more suitable for size_t? since size_t is unsigned integer.
and a personal question after digging into size_t:
2. Does the size of size_t always equal to the word length[*] of computer
[*] https://en.wikipedia.org/wiki/Word_%28computer_architecture%29
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get()
2016-01-07 3:13 ` Cao jin
@ 2016-01-07 16:47 ` Eric Blake
0 siblings, 0 replies; 20+ messages in thread
From: Eric Blake @ 2016-01-07 16:47 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: stefano.stabellini
[-- Attachment #1: Type: text/plain, Size: 4076 bytes --]
On 01/06/2016 08:13 PM, Cao jin wrote:
>
>
> On 01/06/2016 11:51 PM, Eric Blake wrote:
>> On 01/05/2016 07:39 PM, Cao jin wrote:
>>> To catch the error msg. Also modify the caller
>>>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>> ---
>>> xen_host_pci_get_resource(XenHostPCIDevice *d)
>>>
>>> rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
>>> if (rc) {
>>> - return rc;
>>> + error_setg_errno(errp, errno, "snprintf err");
>>
>> Are you sure that errno is relevant? And "snprintf err" doesn't seem to
>> be the correct message, as there is no snprintf in the line above.
>>
>
> snprintf() is called in xen_host_pci_sysfs_path() above, and is the only
> possible error source, so I guess the errno is relevant?
Then maybe it's better to pass Error **errp through
xen_host_pci_sysfs_path(), so that the error message is closer to the
real error. Especially since you have multiple callers, all identically
affected (each caller shouldn't have to repeat the same common
error-handling logic, if you can push it down lower in the callstack).
For that matter, the only failure of xen_host_pci_sysfs_path is to
return -ENODEV, but it does NOT set 'errno = ENODEV' on that error path,
so you most likely ARE printing the wrong errno value.
Another option is to at least reword the message to make sense locally,
as in:
if (xen_host_pci_sysfs_path(...)) {
error_setg(errp, "failed to determine device path for %s", name);
return;
}
>
> Or, replace the error_setg_errno() to assert(0)? because if snprintf
> goes wrong, user seems can do nothing.
If you want to assert, then do that in xen_host_pci_sysfs_path(), not in
all callers; at which point xen_host_pci_sysfs_path() should be fixed to
return void.
>>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>>> + uint8_t bus, uint8_t dev, uint8_t func,
>>> + Error **errp)
>>> {
>>> unsigned int v;
>>> - int rc = 0;
>>> + Error *local_err = NULL;
>>
>> These days, naming the local variable 'err' is more common than
>> 'local_err'.
>>
>
> agree. I guess maybe "local_err" is a better mnemonic for newbie. and
> when I am gradually familiar with error report, I also prefer "err".
> Actually I considered to change this name, I just don`t want to bother
> the reviewer to review it again, especially when the patch got a
> Review-by and new version just changes some names. Will fix it.
We don't mind re-reviewing a patch, but it does help if your cover
letter and/or text after the --- divider explains how v4 differs from
v3, to help us focus on the changes.
>>> + xen_host_pci_device_get(&s->real_device,
>>> + s->hostaddr.domain, s->hostaddr.bus,
>>> + s->hostaddr.slot, s->hostaddr.function,
>>> + &local_err);
>>> + if (local_err) {
>>> + XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
>>
>> Leaks local_err.
>>
>
> Yes, but this will be fixed with error_propagate when patch "4/4 convert
> to realize" comes, so is it ok to let it be here?
No. Every patch should be stand-alone correct, or else strongly document
why it is taking a shortcut that a later patch will clean up. At a bare
minimum, if you don't fix the leak here, you should have a FIXME comment
both here and in the commit message; but it's probably easier to just
avoid the leak in the first place.
>
> I originally want to make these patch independent from each other with
> the most necessary modification.
Splitting patches can be an art form. But your comment about the patch
being independent _is_ the reason why the patch must not leak - if a
backporter uses this patch but not the rest of the series, they should
not be introducing a leak in their backport.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init()
2016-01-07 8:12 ` Cao jin
@ 2016-01-07 16:51 ` Eric Blake
2016-01-08 8:41 ` Cao jin
0 siblings, 1 reply; 20+ messages in thread
From: Eric Blake @ 2016-01-07 16:51 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: stefano.stabellini
[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]
On 01/07/2016 01:12 AM, Cao jin wrote:
>>> if (rc < 0) {
>>> - XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld,
>>> type=0x%x, rc:%d\n",
>>> - i, ARRAY_SIZE(xen_pt_emu_reg_grps),
>>> + error_setg(&local_err, "Failed to initialize %d/%ld,
>>> type=0x%x,"
>>> + " rc:%d", i,
>>> ARRAY_SIZE(xen_pt_emu_reg_grps),
>>
>> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
>> compile on 32-bit platforms (where size_t is not necessarily long). Fix
>> it to %zd while touching it.
>>
>
> a question:
> 1. Is %zu more suitable for size_t? since size_t is unsigned integer.
Yes, you're right on that one.
>
> and a personal question after digging into size_t:
> 2. Does the size of size_t always equal to the word length[*] of computer
No. It equals the maximum size the program can use. But the x32 ABI
project is a good example of a 32-bit size_t while still taking full
advantage of the 64-bit word size registers, in the name of memory
efficiencies. See https://en.wikipedia.org/wiki/X32_ABI.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize()
2016-01-07 7:16 ` Cao jin
@ 2016-01-07 16:53 ` Eric Blake
0 siblings, 0 replies; 20+ messages in thread
From: Eric Blake @ 2016-01-07 16:53 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: stefano.stabellini
[-- Attachment #1: Type: text/plain, Size: 931 bytes --]
On 01/07/2016 12:16 AM, Cao jin wrote:
>
>
> On 01/07/2016 12:07 AM, Eric Blake wrote:
>> On 01/05/2016 07:39 PM, Cao jin wrote:
> [...]
>>
>>> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>>> xen_pt_config_init(s, &local_err);
>>> if (local_err) {
>>> error_append_hint(&local_err, "PCI Config space
>>> initialisation failed");
>>> - rc = -1;
>>> + error_propagate(errp, local_err);
>>> goto err_out;
>>> }
>>
>> Looks like this fixes a memory leak in an earlier patch; maybe you need
>> to shuffle hunks around?
>>
>
> Sorry, don`t quite undertand what "shuffle hunks around" means, could
> you detail it?
It means this change should occur in an earlier patch (probably 2/4?).
I shuffle hunks by using 'git rebase -i'.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init()
2016-01-07 16:51 ` Eric Blake
@ 2016-01-08 8:41 ` Cao jin
0 siblings, 0 replies; 20+ messages in thread
From: Cao jin @ 2016-01-08 8:41 UTC (permalink / raw)
To: Eric Blake, qemu-devel
On 01/08/2016 12:51 AM, Eric Blake wrote:
> On 01/07/2016 01:12 AM, Cao jin wrote:
>
>>>> if (rc < 0) {
>>>> - XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld,
>>>> type=0x%x, rc:%d\n",
>>>> - i, ARRAY_SIZE(xen_pt_emu_reg_grps),
>>>> + error_setg(&local_err, "Failed to initialize %d/%ld,
>>>> type=0x%x,"
>>>> + " rc:%d", i,
>>>> ARRAY_SIZE(xen_pt_emu_reg_grps),
>>>
>>> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
>>> compile on 32-bit platforms (where size_t is not necessarily long). Fix
>>> it to %zd while touching it.
>>>
>>
>> a question:
>> 1. Is %zu more suitable for size_t? since size_t is unsigned integer.
>
> Yes, you're right on that one.
>
>>
>> and a personal question after digging into size_t:
>> 2. Does the size of size_t always equal to the word length[*] of computer
>
> No. It equals the maximum size the program can use. But the x32 ABI
> project is a good example of a 32-bit size_t while still taking full
> advantage of the 64-bit word size registers, in the name of memory
> efficiencies. See https://en.wikipedia.org/wiki/X32_ABI.
>
Thanks very much. Have send v4 version, hope I don`t miss any comment:)
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-01-08 8:39 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-06 2:39 [Qemu-devel] [PATCH v3 0/4] Convert to realize() Cao jin
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
2016-01-06 15:51 ` Eric Blake
2016-01-07 3:13 ` Cao jin
2016-01-07 16:47 ` Eric Blake
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 2/4] Add Error **errp for xen_pt_setup_vga() Cao jin
2016-01-06 15:53 ` Eric Blake
2016-01-07 3:26 ` Cao jin
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init() Cao jin
2016-01-06 16:02 ` Eric Blake
2016-01-07 8:12 ` Cao jin
2016-01-07 16:51 ` Eric Blake
2016-01-08 8:41 ` Cao jin
2016-01-06 2:39 ` [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize() Cao jin
2016-01-06 16:07 ` Eric Blake
2016-01-07 7:16 ` Cao jin
2016-01-07 16:53 ` Eric Blake
2016-01-06 11:08 ` [Qemu-devel] [PATCH v3 0/4] Convert " Stefano Stabellini
2016-01-06 15:51 ` Eric Blake
2016-01-06 16:02 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.