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

[Markus Armbruster <armbru@redhat.com>]

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.