|
From: | Mao Zhongyi |
Subject: | Re: [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability() |
Date: | Wed, 7 Jun 2017 13:33:51 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
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: address@hidden Cc: address@hidden Cc: address@hidden Cc: address@hidden CC: address@hidden Cc: address@hidden Cc: address@hidden Cc: address@hidden Cc: address@hidden Signed-off-by: Mao Zhongyi <address@hidden> ---
[...]
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
[Prev in Thread] | Current Thread | [Next in Thread] |