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

[Eduardo Habkost]

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>
> ---
>  hw/i386/amd_iommu.c       | 24 +++++++++++++++++-------
>  hw/net/e1000e.c           |  7 ++++++-
>  hw/net/eepro100.c         | 20 +++++++++++++++-----
>  hw/pci-bridge/i82801b11.c |  1 +
>  hw/pci/pci.c              | 10 ++++------
>  hw/pci/pci_bridge.c       |  7 ++++++-
>  hw/pci/pcie.c             | 10 ++++++++--
>  hw/pci/shpc.c             |  5 ++++-
>  hw/pci/slotid_cap.c       |  7 ++++++-
>  hw/vfio/pci.c             |  3 ++-
>  hw/virtio/virtio-pci.c    | 19 ++++++++++++++-----
>  include/hw/pci/pci.h      |  3 ++-
>  12 files changed, 85 insertions(+), 31 deletions(-)


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;



> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 7b6d4ea..d93ffc2 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error 
> **err)
>      x86_iommu->type = TYPE_AMD;
>      qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
>      object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
> -    s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> -                                         AMDVI_CAPAB_SIZE);
> -    assert(s->capab_offset > 0);
> -    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, 
> AMDVI_CAPAB_REG_SIZE);
> -    assert(ret > 0);
> -    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, 
> AMDVI_CAPAB_REG_SIZE);
> -    assert(ret > 0);
> +    ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> +                                         AMDVI_CAPAB_SIZE, err);
> +    if (ret < 0) {
> +        return;

Maybe adding a local_err variable is preferred instead of
checking for (pos < 0), but I'm not sure it's necessary.

Markus, what's the recommendation on those cases?  Should we use
the negative return value to avoid adding an extra local_err
variable, or should we add local_err anyway to match the existing
style elsewhere?

(The same applies to other functions below [*])

> +    }
> +    s->capab_offset = ret;
> +
> +    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
> +                             AMDVI_CAPAB_REG_SIZE, err);
> +    if (ret < 0) {
> +        return;
> +    }
> +    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
> +                             AMDVI_CAPAB_REG_SIZE, err);
> +    if (ret < 0) {
> +        return;
> +    }
>  
>      /* set up MMIO */
>      memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, 
> "amdvi-mmio",
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 8259d67..41430766 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -47,6 +47,7 @@
>  #include "e1000e_core.h"
>  
>  #include "trace.h"
> +#include "qapi/error.h"
>  
>  #define TYPE_E1000E "e1000e"
>  #define E1000E(obj) OBJECT_CHECK(E1000EState, (obj), TYPE_E1000E)
> @@ -372,7 +373,9 @@ e1000e_gen_dsn(uint8_t *mac)
>  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;


>  
>      return ret;
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 62e989c..0625839 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -48,6 +48,7 @@
>  #include "sysemu/sysemu.h"
>  #include "sysemu/dma.h"
>  #include "qemu/bitops.h"
> +#include "qapi/error.h"
>  
>  /* QEMU sends frames smaller than 60 bytes to ethernet nics.
>   * Such frames are rejected by real nics and their emulations.
> @@ -494,7 +495,7 @@ static void eepro100_fcp_interrupt(EEPRO100State *s)
>  }
>  #endif
>  
> -static void e100_pci_reset(EEPRO100State *s)
> +static void e100_pci_reset(EEPRO100State *s, Error **errp)
>  {
>      E100PCIDeviceInfo *info = eepro100_get_class(s);
>      uint32_t device = s->device;
> @@ -570,9 +571,13 @@ static void e100_pci_reset(EEPRO100State *s)
>          /* Power Management Capabilities */
>          int cfg_offset = 0xdc;
>          int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
> -                                   cfg_offset, PCI_PM_SIZEOF);
> -        assert(r > 0);
> -        pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> +                                   cfg_offset, PCI_PM_SIZEOF,
> +                                   errp);
> +        if (r > 0) {
> +            pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> +        } else {
> +            return;
> +        }

I suggest this instead:

    int r = pci_add_capability(..., errp);

    if (r < 0) {  [*]
        return;
    }

    pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
    ...


Or, even better, as this function is very long:

    Error *local_err = NULL;

    pci_add_capability(..., &local_err);
    if (local_err) {
        goto out;
    }

    pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
    ...

out:
    error_propagate(errp, local_err);
    return;

>  #if 0 /* TODO: replace dummy code for power management emulation. */
>          /* TODO: Power Management Control / Status. */
>          pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
> @@ -1858,12 +1863,17 @@ static void e100_nic_realize(PCIDevice *pci_dev, 
> Error **errp)
>  {
>      EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
>      E100PCIDeviceInfo *info = eepro100_get_class(s);
> +    Error *local_err = NULL;
>  
>      TRACE(OTHER, logout("\n"));
>  
>      s->device = info->device;
>  
> -    e100_pci_reset(s);
> +    e100_pci_reset(s, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
>       * i82559 and later support 64 or 256 word EEPROM. */
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index 2404e7e..2c065c3 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -44,6 +44,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/pci/pci.h"
>  #include "hw/i386/ich9.h"
> +#include "qapi/error.h"
>  
>  
>  
> /*****************************************************************************/
> 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).


>  
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 5118ef4..bb0f3a3 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -33,6 +33,7 @@
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
>  #include "qemu/range.h"
> +#include "qapi/error.h"
>  
>  /* PCI bridge subsystem vendor ID helper functions */
>  #define PCI_SSVID_SIZEOF        8
> @@ -43,8 +44,12 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
>                            uint16_t svid, uint16_t ssid)
>  {
>      int pos;
> -    pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, 
> PCI_SSVID_SIZEOF);
> +    Error *local_err = NULL;
> +
> +    pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
> +                             PCI_SSVID_SIZEOF, &local_err);
>      if (pos < 0) {

[*]

> +        error_report_err(local_err);
>          return pos;
>      }
>  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 18e634f..f187512 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -91,11 +91,14 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t 
> type, uint8_t port)
>      /* PCIe cap v2 init */
>      int pos;
>      uint8_t *exp_cap;
> +    Error *local_err = NULL;
>  
>      assert(pci_is_express(dev));
>  
> -    pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, 
> PCI_EXP_VER2_SIZEOF);
> +    pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> +                             PCI_EXP_VER2_SIZEOF, &local_err);
>      if (pos < 0) {

[*]

> +        error_report_err(local_err);
>          return pos;
>      }
>      dev->exp.exp_cap = pos;
> @@ -123,11 +126,14 @@ int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, 
> uint8_t type,
>  {
>      /* PCIe cap v1 init */
>      int pos;
> +    Error *local_err = NULL;
>  
>      assert(pci_is_express(dev));
>  
> -    pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, 
> PCI_EXP_VER1_SIZEOF);
> +    pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> +                             PCI_EXP_VER1_SIZEOF, &local_err);
>      if (pos < 0) {

[*]

> +        error_report_err(local_err);
>          return pos;
>      }
>      dev->exp.exp_cap = pos;
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 42fafac..d72d5e4 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -450,9 +450,12 @@ static int shpc_cap_add_config(PCIDevice *d)
>  {
>      uint8_t *config;
>      int config_offset;
> +    Error *local_err = NULL;
>      config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
> -                                       0, SHPC_CAP_LENGTH);
> +                                       0, SHPC_CAP_LENGTH,
> +                                       &local_err);
>      if (config_offset < 0) {

[*]

> +        error_report_err(local_err);
>          return config_offset;
>      }
>      config = d->config + config_offset;
> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
> index aec1e91..bdca205 100644
> --- a/hw/pci/slotid_cap.c
> +++ b/hw/pci/slotid_cap.c
> @@ -2,6 +2,7 @@
>  #include "hw/pci/slotid_cap.h"
>  #include "hw/pci/pci.h"
>  #include "qemu/error-report.h"
> +#include "qapi/error.h"
>  
>  #define SLOTID_CAP_LENGTH 4
>  #define SLOTID_NSLOTS_SHIFT ctz32(PCI_SID_ESR_NSLOTS)
> @@ -11,6 +12,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
>                      unsigned offset)
>  {
>      int cap;
> +    Error *local_err = NULL;
> +
>      if (!chassis) {
>          error_report("Bridge chassis not specified. Each bridge is required "
>                       "to be assigned a unique chassis id > 0.");
> @@ -21,8 +24,10 @@ int slotid_cap_init(PCIDevice *d, int nslots,
>          return -EINVAL;
>      }
>  
> -    cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, 
> SLOTID_CAP_LENGTH);
> +    cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
> +                             SLOTID_CAP_LENGTH, &local_err);
>      if (cap < 0) {

[*]

> +        error_report_err(local_err);
>          return cap;
>      }
>      /* We make each chassis unique, this way each bridge is First in Chassis 
> */
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 5881968..85cfe38 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1743,7 +1743,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
> pos, uint8_t size,
>                                 PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
>      }
>  
> -    pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
> +    pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
> +                             errp);
>      if (pos > 0) {
>          vdev->pdev.exp.exp_cap = pos;
>      }

I would this instead:

    pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
                             errp);
    if (pos < 0) {   [*]
        return pos;
    }
    vdev->pdev.exp.exp_cap = pos;
    ...

> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..cca5276 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1161,9 +1161,14 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy 
> *proxy,
>  {
>      PCIDevice *dev = &proxy->pci_dev;
>      int offset;
> +    Error *local_err = NULL;
>  
> -    offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
> -    assert(offset > 0);
> +    offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
> +                                cap->cap_len, &local_err);
> +    if (offset < 0) {
> +        error_report_err(local_err);
> +        abort();
> +    }
>  

This can be simplified to:

    offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
                                cap->cap_len, &error_abort);

>      assert(cap->cap_len >= sizeof *cap);
>      memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> @@ -1810,9 +1815,13 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
> Error **errp)
>          pos = pcie_endpoint_cap_init(pci_dev, 0);
>          assert(pos > 0);
>  
> -        pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
> -        assert(pos > 0);
> -        pci_dev->exp.pm_cap = pos;
> +        pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
> +                                 PCI_PM_SIZEOF, errp);
> +        if (pos > 0) {
> +            pci_dev->exp.pm_cap = pos;
> +        } else {
> +            return;
> +        }
>  


I suggest:

    pos = pci_add_capability(...)
    if (pos < 0) {
        return;
    }

    pci_dev->exp.pm_cap = pos;

>          /*
>           * Indicates that this function complies with revision 1.2 of the
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5..fe52aa8 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -356,7 +356,8 @@ void pci_unregister_vga(PCIDevice *pci_dev);
>  pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
>  
>  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 pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
>                         uint8_t offset, uint8_t size,
>                         Error **errp);
> -- 
> 2.9.3
> 
> 
> 

-- 
Eduardo