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: pbonz...@redhat.com
Cc: r...@twiddle.net
Cc: ehabk...@redhat.com
Cc: m...@redhat.com
CC: dmi...@daynix.com
Cc: jasow...@redhat.com
Cc: mar...@redhat.com
Cc: alex.william...@redhat.com
Cc: arm...@redhat.com
Signed-off-by: Mao Zhongyi <maozy.f...@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
