* [Qemu-devel] [PATCH v2 0/4] Convert to realize()
@ 2015-12-27 13:40 Cao jin
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Cao jin @ 2015-12-27 13:40 UTC (permalink / raw)
To: qemu-devel; +Cc: stefano.stabellini
v2 changelog:
1. separate every supporting function into a single patch
2. Change supporting function type to void, for consistency
3. other fixes as per Stefano`s review
@Stefano: the last patch build failure you feedback:
qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’:
qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used uninitialized in this func
I admit it is careless misktake(fixed), but weird thing is, I confirmed
again, it indeed can compile in my environment, the evidence is:
1. every time I make after touching xen_pt_config_init.c, I can see the log:
CC x86_64-softmmu/hw/xen/xen_pt_config_init.o
LINK x86_64-softmmu/qemu-system-x86_64
2. $ readelf -s qemu-system-x86_64 | grep xen_pt_config_init , can got the
symbol
Will the different compiler make the difference?(I am using
gcc (GCC) 4.9.2 20150212 (Red Hat 4.9.2-6))
Cao jin (4):
Add Error **errp for xen_host_pci_device_get()
Add Error **errp for xen_pt_setup_vga()
Add Error **errp for xen_pt_config_init()
Xen PCI passthru: convert to realize()
hw/xen/xen-host-pci-device.c | 102 ++++++++++++++++++++++++-------------------
hw/xen/xen-host-pci-device.h | 5 ++-
hw/xen/xen_pt.c | 73 +++++++++++++++++--------------
hw/xen/xen_pt.h | 5 ++-
hw/xen/xen_pt_config_init.c | 48 ++++++++++----------
hw/xen/xen_pt_graphics.c | 11 ++---
6 files changed, 134 insertions(+), 110 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get()
2015-12-27 13:40 [Qemu-devel] [PATCH v2 0/4] Convert to realize() Cao jin
@ 2015-12-27 13:40 ` Cao jin
2016-01-04 15:15 ` Stefano Stabellini
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 2/4] Add Error **errp for xen_pt_setup_vga() Cao jin
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Cao jin @ 2015-12-27 13:40 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 | 102 ++++++++++++++++++++++++-------------------
hw/xen/xen-host-pci-device.h | 5 ++-
hw/xen/xen_pt.c | 12 ++---
3 files changed, 67 insertions(+), 52 deletions(-)
diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 7d8a023..3d22095 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
/* This size should be enough to read the first 7 lines of a resource file */
#define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
-static int xen_host_pci_get_resource(XenHostPCIDevice *d)
+static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
{
int i, rc, fd;
char path[PATH_MAX];
@@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
if (rc) {
- return rc;
+ error_setg_errno(errp, errno, "snprintf err");
+ return;
}
+
fd = open(path, O_RDONLY);
if (fd == -1) {
- XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
- return -errno;
+ error_setg_errno(errp, errno, "open %s err", path);
+ return;
}
do {
rc = read(fd, &buf, sizeof (buf) - 1);
if (rc < 0 && errno != EINTR) {
- rc = -errno;
+ error_setg_errno(errp, errno, "read err");
goto out;
}
} while (rc < 0);
buf[rc] = 0;
- rc = 0;
s = buf;
for (i = 0; i < PCI_NUM_REGIONS; i++) {
@@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
d->rom.bus_flags = flags & IORESOURCE_BITS;
}
}
+
if (i != PCI_NUM_REGIONS) {
/* Invalid format or input to short */
- rc = -ENODEV;
+ error_setg(errp, "Invalid format or input to short: %s", buf);
}
out:
close(fd);
- return rc;
}
/* This size should be enough to read a long from a file */
#define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
-static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
- unsigned int *pvalue, int base)
+static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
+ unsigned int *pvalue, int base, Error **errp)
{
char path[PATH_MAX];
char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
@@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
if (rc) {
- return rc;
+ error_setg_errno(errp, errno, "snprintf err");
+ return;
}
+
fd = open(path, O_RDONLY);
if (fd == -1) {
- XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
- return -errno;
+ error_setg_errno(errp, errno, "open %s err", path);
+ return;
}
+
do {
rc = read(fd, &buf, sizeof (buf) - 1);
if (rc < 0 && errno != EINTR) {
- rc = -errno;
+ error_setg_errno(errp, errno, "read err");
goto out;
}
} while (rc < 0);
+
buf[rc] = 0;
value = strtol(buf, &endptr, base);
if (endptr == buf || *endptr != '\n') {
- rc = -1;
+ error_setg(errp, "format invalid: %s", buf);
} else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
- rc = -errno;
+ error_setg_errno(errp, errno, "strtol err");
} else {
- rc = 0;
*pvalue = value;
}
+
out:
close(fd);
- return rc;
}
-static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
+static inline void xen_host_pci_get_hex_value(XenHostPCIDevice *d,
const char *name,
- unsigned int *pvalue)
+ unsigned int *pvalue,
+ Error **errp)
{
- return xen_host_pci_get_value(d, name, pvalue, 16);
+ xen_host_pci_get_value(d, name, pvalue, 16, errp);
}
-static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
+static inline void xen_host_pci_get_dec_value(XenHostPCIDevice *d,
const char *name,
- unsigned int *pvalue)
+ unsigned int *pvalue,
+ Error **errp)
{
- return xen_host_pci_get_value(d, name, pvalue, 10);
+ xen_host_pci_get_value(d, name, pvalue, 10, errp);
}
static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
@@ -206,20 +212,21 @@ static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
return !stat(path, &buf);
}
-static int xen_host_pci_config_open(XenHostPCIDevice *d)
+static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
{
char path[PATH_MAX];
int rc;
rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
if (rc) {
- return rc;
+ error_setg_errno(errp, errno, "snprintf err");
+ return;
}
+
d->config_fd = open(path, O_RDWR);
if (d->config_fd < 0) {
- return -errno;
+ error_setg_errno(errp, errno, "open %s err", path);
}
- return 0;
}
static int xen_host_pci_config_read(XenHostPCIDevice *d,
@@ -341,11 +348,11 @@ int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
return -1;
}
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
- uint8_t bus, uint8_t dev, uint8_t func)
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+ uint8_t bus, uint8_t dev, uint8_t func,
+ Error **errp)
{
unsigned int v;
- int rc = 0;
d->config_fd = -1;
d->domain = domain;
@@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
d->dev = dev;
d->func = func;
- rc = xen_host_pci_config_open(d);
- if (rc) {
+ xen_host_pci_config_open(d, errp);
+ if (*errp) {
goto error;
}
- rc = xen_host_pci_get_resource(d);
- if (rc) {
+
+ xen_host_pci_get_resource(d, errp);
+ if (*errp) {
goto error;
}
- rc = xen_host_pci_get_hex_value(d, "vendor", &v);
- if (rc) {
+
+ xen_host_pci_get_hex_value(d, "vendor", &v, errp);
+ if (*errp) {
goto error;
}
d->vendor_id = v;
- rc = xen_host_pci_get_hex_value(d, "device", &v);
- if (rc) {
+
+ xen_host_pci_get_hex_value(d, "device", &v, errp);
+ if (*errp) {
goto error;
}
d->device_id = v;
- rc = xen_host_pci_get_dec_value(d, "irq", &v);
- if (rc) {
+
+ xen_host_pci_get_dec_value(d, "irq", &v, errp);
+ if (*errp) {
goto error;
}
d->irq = v;
- rc = xen_host_pci_get_hex_value(d, "class", &v);
- if (rc) {
+
+ xen_host_pci_get_hex_value(d, "class", &v, errp);
+ if (*errp) {
goto error;
}
d->class_code = v;
+
d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
- return 0;
+ return;
error:
if (d->config_fd >= 0) {
close(d->config_fd);
d->config_fd = -1;
}
- return rc;
}
bool xen_host_pci_device_closed(XenHostPCIDevice *d)
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index 3d44e04..42b9138 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -36,8 +36,9 @@ typedef struct XenHostPCIDevice {
int config_fd;
} XenHostPCIDevice;
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
- uint8_t bus, uint8_t dev, uint8_t func);
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+ uint8_t bus, uint8_t dev, uint8_t func,
+ Error **errp);
void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
bool xen_host_pci_device_closed(XenHostPCIDevice *d);
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index aa96288..1bd4109 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -767,6 +767,7 @@ static int xen_pt_initfn(PCIDevice *d)
uint8_t machine_irq = 0, scratch;
uint16_t cmd = 0;
int pirq = XEN_PT_UNASSIGNED_PIRQ;
+ Error *local_err = NULL;
/* register real device */
XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
@@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
s->dev.devfn);
- rc = xen_host_pci_device_get(&s->real_device,
- s->hostaddr.domain, s->hostaddr.bus,
- s->hostaddr.slot, s->hostaddr.function);
- if (rc) {
- XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
+ xen_host_pci_device_get(&s->real_device,
+ s->hostaddr.domain, s->hostaddr.bus,
+ s->hostaddr.slot, s->hostaddr.function,
+ &local_err);
+ if (local_err) {
+ XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
return -1;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] Add Error **errp for xen_pt_setup_vga()
2015-12-27 13:40 [Qemu-devel] [PATCH v2 0/4] Convert to realize() Cao jin
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
@ 2015-12-27 13:40 ` Cao jin
2016-01-04 15:18 ` Stefano Stabellini
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 3/4] Add Error **errp for xen_pt_config_init() Cao jin
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 4/4] Xen PCI passthru: convert to realize() Cao jin
3 siblings, 1 reply; 13+ messages in thread
From: Cao jin @ 2015-12-27 13:40 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 | 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] 13+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] Add Error **errp for xen_pt_config_init()
2015-12-27 13:40 [Qemu-devel] [PATCH v2 0/4] Convert to realize() Cao jin
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 2/4] Add Error **errp for xen_pt_setup_vga() Cao jin
@ 2015-12-27 13:40 ` Cao jin
2016-01-04 15:23 ` Stefano Stabellini
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 4/4] Xen PCI passthru: convert to realize() Cao jin
3 siblings, 1 reply; 13+ messages in thread
From: Cao jin @ 2015-12-27 13:40 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 | 48 ++++++++++++++++++++++-----------------------
3 files changed, 29 insertions(+), 28 deletions(-)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index fbce55c..3787c26 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -824,9 +824,10 @@ static int xen_pt_initfn(PCIDevice *d)
xen_pt_register_regions(s, &cmd);
/* reinitialize each config register to be emulated */
- rc = xen_pt_config_init(s);
- if (rc) {
- XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
+ xen_pt_config_init(s, &local_err);
+ if (local_err) {
+ error_append_hint(&local_err, "PCI Config space initialisation failed");
+ rc = -1;
goto err_out;
}
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index dc74d3e..466525f 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -228,7 +228,7 @@ struct XenPCIPassthroughState {
bool listener_set;
};
-int xen_pt_config_init(XenPCIPassthroughState *s);
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp);
void xen_pt_config_delete(XenPCIPassthroughState *s);
XenPTRegGroup *xen_pt_find_reg_grp(XenPCIPassthroughState *s, uint32_t address);
XenPTReg *xen_pt_find_reg(XenPTRegGroup *reg_grp, uint32_t address);
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 3d8686d..096f204 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1899,8 +1899,9 @@ static uint8_t find_cap_offset(XenPCIPassthroughState *s, uint8_t cap)
return 0;
}
-static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
- XenPTRegGroup *reg_grp, XenPTRegInfo *reg)
+static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
+ XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
+ Error **errp)
{
XenPTReg *reg_entry;
uint32_t data = 0;
@@ -1919,12 +1920,13 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
reg_grp->base_offset + reg->offset, &data);
if (rc < 0) {
g_free(reg_entry);
- return rc;
+ error_setg(errp, "Init emulate register fail");
+ return;
}
if (data == XEN_PT_INVALID_REG) {
/* free unused BAR register entry */
g_free(reg_entry);
- return 0;
+ return;
}
/* Sync up the data to dev.config */
offset = reg_grp->base_offset + reg->offset;
@@ -1942,7 +1944,8 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
if (rc) {
/* Serious issues when we cannot read the host values! */
g_free(reg_entry);
- return rc;
+ error_setg(errp, "Cannot read host values");
+ return;
}
/* Set bits in emu_mask are the ones we emulate. The dev.config shall
* contain the emulated view of the guest - therefore we flip the mask
@@ -1967,10 +1970,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
val = data;
if (val & ~size_mask) {
- XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
- offset, val, reg->size);
+ error_setg(errp, "Offset 0x%04x:0x%04x expands past"
+ " register size(%d)!", offset, val, reg->size);
g_free(reg_entry);
- return -ENXIO;
+ return;
}
/* This could be just pci_set_long as we don't modify the bits
* past reg->size, but in case this routine is run in parallel or the
@@ -1990,11 +1993,9 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
}
/* list add register entry */
QLIST_INSERT_HEAD(®_grp->reg_tbl_list, reg_entry, entries);
-
- return 0;
}
-int xen_pt_config_init(XenPCIPassthroughState *s)
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
{
int i, rc;
@@ -2039,11 +2040,11 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
reg_grp_offset,
®_grp_entry->size);
if (rc < 0) {
- XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
- i, ARRAY_SIZE(xen_pt_emu_reg_grps),
+ error_setg(errp, "Failed to initialize %d/%ld, type=0x%x,"
+ " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
xen_pt_emu_reg_grps[i].grp_type, rc);
xen_pt_config_delete(s);
- return rc;
+ return;
}
}
@@ -2051,24 +2052,23 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
if (xen_pt_emu_reg_grps[i].emu_regs) {
int j = 0;
XenPTRegInfo *regs = xen_pt_emu_reg_grps[i].emu_regs;
+
/* initialize capability register */
for (j = 0; regs->size != 0; j++, regs++) {
- /* initialize capability register */
- rc = xen_pt_config_reg_init(s, reg_grp_entry, regs);
- if (rc < 0) {
- XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
- j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
- regs->offset, xen_pt_emu_reg_grps[i].grp_type,
- i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
+ xen_pt_config_reg_init(s, reg_grp_entry, regs, errp);
+ if (*errp) {
+ error_append_hint(errp, "Failed to initialize %d/%ld"
+ " reg 0x%x in grp_type=0x%x (%d/%ld)",
+ j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
+ regs->offset, xen_pt_emu_reg_grps[i].grp_type,
+ i, ARRAY_SIZE(xen_pt_emu_reg_grps));
xen_pt_config_delete(s);
- return rc;
+ return;
}
}
}
}
}
-
- return 0;
}
/* delete all emulate register */
--
2.1.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] Xen PCI passthru: convert to realize()
2015-12-27 13:40 [Qemu-devel] [PATCH v2 0/4] Convert to realize() Cao jin
` (2 preceding siblings ...)
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 3/4] Add Error **errp for xen_pt_config_init() Cao jin
@ 2015-12-27 13:40 ` Cao jin
2016-01-04 15:29 ` Stefano Stabellini
3 siblings, 1 reply; 13+ messages in thread
From: Cao jin @ 2015-12-27 13:40 UTC (permalink / raw)
To: qemu-devel; +Cc: stefano.stabellini
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/xen/xen_pt.c | 53 ++++++++++++++++++++++++++++-------------------------
1 file changed, 28 insertions(+), 25 deletions(-)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 3787c26..b058f61 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -760,10 +760,10 @@ static void xen_pt_destroy(PCIDevice *d) {
}
/* init */
-static int xen_pt_initfn(PCIDevice *d)
+static void xen_pt_realize(PCIDevice *d, Error **errp)
{
XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
- int rc = 0;
+ int i, rc = 0;
uint8_t machine_irq = 0, scratch;
uint16_t cmd = 0;
int pirq = XEN_PT_UNASSIGNED_PIRQ;
@@ -780,8 +780,8 @@ static int xen_pt_initfn(PCIDevice *d)
s->hostaddr.slot, s->hostaddr.function,
&local_err);
if (local_err) {
- XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
- return -1;
+ error_propagate(errp, local_err);
+ return;
}
s->is_virtfn = s->real_device.is_virtfn;
@@ -801,19 +801,19 @@ static int xen_pt_initfn(PCIDevice *d)
if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
(s->real_device.dev == 2) && (s->real_device.func == 0)) {
if (!is_igd_vga_passthrough(&s->real_device)) {
- XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
- " to passthrough IGD GFX.\n");
+ error_setg(errp, "Need to enable igd-passthru if you're trying"
+ " to passthrough IGD GFX.");
xen_host_pci_device_put(&s->real_device);
- return -1;
+ return;
}
xen_pt_setup_vga(s, &s->real_device, &local_err);
if (local_err) {
error_append_hint(&local_err, "Setup VGA BIOS of passthrough"
" GFX failed!");
- XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+ error_propagate(errp, local_err);
xen_host_pci_device_put(&s->real_device);
- return -1;
+ return;
}
/* Register ISA bridge for passthrough GFX. */
@@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
xen_pt_config_init(s, &local_err);
if (local_err) {
error_append_hint(&local_err, "PCI Config space initialisation failed");
- rc = -1;
+ error_propagate(errp, local_err);
goto err_out;
}
/* Bind interrupt */
rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
if (rc) {
- XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
+ error_setg_errno(errp, errno, "Failed to read PCI_INTERRUPT_PIN!");
goto err_out;
}
if (!scratch) {
- XEN_PT_LOG(d, "no pin interrupt\n");
+ error_setg(errp, "no pin interrupt");
goto out;
}
machine_irq = s->real_device.irq;
rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
-
if (rc < 0) {
- XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
- machine_irq, pirq, errno);
+ error_setg_errno(errp, errno, "Mapping machine irq %u to"
+ " pirq %i failed", machine_irq, pirq);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -868,8 +867,8 @@ static int xen_pt_initfn(PCIDevice *d)
PCI_SLOT(d->devfn),
e_intx);
if (rc < 0) {
- XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
- e_intx, errno);
+ error_setg_errno(errp, errno, "Binding of interrupt %i failed!",
+ e_intx);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -877,8 +876,8 @@ static int xen_pt_initfn(PCIDevice *d)
if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
- XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
- " (err: %d)\n", machine_irq, errno);
+ error_setg_errno(errp, errno, "Unmapping of machine"
+ " interrupt %i failed!", machine_irq);
}
}
s->machine_irq = 0;
@@ -891,14 +890,14 @@ out:
rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
if (rc) {
- XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
+ error_setg_errno(errp, errno, "Failed to read PCI_COMMAND!");
goto err_out;
} else {
val |= cmd;
rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
if (rc) {
- XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
- val, rc);
+ error_setg_errno(errp, errno, "Failed to write PCI_COMMAND"
+ " val=0x%x!", val);
goto err_out;
}
}
@@ -911,12 +910,16 @@ out:
"Real physical device %02x:%02x.%d registered successfully!\n",
s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
- return 0;
+ return;
err_out:
+ for (i = 0; i < PCI_ROM_SLOT; i++) {
+ object_unparent(OBJECT(&s->bar[i]));
+ }
+ object_unparent(OBJECT(&s->rom));
+
xen_pt_destroy(d);
assert(rc);
- return rc;
}
static void xen_pt_unregister_device(PCIDevice *d)
@@ -935,7 +938,7 @@ static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
- k->init = xen_pt_initfn;
+ k->realize = xen_pt_realize;
k->exit = xen_pt_unregister_device;
k->config_read = xen_pt_pci_read_config;
k->config_write = xen_pt_pci_write_config;
--
2.1.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get()
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
@ 2016-01-04 15:15 ` Stefano Stabellini
2016-01-05 5:31 ` Cao jin
2016-01-05 7:23 ` Cao jin
0 siblings, 2 replies; 13+ messages in thread
From: Stefano Stabellini @ 2016-01-04 15:15 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, stefano.stabellini
On Sun, 27 Dec 2015, Cao jin wrote:
> To catch the error msg. Also modify the caller
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
This looks much better, thanks.
> hw/xen/xen-host-pci-device.c | 102 ++++++++++++++++++++++++-------------------
> hw/xen/xen-host-pci-device.h | 5 ++-
> hw/xen/xen_pt.c | 12 ++---
> 3 files changed, 67 insertions(+), 52 deletions(-)
>
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 7d8a023..3d22095 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>
> /* This size should be enough to read the first 7 lines of a resource file */
> #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
> -static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> +static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
> {
> int i, rc, fd;
> char path[PATH_MAX];
> @@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>
> rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
> + return;
> }
> +
> fd = open(path, O_RDONLY);
> if (fd == -1) {
> - XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> - return -errno;
> + error_setg_errno(errp, errno, "open %s err", path);
> + return;
> }
>
> do {
> rc = read(fd, &buf, sizeof (buf) - 1);
> if (rc < 0 && errno != EINTR) {
> - rc = -errno;
> + error_setg_errno(errp, errno, "read err");
> goto out;
> }
> } while (rc < 0);
> buf[rc] = 0;
> - rc = 0;
>
> s = buf;
> for (i = 0; i < PCI_NUM_REGIONS; i++) {
> @@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> d->rom.bus_flags = flags & IORESOURCE_BITS;
> }
> }
> +
> if (i != PCI_NUM_REGIONS) {
> /* Invalid format or input to short */
> - rc = -ENODEV;
> + error_setg(errp, "Invalid format or input to short: %s", buf);
> }
>
> out:
> close(fd);
> - return rc;
> }
>
> /* This size should be enough to read a long from a file */
> #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
> -static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> - unsigned int *pvalue, int base)
> +static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> + unsigned int *pvalue, int base, Error **errp)
> {
> char path[PATH_MAX];
> char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
> @@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>
> rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
> + return;
> }
> +
> fd = open(path, O_RDONLY);
> if (fd == -1) {
> - XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> - return -errno;
> + error_setg_errno(errp, errno, "open %s err", path);
> + return;
> }
> +
> do {
> rc = read(fd, &buf, sizeof (buf) - 1);
> if (rc < 0 && errno != EINTR) {
> - rc = -errno;
> + error_setg_errno(errp, errno, "read err");
> goto out;
> }
> } while (rc < 0);
> +
> buf[rc] = 0;
> value = strtol(buf, &endptr, base);
> if (endptr == buf || *endptr != '\n') {
> - rc = -1;
> + error_setg(errp, "format invalid: %s", buf);
> } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> - rc = -errno;
> + error_setg_errno(errp, errno, "strtol err");
> } else {
> - rc = 0;
> *pvalue = value;
> }
> +
> out:
> close(fd);
> - return rc;
> }
>
> -static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
> +static inline void xen_host_pci_get_hex_value(XenHostPCIDevice *d,
> const char *name,
> - unsigned int *pvalue)
> + unsigned int *pvalue,
> + Error **errp)
> {
> - return xen_host_pci_get_value(d, name, pvalue, 16);
> + xen_host_pci_get_value(d, name, pvalue, 16, errp);
> }
>
> -static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
> +static inline void xen_host_pci_get_dec_value(XenHostPCIDevice *d,
> const char *name,
> - unsigned int *pvalue)
> + unsigned int *pvalue,
> + Error **errp)
> {
> - return xen_host_pci_get_value(d, name, pvalue, 10);
> + xen_host_pci_get_value(d, name, pvalue, 10, errp);
> }
>
> static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
> @@ -206,20 +212,21 @@ static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
> return !stat(path, &buf);
> }
>
> -static int xen_host_pci_config_open(XenHostPCIDevice *d)
> +static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
> {
> char path[PATH_MAX];
> int rc;
>
> rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
> + return;
> }
> +
> d->config_fd = open(path, O_RDWR);
> if (d->config_fd < 0) {
> - return -errno;
> + error_setg_errno(errp, errno, "open %s err", path);
> }
> - return 0;
> }
>
> static int xen_host_pci_config_read(XenHostPCIDevice *d,
> @@ -341,11 +348,11 @@ int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
> return -1;
> }
>
> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> - uint8_t bus, uint8_t dev, uint8_t func)
> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> + uint8_t bus, uint8_t dev, uint8_t func,
> + Error **errp)
> {
> unsigned int v;
> - int rc = 0;
>
> d->config_fd = -1;
> d->domain = domain;
> @@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> d->dev = dev;
> d->func = func;
>
> - rc = xen_host_pci_config_open(d);
> - if (rc) {
> + xen_host_pci_config_open(d, errp);
> + if (*errp) {
I think that errp could be NULL, therefore the right way to do this is:
Error *err = NULL;
foo(arg, &err);
if (err) {
handle the error...
error_propagate(errp, err);
}
see the comment at the beginning of include/qapi/error.h.
> goto error;
> }
> - rc = xen_host_pci_get_resource(d);
> - if (rc) {
> +
> + xen_host_pci_get_resource(d, errp);
> + if (*errp) {
> goto error;
> }
> - rc = xen_host_pci_get_hex_value(d, "vendor", &v);
> - if (rc) {
> +
> + xen_host_pci_get_hex_value(d, "vendor", &v, errp);
> + if (*errp) {
> goto error;
> }
> d->vendor_id = v;
> - rc = xen_host_pci_get_hex_value(d, "device", &v);
> - if (rc) {
> +
> + xen_host_pci_get_hex_value(d, "device", &v, errp);
> + if (*errp) {
> goto error;
> }
> d->device_id = v;
> - rc = xen_host_pci_get_dec_value(d, "irq", &v);
> - if (rc) {
> +
> + xen_host_pci_get_dec_value(d, "irq", &v, errp);
> + if (*errp) {
> goto error;
> }
> d->irq = v;
> - rc = xen_host_pci_get_hex_value(d, "class", &v);
> - if (rc) {
> +
> + xen_host_pci_get_hex_value(d, "class", &v, errp);
> + if (*errp) {
> goto error;
> }
> d->class_code = v;
> +
> d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
>
> - return 0;
> + return;
> error:
> if (d->config_fd >= 0) {
> close(d->config_fd);
> d->config_fd = -1;
> }
> - return rc;
> }
>
> bool xen_host_pci_device_closed(XenHostPCIDevice *d)
> diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
> index 3d44e04..42b9138 100644
> --- a/hw/xen/xen-host-pci-device.h
> +++ b/hw/xen/xen-host-pci-device.h
> @@ -36,8 +36,9 @@ typedef struct XenHostPCIDevice {
> int config_fd;
> } XenHostPCIDevice;
>
> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> - uint8_t bus, uint8_t dev, uint8_t func);
> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> + uint8_t bus, uint8_t dev, uint8_t func,
> + Error **errp);
> void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
> bool xen_host_pci_device_closed(XenHostPCIDevice *d);
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index aa96288..1bd4109 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -767,6 +767,7 @@ static int xen_pt_initfn(PCIDevice *d)
> uint8_t machine_irq = 0, scratch;
> uint16_t cmd = 0;
> int pirq = XEN_PT_UNASSIGNED_PIRQ;
> + Error *local_err = NULL;
>
> /* register real device */
> XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
> @@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
> s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
> s->dev.devfn);
>
> - rc = xen_host_pci_device_get(&s->real_device,
> - s->hostaddr.domain, s->hostaddr.bus,
> - s->hostaddr.slot, s->hostaddr.function);
> - if (rc) {
> - XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
> + xen_host_pci_device_get(&s->real_device,
> + s->hostaddr.domain, s->hostaddr.bus,
> + s->hostaddr.slot, s->hostaddr.function,
> + &local_err);
> + if (local_err) {
> + XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
> return -1;
> }
>
> --
> 2.1.0
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Add Error **errp for xen_pt_setup_vga()
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 2/4] Add Error **errp for xen_pt_setup_vga() Cao jin
@ 2016-01-04 15:18 ` Stefano Stabellini
0 siblings, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2016-01-04 15:18 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, stefano.stabellini
On Sun, 27 Dec 2015, Cao jin wrote:
> To catch the error msg. Also modify the caller
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> hw/xen/xen_pt.c | 5 ++++-
> hw/xen/xen_pt.h | 3 ++-
> hw/xen/xen_pt_graphics.c | 11 ++++++-----
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 1bd4109..fbce55c 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -807,7 +807,10 @@ static int xen_pt_initfn(PCIDevice *d)
> return -1;
> }
>
> - if (xen_pt_setup_vga(s, &s->real_device) < 0) {
> + xen_pt_setup_vga(s, &s->real_device, &local_err);
> + if (local_err) {
> + error_append_hint(&local_err, "Setup VGA BIOS of passthrough"
> + " GFX failed!");
> XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
> xen_host_pci_device_put(&s->real_device);
> return -1;
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index c545280..dc74d3e 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -328,5 +328,6 @@ static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
> }
> int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
> int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);
> -int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev);
> +void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
> + Error **errp);
> #endif /* !XEN_PT_H */
> diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
> index df6069b..a0a7e9c 100644
> --- a/hw/xen/xen_pt_graphics.c
> +++ b/hw/xen/xen_pt_graphics.c
> @@ -161,7 +161,8 @@ struct pci_data {
> uint16_t reserved;
> } __attribute__((packed));
>
> -int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
> +void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
> + Error **errp)
> {
> unsigned char *bios = NULL;
> struct rom_header *rom;
> @@ -172,13 +173,14 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
> struct pci_data *pd = NULL;
>
> if (!is_igd_vga_passthrough(dev)) {
> - return -1;
> + error_setg(errp, "Need to enable igd-passthrough");
> + return;
> }
>
> bios = get_vgabios(s, &bios_size, dev);
> if (!bios) {
> - XEN_PT_ERR(&s->dev, "VGA: Can't getting VBIOS!\n");
> - return -1;
> + error_setg(errp, "VGA: Can't getting VBIOS!");
> + return;
> }
>
> /* Currently we fixed this address as a primary. */
> @@ -203,7 +205,6 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
>
> /* Currently we fixed this address as a primary for legacy BIOS. */
> cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
> - return 0;
> }
>
> uint32_t igd_read_opregion(XenPCIPassthroughState *s)
> --
> 2.1.0
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] Add Error **errp for xen_pt_config_init()
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 3/4] Add Error **errp for xen_pt_config_init() Cao jin
@ 2016-01-04 15:23 ` Stefano Stabellini
0 siblings, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2016-01-04 15:23 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, stefano.stabellini
On Sun, 27 Dec 2015, Cao jin wrote:
> To catch the error msg. Also modify the caller
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/xen/xen_pt.c | 7 ++++---
> hw/xen/xen_pt.h | 2 +-
> hw/xen/xen_pt_config_init.c | 48 ++++++++++++++++++++++-----------------------
> 3 files changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index fbce55c..3787c26 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -824,9 +824,10 @@ static int xen_pt_initfn(PCIDevice *d)
> xen_pt_register_regions(s, &cmd);
>
> /* reinitialize each config register to be emulated */
> - rc = xen_pt_config_init(s);
> - if (rc) {
> - XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
> + xen_pt_config_init(s, &local_err);
> + if (local_err) {
> + error_append_hint(&local_err, "PCI Config space initialisation failed");
> + rc = -1;
> goto err_out;
> }
>
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index dc74d3e..466525f 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -228,7 +228,7 @@ struct XenPCIPassthroughState {
> bool listener_set;
> };
>
> -int xen_pt_config_init(XenPCIPassthroughState *s);
> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp);
> void xen_pt_config_delete(XenPCIPassthroughState *s);
> XenPTRegGroup *xen_pt_find_reg_grp(XenPCIPassthroughState *s, uint32_t address);
> XenPTReg *xen_pt_find_reg(XenPTRegGroup *reg_grp, uint32_t address);
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 3d8686d..096f204 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1899,8 +1899,9 @@ static uint8_t find_cap_offset(XenPCIPassthroughState *s, uint8_t cap)
> return 0;
> }
>
> -static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> - XenPTRegGroup *reg_grp, XenPTRegInfo *reg)
> +static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
> + XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
> + Error **errp)
> {
> XenPTReg *reg_entry;
> uint32_t data = 0;
> @@ -1919,12 +1920,13 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> reg_grp->base_offset + reg->offset, &data);
> if (rc < 0) {
> g_free(reg_entry);
> - return rc;
> + error_setg(errp, "Init emulate register fail");
> + return;
> }
> if (data == XEN_PT_INVALID_REG) {
> /* free unused BAR register entry */
> g_free(reg_entry);
> - return 0;
> + return;
> }
> /* Sync up the data to dev.config */
> offset = reg_grp->base_offset + reg->offset;
> @@ -1942,7 +1944,8 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> if (rc) {
> /* Serious issues when we cannot read the host values! */
> g_free(reg_entry);
> - return rc;
> + error_setg(errp, "Cannot read host values");
> + return;
> }
> /* Set bits in emu_mask are the ones we emulate. The dev.config shall
> * contain the emulated view of the guest - therefore we flip the mask
> @@ -1967,10 +1970,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> val = data;
>
> if (val & ~size_mask) {
> - XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
> - offset, val, reg->size);
> + error_setg(errp, "Offset 0x%04x:0x%04x expands past"
> + " register size(%d)!", offset, val, reg->size);
> g_free(reg_entry);
> - return -ENXIO;
> + return;
> }
> /* This could be just pci_set_long as we don't modify the bits
> * past reg->size, but in case this routine is run in parallel or the
> @@ -1990,11 +1993,9 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> }
> /* list add register entry */
> QLIST_INSERT_HEAD(®_grp->reg_tbl_list, reg_entry, entries);
> -
> - return 0;
> }
>
> -int xen_pt_config_init(XenPCIPassthroughState *s)
> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
> {
> int i, rc;
>
> @@ -2039,11 +2040,11 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
> reg_grp_offset,
> ®_grp_entry->size);
> if (rc < 0) {
> - XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
> - i, ARRAY_SIZE(xen_pt_emu_reg_grps),
> + error_setg(errp, "Failed to initialize %d/%ld, type=0x%x,"
> + " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
> xen_pt_emu_reg_grps[i].grp_type, rc);
> xen_pt_config_delete(s);
> - return rc;
> + return;
> }
> }
>
> @@ -2051,24 +2052,23 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
> if (xen_pt_emu_reg_grps[i].emu_regs) {
> int j = 0;
> XenPTRegInfo *regs = xen_pt_emu_reg_grps[i].emu_regs;
> +
> /* initialize capability register */
> for (j = 0; regs->size != 0; j++, regs++) {
> - /* initialize capability register */
> - rc = xen_pt_config_reg_init(s, reg_grp_entry, regs);
> - if (rc < 0) {
> - XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
> - j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
> - regs->offset, xen_pt_emu_reg_grps[i].grp_type,
> - i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
> + xen_pt_config_reg_init(s, reg_grp_entry, regs, errp);
> + if (*errp) {
errp could theoretically be NULL
> + error_append_hint(errp, "Failed to initialize %d/%ld"
> + " reg 0x%x in grp_type=0x%x (%d/%ld)",
> + j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
> + regs->offset, xen_pt_emu_reg_grps[i].grp_type,
> + i, ARRAY_SIZE(xen_pt_emu_reg_grps));
> xen_pt_config_delete(s);
> - return rc;
> + return;
> }
> }
> }
> }
> }
> -
> - return 0;
> }
>
> /* delete all emulate register */
> --
> 2.1.0
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] Xen PCI passthru: convert to realize()
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 4/4] Xen PCI passthru: convert to realize() Cao jin
@ 2016-01-04 15:29 ` Stefano Stabellini
0 siblings, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2016-01-04 15:29 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, stefano.stabellini
On Sun, 27 Dec 2015, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> hw/xen/xen_pt.c | 53 ++++++++++++++++++++++++++++-------------------------
> 1 file changed, 28 insertions(+), 25 deletions(-)
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 3787c26..b058f61 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -760,10 +760,10 @@ static void xen_pt_destroy(PCIDevice *d) {
> }
> /* init */
>
> -static int xen_pt_initfn(PCIDevice *d)
> +static void xen_pt_realize(PCIDevice *d, Error **errp)
> {
> XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
> - int rc = 0;
> + int i, rc = 0;
> uint8_t machine_irq = 0, scratch;
> uint16_t cmd = 0;
> int pirq = XEN_PT_UNASSIGNED_PIRQ;
> @@ -780,8 +780,8 @@ static int xen_pt_initfn(PCIDevice *d)
> s->hostaddr.slot, s->hostaddr.function,
> &local_err);
> if (local_err) {
> - XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
> - return -1;
> + error_propagate(errp, local_err);
> + return;
> }
>
> s->is_virtfn = s->real_device.is_virtfn;
> @@ -801,19 +801,19 @@ static int xen_pt_initfn(PCIDevice *d)
> if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
> (s->real_device.dev == 2) && (s->real_device.func == 0)) {
> if (!is_igd_vga_passthrough(&s->real_device)) {
> - XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
> - " to passthrough IGD GFX.\n");
> + error_setg(errp, "Need to enable igd-passthru if you're trying"
> + " to passthrough IGD GFX.");
> xen_host_pci_device_put(&s->real_device);
> - return -1;
> + return;
> }
>
> xen_pt_setup_vga(s, &s->real_device, &local_err);
> if (local_err) {
> error_append_hint(&local_err, "Setup VGA BIOS of passthrough"
> " GFX failed!");
> - XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
> + error_propagate(errp, local_err);
> xen_host_pci_device_put(&s->real_device);
> - return -1;
> + return;
> }
>
> /* Register ISA bridge for passthrough GFX. */
> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
> xen_pt_config_init(s, &local_err);
> if (local_err) {
> error_append_hint(&local_err, "PCI Config space initialisation failed");
> - rc = -1;
> + error_propagate(errp, local_err);
> goto err_out;
> }
>
> /* Bind interrupt */
> rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
> if (rc) {
> - XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
> + error_setg_errno(errp, errno, "Failed to read PCI_INTERRUPT_PIN!");
> goto err_out;
> }
> if (!scratch) {
> - XEN_PT_LOG(d, "no pin interrupt\n");
> + error_setg(errp, "no pin interrupt");
> goto out;
> }
>
> machine_irq = s->real_device.irq;
> rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
> -
> if (rc < 0) {
> - XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
> - machine_irq, pirq, errno);
> + error_setg_errno(errp, errno, "Mapping machine irq %u to"
> + " pirq %i failed", machine_irq, pirq);
>
> /* Disable PCI intx assertion (turn on bit10 of devctl) */
> cmd |= PCI_COMMAND_INTX_DISABLE;
> @@ -868,8 +867,8 @@ static int xen_pt_initfn(PCIDevice *d)
> PCI_SLOT(d->devfn),
> e_intx);
> if (rc < 0) {
> - XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
> - e_intx, errno);
> + error_setg_errno(errp, errno, "Binding of interrupt %i failed!",
> + e_intx);
>
> /* Disable PCI intx assertion (turn on bit10 of devctl) */
> cmd |= PCI_COMMAND_INTX_DISABLE;
> @@ -877,8 +876,8 @@ static int xen_pt_initfn(PCIDevice *d)
>
> if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
> if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
> - XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
> - " (err: %d)\n", machine_irq, errno);
> + error_setg_errno(errp, errno, "Unmapping of machine"
> + " interrupt %i failed!", machine_irq);
> }
> }
> s->machine_irq = 0;
> @@ -891,14 +890,14 @@ out:
>
> rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
> if (rc) {
> - XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
> + error_setg_errno(errp, errno, "Failed to read PCI_COMMAND!");
> goto err_out;
> } else {
> val |= cmd;
> rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
> if (rc) {
> - XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
> - val, rc);
> + error_setg_errno(errp, errno, "Failed to write PCI_COMMAND"
> + " val=0x%x!", val);
> goto err_out;
> }
> }
> @@ -911,12 +910,16 @@ out:
> "Real physical device %02x:%02x.%d registered successfully!\n",
> s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
>
> - return 0;
> + return;
>
> err_out:
> + for (i = 0; i < PCI_ROM_SLOT; i++) {
> + object_unparent(OBJECT(&s->bar[i]));
> + }
> + object_unparent(OBJECT(&s->rom));
> +
> xen_pt_destroy(d);
> assert(rc);
> - return rc;
> }
>
> static void xen_pt_unregister_device(PCIDevice *d)
> @@ -935,7 +938,7 @@ static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> - k->init = xen_pt_initfn;
> + k->realize = xen_pt_realize;
> k->exit = xen_pt_unregister_device;
> k->config_read = xen_pt_pci_read_config;
> k->config_write = xen_pt_pci_write_config;
> --
> 2.1.0
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get()
2016-01-04 15:15 ` Stefano Stabellini
@ 2016-01-05 5:31 ` Cao jin
2016-01-05 7:23 ` Cao jin
1 sibling, 0 replies; 13+ messages in thread
From: Cao jin @ 2016-01-05 5:31 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel
On 01/04/2016 11:15 PM, Stefano Stabellini wrote:
> On Sun, 27 Dec 2015, Cao jin wrote:
>> To catch the error msg. Also modify the caller
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>
> This looks much better, thanks.
>
>
[...]
>>
>> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> - uint8_t bus, uint8_t dev, uint8_t func)
>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> + uint8_t bus, uint8_t dev, uint8_t func,
>> + Error **errp)
>> {
>> unsigned int v;
>> - int rc = 0;
>>
>> d->config_fd = -1;
>> d->domain = domain;
>> @@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> d->dev = dev;
>> d->func = func;
>>
>> - rc = xen_host_pci_config_open(d);
>> - if (rc) {
>> + xen_host_pci_config_open(d, errp);
>> + if (*errp) {
>
> I think that errp could be NULL, therefore the right way to do this is:
>
> Error *err = NULL;
> foo(arg, &err);
> if (err) {
> handle the error...
> error_propagate(errp, err);
> }
>
> see the comment at the beginning of include/qapi/error.h.
>
Thanks for reminding, I didn`t see the comment of error.h before, now I
am aware why lots of people like the style you mentioned. Will fix it in
next version, also the comments in other patch.
[...]
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get()
2016-01-04 15:15 ` Stefano Stabellini
2016-01-05 5:31 ` Cao jin
@ 2016-01-05 7:23 ` Cao jin
2016-01-05 10:40 ` Stefano Stabellini
1 sibling, 1 reply; 13+ messages in thread
From: Cao jin @ 2016-01-05 7:23 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel
On 01/04/2016 11:15 PM, Stefano Stabellini wrote:
> On Sun, 27 Dec 2015, Cao jin wrote:
>> To catch the error msg. Also modify the caller
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>
> This looks much better, thanks.
>
>
[...]
>>
>> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> - uint8_t bus, uint8_t dev, uint8_t func)
>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> + uint8_t bus, uint8_t dev, uint8_t func,
>> + Error **errp)
>> {
>> unsigned int v;
>> - int rc = 0;
>>
>> d->config_fd = -1;
>> d->domain = domain;
>> @@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> d->dev = dev;
>> d->func = func;
>>
>> - rc = xen_host_pci_config_open(d);
>> - if (rc) {
>> + xen_host_pci_config_open(d, errp);
>> + if (*errp) {
>
> I think that errp could be NULL, therefore the right way to do this is:
>
> Error *err = NULL;
> foo(arg, &err);
> if (err) {
> handle the error...
> error_propagate(errp, err);
> }
>
> see the comment at the beginning of include/qapi/error.h.
>
Hi stefano,
I read that comment, and find something maybe new:
"errp could be NULL", I think it is saying, if we are in a .realize()
function, yes, *errp* maybe NULL, but reality is, here is the callee of
.realize(), and we defined a local variable: Error *local_err = NULL in
.realize() and passed it to all the callee, so, theoretically *errp*
won`t be NULL. so the way you said above is suitable in .realize() IMHO,
and I also did it in that way.
comment also says:
* Receive an error and pass it on to the caller:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
* error_propagate(errp, err);
* }
* where Error **errp is a parameter, by convention the last one.
If I understand the last sentence well, the Error **errp in .realize()
prototype is *the last one*, so we could call error_propagate(errp, err)
only in .realize()
The comment also says:
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability."
We just pass error on in all the callees, so I guess I also did as
comment suggest?
How do you think?
[...]
--
Yours Sincerely,
Cao Jin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get()
2016-01-05 7:23 ` Cao jin
@ 2016-01-05 10:40 ` Stefano Stabellini
2016-01-05 12:41 ` Cao jin
0 siblings, 1 reply; 13+ messages in thread
From: Stefano Stabellini @ 2016-01-05 10:40 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, Stefano Stabellini
On Tue, 5 Jan 2016, Cao jin wrote:
> On 01/04/2016 11:15 PM, Stefano Stabellini wrote:
> > On Sun, 27 Dec 2015, Cao jin wrote:
> > > To catch the error msg. Also modify the caller
> > >
> > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> >
> > This looks much better, thanks.
> >
> >
> [...]
> > >
> > > -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> > > - uint8_t bus, uint8_t dev, uint8_t func)
> > > +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> > > + uint8_t bus, uint8_t dev, uint8_t func,
> > > + Error **errp)
> > > {
> > > unsigned int v;
> > > - int rc = 0;
> > >
> > > d->config_fd = -1;
> > > d->domain = domain;
> > > @@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d,
> > > uint16_t domain,
> > > d->dev = dev;
> > > d->func = func;
> > >
> > > - rc = xen_host_pci_config_open(d);
> > > - if (rc) {
> > > + xen_host_pci_config_open(d, errp);
> > > + if (*errp) {
> >
> > I think that errp could be NULL, therefore the right way to do this is:
> >
> > Error *err = NULL;
> > foo(arg, &err);
> > if (err) {
> > handle the error...
> > error_propagate(errp, err);
> > }
> >
> > see the comment at the beginning of include/qapi/error.h.
> >
>
> Hi stefano,
>
> I read that comment, and find something maybe new:
>
> "errp could be NULL", I think it is saying, if we are in a .realize()
> function, yes, *errp* maybe NULL, but reality is, here is the callee of
> .realize(), and we defined a local variable: Error *local_err = NULL in
> .realize() and passed it to all the callee, so, theoretically *errp* won`t be
> NULL.
This is true, however I think that relying on it is error prone: in a
couple of years from now somebody might change the call sequence without
updating the error handling (easy to forget), causing QEMU to crash on
error. I think it is safer not to rely on errp != NULL.
> so the way you said above is suitable in .realize() IMHO, and I also did
> it in that way.
>
> comment also says:
>
> * Receive an error and pass it on to the caller:
> * Error *err = NULL;
> * foo(arg, &err);
> * if (err) {
> * handle the error...
> * error_propagate(errp, err);
> * }
> * where Error **errp is a parameter, by convention the last one.
>
> If I understand the last sentence well, the Error **errp in .realize()
> prototype is *the last one*, so we could call error_propagate(errp, err) only
> in .realize()
>
> The comment also says:
>
> * But when all you do with the error is pass it on, please use
> * foo(arg, errp);
> * for readability."
>
> We just pass error on in all the callees, so I guess I also did as comment
> suggest?
>
> How do you think?
I think we only need to use a local Error variable when we want to check
for the returned error, in cases such as:
if (*errp) {
In other cases, when we are not interested in *errp, we can simply
propagate the error, like you have done in your patches.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get()
2016-01-05 10:40 ` Stefano Stabellini
@ 2016-01-05 12:41 ` Cao jin
0 siblings, 0 replies; 13+ messages in thread
From: Cao jin @ 2016-01-05 12:41 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel
On 01/05/2016 06:40 PM, Stefano Stabellini wrote:
> On Tue, 5 Jan 2016, Cao jin wrote:
[...]
>
> This is true, however I think that relying on it is error prone: in a
> couple of years from now somebody might change the call sequence without
> updating the error handling (easy to forget), causing QEMU to crash on
> error. I think it is safer not to rely on errp != NULL.
>
I see, sounds reasonable
>
[...]
> I think we only need to use a local Error variable when we want to check
> for the returned error, in cases such as:
>
> if (*errp) {
>
> In other cases, when we are not interested in *errp, we can simply
> propagate the error, like you have done in your patches.
>
Ok, will fix it
> .
>
--
Yours Sincerely,
Cao Jin
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-01-05 12:38 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-27 13:40 [Qemu-devel] [PATCH v2 0/4] Convert to realize() Cao jin
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get() Cao jin
2016-01-04 15:15 ` Stefano Stabellini
2016-01-05 5:31 ` Cao jin
2016-01-05 7:23 ` Cao jin
2016-01-05 10:40 ` Stefano Stabellini
2016-01-05 12:41 ` Cao jin
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 2/4] Add Error **errp for xen_pt_setup_vga() Cao jin
2016-01-04 15:18 ` Stefano Stabellini
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 3/4] Add Error **errp for xen_pt_config_init() Cao jin
2016-01-04 15:23 ` Stefano Stabellini
2015-12-27 13:40 ` [Qemu-devel] [PATCH v2 4/4] Xen PCI passthru: convert to realize() Cao jin
2016-01-04 15:29 ` 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.