Re: [PATCH 13/16] vfio/pci: Make capability related functions return bool

[Cédric Le Goater]

On 5/15/24 10:20, Zhenzhong Duan wrote:

The functions operating on capability don't have a consistent return style.

Below functions are in bool-valued functions style:
vfio_msi_setup()
vfio_msix_setup()
vfio_add_std_cap()
vfio_add_capabilities()

Below two are integer-valued functions:
vfio_add_vendor_specific_cap()
vfio_setup_pcie_cap()

But the returned integer is only used for check succeed/failure.
Change them all to return bool so now all capability related
functions follow the coding standand in qapi/error.h to return
bool.

Signed-off-by: Zhenzhong Duan 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  hw/vfio/pci.c | 77 ---
  1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1922593253..ecfbb9619f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1339,7 +1339,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
  }
  }
  
-static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)

+static bool vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
  {
  uint16_t ctrl;
  bool msi_64bit, msi_maskbit;
@@ -1349,7 +1349,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, 
Error **errp)
  if (pread(vdev->vbasedev.fd, , sizeof(ctrl),
vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
  error_setg_errno(errp, errno, "failed reading MSI PCI_CAP_FLAGS");
-return -errno;
+return false;
  }
  ctrl = le16_to_cpu(ctrl);
  
@@ -1362,14 +1362,14 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)

  ret = msi_init(>pdev, pos, entries, msi_64bit, msi_maskbit, );
  if (ret < 0) {
  if (ret == -ENOTSUP) {
-return 0;
+return true;
  }
  error_propagate_prepend(errp, err, "msi_init failed: ");
-return ret;
+return false;
  }
  vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 
0);
  
-return 0;

+return true;
  }
  
  static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)

@@ -1644,7 +1644,7 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, 
Error **errp)
  return vfio_pci_relocate_msix(vdev, errp);
  }
  
-static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)

+static bool vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
  {
  int ret;
  Error *err = NULL;
@@ -1660,11 +1660,11 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int 
pos, Error **errp)
  if (ret < 0) {
  if (ret == -ENOTSUP) {
  warn_report_err(err);
-return 0;
+return true;
  }
  
  error_propagate(errp, err);

-return ret;
+return false;
  }
  
  /*

@@ -1698,7 +1698,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, 
Error **errp)
  memory_region_set_enabled(>pdev.msix_table_mmio, false);
  }
  
-return 0;

+return true;
  }
  
  static void vfio_teardown_msi(VFIOPCIDevice *vdev)

@@ -1977,8 +1977,8 @@ static void vfio_pci_disable_rp_atomics(VFIOPCIDevice 
*vdev)
  }
  }
  
-static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,

-   Error **errp)
+static bool vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
+Error **errp)
  {
  uint16_t flags;
  uint8_t type;
@@ -1992,7 +1992,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
pos, uint8_t size,
  
  error_setg(errp, "assignment of PCIe type 0x%x "

 "devices is not currently supported", type);
-return -EINVAL;
+return false;
  }
  
  if (!pci_bus_is_express(pci_get_bus(>pdev))) {

@@ -2025,7 +2025,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
pos, uint8_t size,
  }
  
  if (pci_bus_is_express(bus)) {

-return 0;
+return true;
  }
  
  } else if (pci_bus_is_root(pci_get_bus(>pdev))) {

@@ -2063,7 +2063,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
pos, uint8_t size,
   * Legacy endpoints don't belong on the root complex.  Windows
   * seems to be happier with devices if we skip the capability.
   */
-return 0;
+return true;
  }
  
  } else {

@@ -2099,12 +2099,12 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
pos, uint8_t size,
  pos = pci_add_capability(>pdev, PCI_CAP_ID_EXP, pos, size,
   errp);
  if (pos < 0) {
-return pos;
+return false;
  }
  
  vdev->pdev.exp.exp_cap = pos;
  
-return pos;

+return true;
  }
  
  static void vfio_check_pcie_flr(VFIOPCIDevice *vdev, uint8_t pos)

@@ -2137,14 +2137,14 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, 
uint8_t pos)
  }
  }
  

[PATCH 13/16] vfio/pci: Make capability related functions return bool

[Zhenzhong Duan]

The functions operating on capability don't have a consistent return style.

Below functions are in bool-valued functions style:
vfio_msi_setup()
vfio_msix_setup()
vfio_add_std_cap()
vfio_add_capabilities()

Below two are integer-valued functions:
vfio_add_vendor_specific_cap()
vfio_setup_pcie_cap()

But the returned integer is only used for check succeed/failure.
Change them all to return bool so now all capability related
functions follow the coding standand in qapi/error.h to return
bool.

Signed-off-by: Zhenzhong Duan 
---
 hw/vfio/pci.c | 77 ---
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1922593253..ecfbb9619f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1339,7 +1339,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
 }
 }
 
-static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
+static bool vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 {
 uint16_t ctrl;
 bool msi_64bit, msi_maskbit;
@@ -1349,7 +1349,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, 
Error **errp)
 if (pread(vdev->vbasedev.fd, , sizeof(ctrl),
   vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
 error_setg_errno(errp, errno, "failed reading MSI PCI_CAP_FLAGS");
-return -errno;
+return false;
 }
 ctrl = le16_to_cpu(ctrl);
 
@@ -1362,14 +1362,14 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, 
Error **errp)
 ret = msi_init(>pdev, pos, entries, msi_64bit, msi_maskbit, );
 if (ret < 0) {
 if (ret == -ENOTSUP) {
-return 0;
+return true;
 }
 error_propagate_prepend(errp, err, "msi_init failed: ");
-return ret;
+return false;
 }
 vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
 
-return 0;
+return true;
 }
 
 static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)
@@ -1644,7 +1644,7 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, 
Error **errp)
 return vfio_pci_relocate_msix(vdev, errp);
 }
 
-static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
+static bool vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 {
 int ret;
 Error *err = NULL;
@@ -1660,11 +1660,11 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int 
pos, Error **errp)
 if (ret < 0) {
 if (ret == -ENOTSUP) {
 warn_report_err(err);
-return 0;
+return true;
 }
 
 error_propagate(errp, err);
-return ret;
+return false;
 }
 
 /*
@@ -1698,7 +1698,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, 
Error **errp)
 memory_region_set_enabled(>pdev.msix_table_mmio, false);
 }
 
-return 0;
+return true;
 }
 
 static void vfio_teardown_msi(VFIOPCIDevice *vdev)
@@ -1977,8 +1977,8 @@ static void vfio_pci_disable_rp_atomics(VFIOPCIDevice 
*vdev)
 }
 }
 
-static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
-   Error **errp)
+static bool vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
+Error **errp)
 {
 uint16_t flags;
 uint8_t type;
@@ -1992,7 +1992,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
pos, uint8_t size,
 
 error_setg(errp, "assignment of PCIe type 0x%x "
"devices is not currently supported", type);
-return -EINVAL;
+return false;
 }
 
 if (!pci_bus_is_express(pci_get_bus(>pdev))) {
@@ -2025,7 +2025,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
pos, uint8_t size,
 }
 
 if (pci_bus_is_express(bus)) {
-return 0;
+return true;
 }
 
 } else if (pci_bus_is_root(pci_get_bus(>pdev))) {
@@ -2063,7 +2063,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
pos, uint8_t size,
  * Legacy endpoints don't belong on the root complex.  Windows
  * seems to be happier with devices if we skip the capability.
  */
-return 0;
+return true;
 }
 
 } else {
@@ -2099,12 +2099,12 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int 
pos, uint8_t size,
 pos = pci_add_capability(>pdev, PCI_CAP_ID_EXP, pos, size,
  errp);
 if (pos < 0) {
-return pos;
+return false;
 }
 
 vdev->pdev.exp.exp_cap = pos;
 
-return pos;
+return true;
 }
 
 static void vfio_check_pcie_flr(VFIOPCIDevice *vdev, uint8_t pos)
@@ -2137,14 +2137,14 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, 
uint8_t pos)
 }
 }
 
-static int vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos,
-uint8_t size, Error **errp)
+static bool