Add Error argument for pci_add_capability() to leverage the errp
to pass info on errors. This way is helpful for its callers to
make a better error handling when moving to 'realize'.
Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
CC: dmitry@daynix.com
Cc: jasowang@redhat.com
Cc: marcel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
hw/net/e1000e.c | 7 ++++++-
hw/net/eepro100.c | 20 +++++++++++++++-----
hw/pci-bridge/i82801b11.c | 1 +
hw/pci/pci.c | 10 ++++------
hw/pci/pci_bridge.c | 7 ++++++-
hw/pci/pcie.c | 10 ++++++++--
hw/pci/shpc.c | 5 ++++-
hw/pci/slotid_cap.c | 7 ++++++-
hw/vfio/pci.c | 3 ++-
hw/virtio/virtio-pci.c | 19 ++++++++++++++-----
include/hw/pci/pci.h | 3 ++-
12 files changed, 85 insertions(+), 31 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 7b6d4ea..d93ffc2 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
x86_iommu->type = TYPE_AMD;
qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
- s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
- AMDVI_CAPAB_SIZE);
- assert(s->capab_offset > 0);
- ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
- assert(ret > 0);
- ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
- assert(ret > 0);
+ ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
+ AMDVI_CAPAB_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
+ s->capab_offset = ret;
+
+ ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
+ AMDVI_CAPAB_REG_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
+ ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
+ AMDVI_CAPAB_REG_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
/* set up MMIO */
memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 8259d67..41430766 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -47,6 +47,7 @@
#include "e1000e_core.h"
#include "trace.h"
+#include "qapi/error.h"
#define TYPE_E1000E "e1000e"
#define E1000E(obj) OBJECT_CHECK(E1000EState, (obj), TYPE_E1000E)
@@ -372,7 +373,9 @@ e1000e_gen_dsn(uint8_t *mac)
static int
e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
{
- int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
+ Error *local_err = NULL;
+ int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
+ PCI_PM_SIZEOF, &local_err);
if (ret > 0) {
pci_set_word(pdev->config + offset + PCI_PM_PMC,
@@ -386,6 +389,8 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
PCI_PM_CTRL_PME_STATUS);
+ } else {
+ error_report_err(local_err);
}
return ret;
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 62e989c..0625839 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -48,6 +48,7 @@
#include "sysemu/sysemu.h"
#include "sysemu/dma.h"
#include "qemu/bitops.h"
+#include "qapi/error.h"
/* QEMU sends frames smaller than 60 bytes to ethernet nics.
* Such frames are rejected by real nics and their emulations.
@@ -494,7 +495,7 @@ static void eepro100_fcp_interrupt(EEPRO100State *s)
}
#endif
-static void e100_pci_reset(EEPRO100State *s)
+static void e100_pci_reset(EEPRO100State *s, Error **errp)
{
E100PCIDeviceInfo *info = eepro100_get_class(s);
uint32_t device = s->device;
@@ -570,9 +571,13 @@ static void e100_pci_reset(EEPRO100State *s)
/* Power Management Capabilities */
int cfg_offset = 0xdc;
int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
- cfg_offset, PCI_PM_SIZEOF);
- assert(r > 0);
- pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
+ cfg_offset, PCI_PM_SIZEOF,
+ errp);
+ if (r > 0) {
+ pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
+ } else {
+ return;
+ }
#if 0 /* TODO: replace dummy code for power management emulation. */
/* TODO: Power Management Control / Status. */
pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
@@ -1858,12 +1863,17 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
{
EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
E100PCIDeviceInfo *info = eepro100_get_class(s);
+ Error *local_err = NULL;
TRACE(OTHER, logout("\n"));
s->device = info->device;
- e100_pci_reset(s);
+ e100_pci_reset(s, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
/* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
* i82559 and later support 64 or 256 word EEPROM. */
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 2404e7e..2c065c3 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -44,6 +44,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
#include "hw/i386/ich9.h"
+#include "qapi/error.h"
/*****************************************************************************/
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b73bfea..2bba37a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
* in pci config space
*/
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size)
+ uint8_t offset, uint8_t size,
+ Error **errp)
{
int ret;
- Error *local_err = NULL;
- ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
- if (ret < 0) {
- error_report_err(local_err);
- }
+ ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
+
return ret;
}
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 5118ef4..bb0f3a3 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -33,6 +33,7 @@
#include "hw/pci/pci_bridge.h"
#include "hw/pci/pci_bus.h"
#include "qemu/range.h"
+#include "qapi/error.h"
/* PCI bridge subsystem vendor ID helper functions */
#define PCI_SSVID_SIZEOF 8
@@ -43,8 +44,12 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
uint16_t svid, uint16_t ssid)
{
int pos;
- pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, PCI_SSVID_SIZEOF);
+ Error *local_err = NULL;
+
+ pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
+ PCI_SSVID_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 18e634f..f187512 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -91,11 +91,14 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
/* PCIe cap v2 init */
int pos;
uint8_t *exp_cap;
+ Error *local_err = NULL;
assert(pci_is_express(dev));
- pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF);
+ pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
+ PCI_EXP_VER2_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
dev->exp.exp_cap = pos;
@@ -123,11 +126,14 @@ int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
{
/* PCIe cap v1 init */
int pos;
+ Error *local_err = NULL;
assert(pci_is_express(dev));
- pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER1_SIZEOF);
+ pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
+ PCI_EXP_VER1_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
dev->exp.exp_cap = pos;
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 42fafac..d72d5e4 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -450,9 +450,12 @@ static int shpc_cap_add_config(PCIDevice *d)
{
uint8_t *config;
int config_offset;
+ Error *local_err = NULL;
config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
- 0, SHPC_CAP_LENGTH);
+ 0, SHPC_CAP_LENGTH,
+ &local_err);
if (config_offset < 0) {
+ error_report_err(local_err);
return config_offset;
}
config = d->config + config_offset;
diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
index aec1e91..bdca205 100644
--- a/hw/pci/slotid_cap.c
+++ b/hw/pci/slotid_cap.c
@@ -2,6 +2,7 @@
#include "hw/pci/slotid_cap.h"
#include "hw/pci/pci.h"
#include "qemu/error-report.h"
+#include "qapi/error.h"
#define SLOTID_CAP_LENGTH 4
#define SLOTID_NSLOTS_SHIFT ctz32(PCI_SID_ESR_NSLOTS)
@@ -11,6 +12,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
unsigned offset)
{
int cap;
+ Error *local_err = NULL;
+
if (!chassis) {
error_report("Bridge chassis not specified. Each bridge is required "
"to be assigned a unique chassis id > 0.");
@@ -21,8 +24,10 @@ int slotid_cap_init(PCIDevice *d, int nslots,
return -EINVAL;
}
- cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, SLOTID_CAP_LENGTH);
+ cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
+ SLOTID_CAP_LENGTH, &local_err);
if (cap < 0) {
+ error_report_err(local_err);
return cap;
}
/* We make each chassis unique, this way each bridge is First in Chassis */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5881968..85cfe38 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1743,7 +1743,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
}
- pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
+ pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
+ errp);
if (pos > 0) {
vdev->pdev.exp.exp_cap = pos;
}
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f9b7244..cca5276 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1161,9 +1161,14 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
{
PCIDevice *dev = &proxy->pci_dev;
int offset;
+ Error *local_err = NULL;
- offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
- assert(offset > 0);
+ offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
+ cap->cap_len, &local_err);
+ if (offset < 0) {
+ error_report_err(local_err);
+ abort();
+ }
assert(cap->cap_len >= sizeof *cap);
memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
@@ -1810,9 +1815,13 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
pos = pcie_endpoint_cap_init(pci_dev, 0);
assert(pos > 0);
- pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
- assert(pos > 0);
- pci_dev->exp.pm_cap = pos;
+ pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
+ PCI_PM_SIZEOF, errp);
+ if (pos > 0) {
+ pci_dev->exp.pm_cap = pos;
+ } else {
+ return;
+ }
/*
* Indicates that this function complies with revision 1.2 of the
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index a37a2d5..fe52aa8 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -356,7 +356,8 @@ void pci_unregister_vga(PCIDevice *pci_dev);
pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size);
+ uint8_t offset, uint8_t size,
+ Error **errp);
int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size,
Error **errp);
--
2.9.3
On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
> Add Error argument for pci_add_capability() to leverage the errp
> to pass info on errors. This way is helpful for its callers to
> make a better error handling when moving to 'realize'.
>
> Cc: pbonzini@redhat.com
> Cc: rth@twiddle.net
> Cc: ehabkost@redhat.com
> Cc: mst@redhat.com
> CC: dmitry@daynix.com
> Cc: jasowang@redhat.com
> Cc: marcel@redhat.com
> Cc: alex.williamson@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
> hw/net/e1000e.c | 7 ++++++-
> hw/net/eepro100.c | 20 +++++++++++++++-----
> hw/pci-bridge/i82801b11.c | 1 +
> hw/pci/pci.c | 10 ++++------
> hw/pci/pci_bridge.c | 7 ++++++-
> hw/pci/pcie.c | 10 ++++++++--
> hw/pci/shpc.c | 5 ++++-
> hw/pci/slotid_cap.c | 7 ++++++-
> hw/vfio/pci.c | 3 ++-
> hw/virtio/virtio-pci.c | 19 ++++++++++++++-----
> include/hw/pci/pci.h | 3 ++-
> 12 files changed, 85 insertions(+), 31 deletions(-)
There are multiple places below that checks for errors like this:
function(...);
if (function succeeded) {
/* non-error code path here */
foo = bar;
}
Sometimes it even includes another branch for the error path:
function(...);
if (function succeeded) {
/* non-error code path here */
foo = bar;
} else {
/* error path here */
return ret;
}
I suggest doing this instead, for readability:
function(...)
if (function failed) {
return ...; /* or: "goto out" */
}
/* non-error code path here */
foo = bar;
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 7b6d4ea..d93ffc2 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
> x86_iommu->type = TYPE_AMD;
> qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
> object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
> - s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> - AMDVI_CAPAB_SIZE);
> - assert(s->capab_offset > 0);
> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
> - assert(ret > 0);
> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
> - assert(ret > 0);
> + ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> + AMDVI_CAPAB_SIZE, err);
> + if (ret < 0) {
> + return;
Maybe adding a local_err variable is preferred instead of
checking for (pos < 0), but I'm not sure it's necessary.
Markus, what's the recommendation on those cases? Should we use
the negative return value to avoid adding an extra local_err
variable, or should we add local_err anyway to match the existing
style elsewhere?
(The same applies to other functions below [*])
> + }
> + s->capab_offset = ret;
> +
> + ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
> + AMDVI_CAPAB_REG_SIZE, err);
> + if (ret < 0) {
> + return;
> + }
> + ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
> + AMDVI_CAPAB_REG_SIZE, err);
> + if (ret < 0) {
> + return;
> + }
>
> /* set up MMIO */
> memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 8259d67..41430766 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -47,6 +47,7 @@
> #include "e1000e_core.h"
>
> #include "trace.h"
> +#include "qapi/error.h"
>
> #define TYPE_E1000E "e1000e"
> #define E1000E(obj) OBJECT_CHECK(E1000EState, (obj), TYPE_E1000E)
> @@ -372,7 +373,9 @@ e1000e_gen_dsn(uint8_t *mac)
> static int
> e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
> {
> - int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
> + Error *local_err = NULL;
> + int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
> + PCI_PM_SIZEOF, &local_err);
>
> if (ret > 0) {
> pci_set_word(pdev->config + offset + PCI_PM_PMC,
> @@ -386,6 +389,8 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
>
> pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
> PCI_PM_CTRL_PME_STATUS);
> + } else {
> + error_report_err(local_err);
> }
I suggest this instead:
int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
PCI_PM_SIZEOF, &local_err);
if (local_err) {
error_report_err(local_err);
return ret;
}
pci_set_word(...);
pci_set_word(...);
pci_set_word(...);
return ret;
>
> return ret;
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 62e989c..0625839 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -48,6 +48,7 @@
> #include "sysemu/sysemu.h"
> #include "sysemu/dma.h"
> #include "qemu/bitops.h"
> +#include "qapi/error.h"
>
> /* QEMU sends frames smaller than 60 bytes to ethernet nics.
> * Such frames are rejected by real nics and their emulations.
> @@ -494,7 +495,7 @@ static void eepro100_fcp_interrupt(EEPRO100State *s)
> }
> #endif
>
> -static void e100_pci_reset(EEPRO100State *s)
> +static void e100_pci_reset(EEPRO100State *s, Error **errp)
> {
> E100PCIDeviceInfo *info = eepro100_get_class(s);
> uint32_t device = s->device;
> @@ -570,9 +571,13 @@ static void e100_pci_reset(EEPRO100State *s)
> /* Power Management Capabilities */
> int cfg_offset = 0xdc;
> int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
> - cfg_offset, PCI_PM_SIZEOF);
> - assert(r > 0);
> - pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> + cfg_offset, PCI_PM_SIZEOF,
> + errp);
> + if (r > 0) {
> + pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> + } else {
> + return;
> + }
I suggest this instead:
int r = pci_add_capability(..., errp);
if (r < 0) { [*]
return;
}
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
...
Or, even better, as this function is very long:
Error *local_err = NULL;
pci_add_capability(..., &local_err);
if (local_err) {
goto out;
}
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
...
out:
error_propagate(errp, local_err);
return;
> #if 0 /* TODO: replace dummy code for power management emulation. */
> /* TODO: Power Management Control / Status. */
> pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
> @@ -1858,12 +1863,17 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
> {
> EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
> E100PCIDeviceInfo *info = eepro100_get_class(s);
> + Error *local_err = NULL;
>
> TRACE(OTHER, logout("\n"));
>
> s->device = info->device;
>
> - e100_pci_reset(s);
> + e100_pci_reset(s, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
>
> /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
> * i82559 and later support 64 or 256 word EEPROM. */
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index 2404e7e..2c065c3 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -44,6 +44,7 @@
> #include "qemu/osdep.h"
> #include "hw/pci/pci.h"
> #include "hw/i386/ich9.h"
> +#include "qapi/error.h"
>
>
> /*****************************************************************************/
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b73bfea..2bba37a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
> * in pci config space
> */
> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size)
> + uint8_t offset, uint8_t size,
> + Error **errp)
> {
> int ret;
> - Error *local_err = NULL;
>
> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
> - if (ret < 0) {
> - error_report_err(local_err);
> - }
> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
> +
> return ret;
> }
pci_add_capability() and pci_add_capability2() now do exactly the
same, why are both being kept? I suggest replacing
pci_add_capability2() with pci_add_capability() everywhere (on a
separate patch).
>
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 5118ef4..bb0f3a3 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -33,6 +33,7 @@
> #include "hw/pci/pci_bridge.h"
> #include "hw/pci/pci_bus.h"
> #include "qemu/range.h"
> +#include "qapi/error.h"
>
> /* PCI bridge subsystem vendor ID helper functions */
> #define PCI_SSVID_SIZEOF 8
> @@ -43,8 +44,12 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
> uint16_t svid, uint16_t ssid)
> {
> int pos;
> - pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, PCI_SSVID_SIZEOF);
> + Error *local_err = NULL;
> +
> + pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
> + PCI_SSVID_SIZEOF, &local_err);
> if (pos < 0) {
[*]
> + error_report_err(local_err);
> return pos;
> }
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 18e634f..f187512 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -91,11 +91,14 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
> /* PCIe cap v2 init */
> int pos;
> uint8_t *exp_cap;
> + Error *local_err = NULL;
>
> assert(pci_is_express(dev));
>
> - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF);
> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> + PCI_EXP_VER2_SIZEOF, &local_err);
> if (pos < 0) {
[*]
> + error_report_err(local_err);
> return pos;
> }
> dev->exp.exp_cap = pos;
> @@ -123,11 +126,14 @@ int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
> {
> /* PCIe cap v1 init */
> int pos;
> + Error *local_err = NULL;
>
> assert(pci_is_express(dev));
>
> - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER1_SIZEOF);
> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> + PCI_EXP_VER1_SIZEOF, &local_err);
> if (pos < 0) {
[*]
> + error_report_err(local_err);
> return pos;
> }
> dev->exp.exp_cap = pos;
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 42fafac..d72d5e4 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -450,9 +450,12 @@ static int shpc_cap_add_config(PCIDevice *d)
> {
> uint8_t *config;
> int config_offset;
> + Error *local_err = NULL;
> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
> - 0, SHPC_CAP_LENGTH);
> + 0, SHPC_CAP_LENGTH,
> + &local_err);
> if (config_offset < 0) {
[*]
> + error_report_err(local_err);
> return config_offset;
> }
> config = d->config + config_offset;
> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
> index aec1e91..bdca205 100644
> --- a/hw/pci/slotid_cap.c
> +++ b/hw/pci/slotid_cap.c
> @@ -2,6 +2,7 @@
> #include "hw/pci/slotid_cap.h"
> #include "hw/pci/pci.h"
> #include "qemu/error-report.h"
> +#include "qapi/error.h"
>
> #define SLOTID_CAP_LENGTH 4
> #define SLOTID_NSLOTS_SHIFT ctz32(PCI_SID_ESR_NSLOTS)
> @@ -11,6 +12,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
> unsigned offset)
> {
> int cap;
> + Error *local_err = NULL;
> +
> if (!chassis) {
> error_report("Bridge chassis not specified. Each bridge is required "
> "to be assigned a unique chassis id > 0.");
> @@ -21,8 +24,10 @@ int slotid_cap_init(PCIDevice *d, int nslots,
> return -EINVAL;
> }
>
> - cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, SLOTID_CAP_LENGTH);
> + cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
> + SLOTID_CAP_LENGTH, &local_err);
> if (cap < 0) {
[*]
> + error_report_err(local_err);
> return cap;
> }
> /* We make each chassis unique, this way each bridge is First in Chassis */
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 5881968..85cfe38 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1743,7 +1743,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
> PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
> }
>
> - pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
> + pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
> + errp);
> if (pos > 0) {
> vdev->pdev.exp.exp_cap = pos;
> }
I would this instead:
pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
errp);
if (pos < 0) { [*]
return pos;
}
vdev->pdev.exp.exp_cap = pos;
...
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..cca5276 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1161,9 +1161,14 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
> {
> PCIDevice *dev = &proxy->pci_dev;
> int offset;
> + Error *local_err = NULL;
>
> - offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
> - assert(offset > 0);
> + offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
> + cap->cap_len, &local_err);
> + if (offset < 0) {
> + error_report_err(local_err);
> + abort();
> + }
>
This can be simplified to:
offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
cap->cap_len, &error_abort);
> assert(cap->cap_len >= sizeof *cap);
> memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> @@ -1810,9 +1815,13 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> pos = pcie_endpoint_cap_init(pci_dev, 0);
> assert(pos > 0);
>
> - pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
> - assert(pos > 0);
> - pci_dev->exp.pm_cap = pos;
> + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
> + PCI_PM_SIZEOF, errp);
> + if (pos > 0) {
> + pci_dev->exp.pm_cap = pos;
> + } else {
> + return;
> + }
>
I suggest:
pos = pci_add_capability(...)
if (pos < 0) {
return;
}
pci_dev->exp.pm_cap = pos;
> /*
> * Indicates that this function complies with revision 1.2 of the
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5..fe52aa8 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -356,7 +356,8 @@ void pci_unregister_vga(PCIDevice *pci_dev);
> pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
>
> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size);
> + uint8_t offset, uint8_t size,
> + Error **errp);
> int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
> uint8_t offset, uint8_t size,
> Error **errp);
> --
> 2.9.3
>
>
>
--
Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
>> Add Error argument for pci_add_capability() to leverage the errp
>> to pass info on errors. This way is helpful for its callers to
>> make a better error handling when moving to 'realize'.
>>
>> Cc: pbonzini@redhat.com
>> Cc: rth@twiddle.net
>> Cc: ehabkost@redhat.com
>> Cc: mst@redhat.com
>> CC: dmitry@daynix.com
>> Cc: jasowang@redhat.com
>> Cc: marcel@redhat.com
>> Cc: alex.williamson@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
>> hw/net/e1000e.c | 7 ++++++-
>> hw/net/eepro100.c | 20 +++++++++++++++-----
>> hw/pci-bridge/i82801b11.c | 1 +
>> hw/pci/pci.c | 10 ++++------
>> hw/pci/pci_bridge.c | 7 ++++++-
>> hw/pci/pcie.c | 10 ++++++++--
>> hw/pci/shpc.c | 5 ++++-
>> hw/pci/slotid_cap.c | 7 ++++++-
>> hw/vfio/pci.c | 3 ++-
>> hw/virtio/virtio-pci.c | 19 ++++++++++++++-----
>> include/hw/pci/pci.h | 3 ++-
>> 12 files changed, 85 insertions(+), 31 deletions(-)
>
>
> There are multiple places below that checks for errors like this:
>
> function(...);
> if (function succeeded) {
> /* non-error code path here */
> foo = bar;
> }
>
> Sometimes it even includes another branch for the error path:
>
> function(...);
> if (function succeeded) {
> /* non-error code path here */
> foo = bar;
> } else {
> /* error path here */
> return ret;
> }
>
> I suggest doing this instead, for readability:
>
> function(...)
> if (function failed) {
> return ...; /* or: "goto out" */
> }
>
> /* non-error code path here */
> foo = bar;
Yes, please.
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 7b6d4ea..d93ffc2 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
>> x86_iommu->type = TYPE_AMD;
>> qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
>> object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
>> - s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
>> - AMDVI_CAPAB_SIZE);
>> - assert(s->capab_offset > 0);
>> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
>> - assert(ret > 0);
>> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
>> - assert(ret > 0);
>> + ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
>> + AMDVI_CAPAB_SIZE, err);
>> + if (ret < 0) {
>> + return;
>
> Maybe adding a local_err variable is preferred instead of
> checking for (pos < 0), but I'm not sure it's necessary.
>
> Markus, what's the recommendation on those cases? Should we use
> the negative return value to avoid adding an extra local_err
> variable, or should we add local_err anyway to match the existing
> style elsewhere?
Opinions and practice vary on this one.
I prefer checking the return value because it lets me avoid the
error_propagate() boiler-plate more often.
Having both an Error parameter and an error return value poses the
question whether the two agree.
You can assert they do, but it's distracting. We generally don't.
When there's no success value to transmit, you avoid the problem by
making the function return void. We used to favor that, but it has
turned out not to be a success, because it leads to cumbersome code.
For what it's worth, GLib wants you to transmit success / failure in the
return value, too:
https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html#gerror-rules
[...]
Hi, Eduardo
On 06/06/2017 10:52 PM, Eduardo Habkost wrote:
> On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
>> Add Error argument for pci_add_capability() to leverage the errp
>> to pass info on errors. This way is helpful for its callers to
>> make a better error handling when moving to 'realize'.
>>
>> Cc: pbonzini@redhat.com
>> Cc: rth@twiddle.net
>> Cc: ehabkost@redhat.com
>> Cc: mst@redhat.com
>> CC: dmitry@daynix.com
>> Cc: jasowang@redhat.com
>> Cc: marcel@redhat.com
>> Cc: alex.williamson@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
[...]
>
>
> There are multiple places below that checks for errors like this:
>
> function(...);
> if (function succeeded) {
> /* non-error code path here */
> foo = bar;
> }
>
> Sometimes it even includes another branch for the error path:
>
> function(...);
> if (function succeeded) {
> /* non-error code path here */
> foo = bar;
> } else {
> /* error path here */
> return ret;
> }
>
> I suggest doing this instead, for readability:
>
> function(...)
> if (function failed) {
> return ...; /* or: "goto out" */
> }
>
> /* non-error code path here */
> foo = bar;
>
Thank you very much for the detailed explanation，will use
this more elegant way to check return value in next version. :)
[...]
>> static int
>> e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
>> {
>> - int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
>> + Error *local_err = NULL;
>> + int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
>> + PCI_PM_SIZEOF, &local_err);
>>
>> if (ret > 0) {
>> pci_set_word(pdev->config + offset + PCI_PM_PMC,
>> @@ -386,6 +389,8 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
>>
>> pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
>> PCI_PM_CTRL_PME_STATUS);
>> + } else {
>> + error_report_err(local_err);
>> }
>
>
> I suggest this instead:
>
> int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
> PCI_PM_SIZEOF, &local_err);
> if (local_err) {
> error_report_err(local_err);
> return ret;
> }
>
> pci_set_word(...);
> pci_set_word(...);
> pci_set_word(...);
> return ret;
>
OK, I see.
>
>>
>>
>> /*****************************************************************************/
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index b73bfea..2bba37a 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
>> * in pci config space
>> */
>> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>> - uint8_t offset, uint8_t size)
>> + uint8_t offset, uint8_t size,
>> + Error **errp)
>> {
>> int ret;
>> - Error *local_err = NULL;
>>
>> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
>> - if (ret < 0) {
>> - error_report_err(local_err);
>> - }
>> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
>> +
>> return ret;
>> }
>
> pci_add_capability() and pci_add_capability2() now do exactly the
> same, why are both being kept? I suggest replacing
> pci_add_capability2() with pci_add_capability() everywhere (on a
> separate patch).
>
Completely remove pci_add_capability and direct use pci_add_capability2()
everywhere is it a more thorough way?
Thanks
Mao
Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
> Hi, Eduardo
>
> On 06/06/2017 10:52 PM, Eduardo Habkost wrote:
>> On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
>>> Add Error argument for pci_add_capability() to leverage the errp
>>> to pass info on errors. This way is helpful for its callers to
>>> make a better error handling when moving to 'realize'.
[...]
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index b73bfea..2bba37a 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
>>> * in pci config space
>>> */
>>> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>>> - uint8_t offset, uint8_t size)
>>> + uint8_t offset, uint8_t size,
>>> + Error **errp)
>>> {
>>> int ret;
>>> - Error *local_err = NULL;
>>>
>>> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
>>> - if (ret < 0) {
>>> - error_report_err(local_err);
>>> - }
>>> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
>>> +
>>> return ret;
>>> }
>>
>> pci_add_capability() and pci_add_capability2() now do exactly the
>> same, why are both being kept? I suggest replacing
>> pci_add_capability2() with pci_add_capability() everywhere (on a
>> separate patch).
>>
>
> Completely remove pci_add_capability and direct use pci_add_capability2()
> everywhere is it a more thorough way?
You're converting pci_add_capability() to Error because you need the
Error for your conversions to realize().
I recommend to change the calls where you need the Error (and only
these) to call pci_add_capability2() instead.
When no calls to pci_add_capability() remain, we remove it. If that
becomes the case in your series, you remove it.
Okay?
Hi, Markus
On 06/07/2017 03:05 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Hi, Eduardo
>>
>> On 06/06/2017 10:52 PM, Eduardo Habkost wrote:
>>> On Tue, Jun 06, 2017 at 07:26:30PM +0800, Mao Zhongyi wrote:
>>>> Add Error argument for pci_add_capability() to leverage the errp
>>>> to pass info on errors. This way is helpful for its callers to
>>>> make a better error handling when moving to 'realize'.
> [...]
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index b73bfea..2bba37a 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
>>>> * in pci config space
>>>> */
>>>> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>>>> - uint8_t offset, uint8_t size)
>>>> + uint8_t offset, uint8_t size,
>>>> + Error **errp)
>>>> {
>>>> int ret;
>>>> - Error *local_err = NULL;
>>>>
>>>> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
>>>> - if (ret < 0) {
>>>> - error_report_err(local_err);
>>>> - }
>>>> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
>>>> +
>>>> return ret;
>>>> }
>>>
>>> pci_add_capability() and pci_add_capability2() now do exactly the
>>> same, why are both being kept? I suggest replacing
>>> pci_add_capability2() with pci_add_capability() everywhere (on a
>>> separate patch).
>>>
>>
>> Completely remove pci_add_capability and direct use pci_add_capability2()
>> everywhere is it a more thorough way?
>
> You're converting pci_add_capability() to Error because you need the
> Error for your conversions to realize().
it's true.
>
> I recommend to change the calls where you need the Error (and only
> these) to call pci_add_capability2() instead.
>
> When no calls to pci_add_capability() remain, we remove it. If that
> becomes the case in your series, you remove it.
>
> Okay?
This is a gentle way of doing it. After read the code I found only
parts need to be replaced by pci_add_capability2() in my series as
follow your advice, this is no problem. But it means that the remaining
replacement will be reworked in the future, although it can be fixed
absolutely in a separate patch now. Of course, this is just my own
opinion, consider the reason for git history I would rather hear your
advice. :)
Thanks
Mao
© 2016 - 2025 Red Hat, Inc.