Re: [Qemu-devel] [PATCH] pci: Fix unreasonable return value check

[Marcel Apfelbaum <marcel@redhat.com>]

On 01/06/2017 5:51, Mao Zhongyi wrote:
> Hi, Markus
>
> On 05/31/2017 07:07 PM, Markus Armbruster wrote:
>> This is cleanup, not a bug fix, so:
>>
>>     pci: Clean up error checking in pci_add_capability()
>>
>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>
>>> The return value of pci_add_capability2() is only 2 cases, positive
>>> on success, nagetive on failure and set error message to Error. In
>>
>> negative
>>
>>> other worlds, If Error is filled, the return value must be nagetive.
>>
>> words, if
>>
>>> There is no case where errp is set but the return value is a positive.
>>> But pci_add_capability() does. So the return value check is illogical.
>>
>> pci_add_capability2() could use a function comment explaining its return
>> value.  Not this patch's job.
>>
>
> Thanks, will make a separated patch to explain it.
>
>>> Meanwhile, all other callers of pci_add_capability2() do the same
>>> check as this patch. So fix it.
>>
>> Suggest:
>>
>>     pci: Clean up error checking in pci_add_capability()
>>
>>   On success, pci_add_capability2() returns a positive value. On
>>   failure, it sets an error and returns a negative value.
>>
>>   pci_add_capability() laboriously checks this behavior.  No other
>>   caller does.  Drop the checks from pci_add_capability().
>
> Thanks for your perfect suggestion.
>
>>
>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>> ---
>>>  hw/pci/pci.c | 6 +-----
>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index 259483b..1faf060 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -2269,12 +2269,8 @@ int pci_add_capability(PCIDevice *pdev, 
>>> uint8_t cap_id,
>>>      Error *local_err = NULL;
>>>
>>>      ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
>>> -    if (local_err) {
>>> -        assert(ret < 0);
>>> +    if (ret < 0) {
>>>          error_report_err(local_err);
>>> -    } else {
>>> -        /* success implies a positive offset in config space */
>>> -        assert(ret > 0);
>>>      }
>>>      return ret;
>>>  }
>>
>> Many functions return distinct error values in addition to setting an
>> error.  We usually check one of the two, and assume the other is sane.
>> This is one of the few places where we assert it is.  Not wrong, just
>> cumbersome.  I'd prefer to drop the assertions, i.e. take this patch.
>> But it's up to the PCI maintainers.
>
> Yes, I also think it really is not necessary. Keeping code as simple as
> practical is desirable. So drop the assertions. Of course, I will listen
> to the views of Marcel and Michael.
>
I have nothing against it.
Thanks for the patch!

With Markus comments:
     Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

> Thanks
> Mao
>
>
>