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 <address@hidden> 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 <address@hidden>
> > ---
> >  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