|
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 17:33:16 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
Hi, Markus On 06/07/2017 03:05 PM, Markus Armbruster wrote:
Mao Zhongyi <address@hidden> 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
[Prev in Thread] | Current Thread | [Next in Thread] |