Re: [Qemu-devel] [PATCH v3 5/7] pci: Make errp the last parameter of pci_add_capability()

[Mao Zhongyi]

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'.
> >
> > Cc: address@hidden
> > Cc: address@hidden
> > Cc: address@hidden
> > Cc: address@hidden
> > CC: address@hidden
> > Cc: address@hidden
> > Cc: address@hidden
> > Cc: address@hidden
> > Cc: address@hidden
> > Signed-off-by: Mao Zhongyi <address@hidden>
> > ---
[...]
> There are multiple places below that checks for errors like this:
>
>     function(...);
>     if (function succeeded) {
>        /* non-error code path here */
>        foo = bar;
>     }
>
> Sometimes it even includes another branch for the error path:
>
>     function(...);
>     if (function succeeded) {
>        /* non-error code path here */
>        foo = bar;
>     } else {
>        /* error path here */
>        return ret;
>     }
>
> I suggest doing this instead, for readability:
>
>     function(...)
>     if (function failed) {
>        return ...;  /* or: "goto out" */
>     }
>
>     /* non-error code path here */
>     foo = bar;

Thank you very much for the detailed explanation，will use
this more elegant way to check return value in next version. :)


[...]
> >  static int
> >  e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
> >  {
> > -    int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
> > +    Error *local_err = NULL;
> > +    int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
> > +                                 PCI_PM_SIZEOF, &local_err);
> >
> >      if (ret > 0) {
> >          pci_set_word(pdev->config + offset + PCI_PM_PMC,
> > @@ -386,6 +389,8 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, 
> > uint16_t pmc)
> >
> >          pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
> >                       PCI_PM_CTRL_PME_STATUS);
> > +    } else {
> > +        error_report_err(local_err);
> >      }
>
>
> I suggest this instead:
>
>     int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
>                                  PCI_PM_SIZEOF, &local_err);
>     if (local_err) {
>         error_report_err(local_err);
>         return ret;
>     }
>
>     pci_set_word(...);
>     pci_set_word(...);
>     pci_set_word(...);
>     return ret;

OK, I see.

> >  /*****************************************************************************/
> > 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?

Thanks
Mao