Hi Markus, On 22/09/2016 18:52, Markus Armbruster wrote: > Eric Auger <eric.au...@redhat.com> writes: > >> Pass an error object to prepare for migration to VFIO-PCI realize. >> The error is cascaded downto vfio_add_std_cap and then vfio_msi(x)_setup, >> vfio_setup_pcie_cap. >> >> vfio_add_ext_cap does not return anything else than 0 so let's transform >> it into a void function. >> >> Also use pci_add_capability2 which takes an error object. >> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> --- >> hw/vfio/pci.c | 66 >> ++++++++++++++++++++++++++++++++--------------------------- >> 1 file changed, 36 insertions(+), 30 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index f67eec4..07a44f5 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1178,7 +1178,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice >> *vdev) >> } >> } >> >> -static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) >> +static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp) >> { >> uint16_t ctrl; >> bool msi_64bit, msi_maskbit; > int ret, entries; > Error *err = NULL; > > if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), > vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { > return -errno; > } > > Fails without setting error here. Sure > >> @@ -1202,8 +1202,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) >> if (ret == -ENOTSUP) { >> return 0; >> } >> - error_prepend(&err, "vfio: msi_init failed: "); >> - error_report_err(err); >> + error_propagate(errp, err); >> return ret; >> } >> vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : >> 0); >> @@ -1361,7 +1360,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, >> Error **errp) >> return 0; >> } >> >> -static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) >> +static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) >> { >> int ret; >> >> @@ -1376,7 +1375,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int >> pos) >> if (ret == -ENOTSUP) { >> return 0; >> } >> - error_report("vfio: msix_init failed"); >> + error_setg(errp, "msix_init failed"); >> return ret; >> } >> >> @@ -1561,7 +1560,8 @@ static void vfio_add_emulated_long(VFIOPCIDevice >> *vdev, int pos, >> vfio_set_long_bits(vdev->emulated_config_bits + pos, mask, mask); >> } >> >> -static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size) >> +static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, >> + Error **errp) >> { >> uint16_t flags; >> uint8_t type; >> @@ -1573,8 +1573,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, >> int pos, uint8_t size) >> type != PCI_EXP_TYPE_LEG_END && >> type != PCI_EXP_TYPE_RC_END) { >> >> - error_report("vfio: Assignment of PCIe type 0x%x " >> - "devices is not currently supported", type); >> + error_setg(errp, "assignment of PCIe type 0x%x " >> + "devices is not currently supported", type); >> return -EINVAL; >> } >> >> @@ -1708,10 +1708,11 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, >> uint8_t pos) >> } >> } >> >> -static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) >> +static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) >> { >> PCIDevice *pdev = &vdev->pdev; >> uint8_t cap_id, next, size; >> + Error *err = NULL; >> int ret; >> >> cap_id = pdev->config[pos]; >> @@ -1733,9 +1734,9 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, >> uint8_t pos) >> * will be changed as we unwind the stack. >> */ >> if (next) { >> - ret = vfio_add_std_cap(vdev, next); >> + ret = vfio_add_std_cap(vdev, next, &err); >> if (ret) { >> - return ret; >> + goto out; >> } >> } else { >> /* Begin the rebuild, use QEMU emulated list bits */ >> @@ -1749,40 +1750,42 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, >> uint8_t pos) >> >> switch (cap_id) { >> case PCI_CAP_ID_MSI: >> - ret = vfio_msi_setup(vdev, pos); >> + ret = vfio_msi_setup(vdev, pos, &err); >> break; >> case PCI_CAP_ID_EXP: >> vfio_check_pcie_flr(vdev, pos); >> - ret = vfio_setup_pcie_cap(vdev, pos, size); >> + ret = vfio_setup_pcie_cap(vdev, pos, size, &err); >> break; >> case PCI_CAP_ID_MSIX: >> - ret = vfio_msix_setup(vdev, pos); >> + ret = vfio_msix_setup(vdev, pos, &err); >> break; >> case PCI_CAP_ID_PM: >> vfio_check_pm_reset(vdev, pos); >> vdev->pm_cap = pos; >> - ret = pci_add_capability(pdev, cap_id, pos, size); >> + ret = pci_add_capability2(pdev, cap_id, pos, size, &err); >> break; >> case PCI_CAP_ID_AF: >> vfio_check_af_flr(vdev, pos); >> - ret = pci_add_capability(pdev, cap_id, pos, size); >> + ret = pci_add_capability2(pdev, cap_id, pos, size, &err); >> break; >> default: >> - ret = pci_add_capability(pdev, cap_id, pos, size); >> + ret = pci_add_capability2(pdev, cap_id, pos, size, &err); >> break; >> } >> >> - if (ret < 0) { >> - error_report("vfio: %s Error adding PCI capability " >> - "0x%x[0x%x]@0x%x: %d", vdev->vbasedev.name, >> - cap_id, size, pos, ret); >> - return ret; >> +out: >> + error_propagate(errp, err); >> + if (*errp) { >> + error_prepend(errp, >> + "failed to add PCI capability 0x%x[0x%x]@0x%x: ", >> + cap_id, size, pos); >> } > > Less verbose, and therefore recommended by the big comment in error.h is > using errp instead of &err, then OK thanks for reminding me of this guideline. > > out: > if (ret < 0) { > error_prepend(errp, > "failed to add PCI capability 0x%x[0x%x]@0x%x: ", > cap_id, size, pos); > } > > Adding the error_propagate() now anyway can make sense when you get rid > of ret later in the series. Yes. So I dared to keep it as is and added a comment in the commit message.
> >> >> - return 0; >> + return ret < 0 ? ret : 0; >> + > > Spurious blank line. OK > >> } >> >> -static int vfio_add_ext_cap(VFIOPCIDevice *vdev) >> +static void vfio_add_ext_cap(VFIOPCIDevice *vdev) >> { >> PCIDevice *pdev = &vdev->pdev; >> uint32_t header; >> @@ -1793,7 +1796,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev) >> /* Only add extended caps if we have them and the guest can see them */ >> if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) || >> !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) { >> - return 0; >> + return; >> } >> >> /* >> @@ -1858,12 +1861,13 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev) >> } >> >> g_free(config); >> - return 0; >> + return; >> } >> >> -static int vfio_add_capabilities(VFIOPCIDevice *vdev) >> +static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp) >> { >> PCIDevice *pdev = &vdev->pdev; >> + Error *err = NULL; >> int ret; >> >> if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) || >> @@ -1871,12 +1875,14 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev) >> return 0; /* Nothing to add */ >> } >> >> - ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]); >> + ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], &err); >> if (ret) { >> + error_propagate(errp, err); >> return ret; >> } > > Likewise, as long as you have (and intend to keep) ret, do > > ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp); > if (ret) { > return ret; > } Agreed Thanks Eric > >> >> - return vfio_add_ext_cap(vdev); >> + vfio_add_ext_cap(vdev); >> + return 0; >> } >> >> static void vfio_pci_pre_reset(VFIOPCIDevice *vdev) >> @@ -2680,7 +2686,7 @@ static int vfio_initfn(PCIDevice *pdev) >> >> vfio_bars_setup(vdev); >> >> - ret = vfio_add_capabilities(vdev); >> + ret = vfio_add_capabilities(vdev, &err); >> if (ret) { >> goto out_teardown; >> } >