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

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

[Mao Zhongyi]

The return value of pci_add_capability2() is only 2 cases, positive
on success, nagetive on failure and set error message to Error. In
other worlds, If Error is filled, the return value must be nagetive.
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.

Meanwhile, all other callers of pci_add_capability2() do the same
check as this patch. So fix it.

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;
 }
-- 
2.9.3

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

[Markus Armbruster]

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.

> 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().

> 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.

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

[Mao Zhongyi]

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.

Thanks
Mao

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

[Marcel Apfelbaum]

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
>
>
>