Re: [PATCH 10/16] vfio/pci: Make vfio_populate_device() return a bool

[Cédric Le Goater]

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

Since vfio_populate_device() takes an 'Error **' argument,
best practices suggest to return a bool. See the qapi/error.h
Rules section.

Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
---
  hw/vfio/pci.c | 19 ++++++++++---------
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 379cbad757..c091d21adf 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2740,7 +2740,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
      return 0;
  }

-static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)

+static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
  {
      VFIODevice *vbasedev = &vdev->vbasedev;
      struct vfio_region_info *reg_info;
@@ -2750,18 +2750,18 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, 
Error **errp)
      /* Sanity check device */
      if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
          error_setg(errp, "this isn't a PCI device");
-        return;
+        return false;
      }

if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {

          error_setg(errp, "unexpected number of io regions %u",
                     vbasedev->num_regions);
-        return;
+        return false;
      }

if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {

          error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
-        return;
+        return false;
      }

for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) {

@@ -2773,7 +2773,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, 
Error **errp)

if (ret) {

              error_setg_errno(errp, -ret, "failed to get region %d info", i);
-            return;
+            return false;
          }

QLIST_INIT(&vdev->bars[i].quirks);

@@ -2783,7 +2783,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, 
Error **errp)
                                 VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
      if (ret) {
          error_setg_errno(errp, -ret, "failed to get config info");
-        return;
+        return false;
      }

trace_vfio_populate_device_config(vdev->vbasedev.name,

@@ -2804,7 +2804,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, 
Error **errp)
          if (ret) {
              error_append_hint(errp, "device does not support "
                                "requested feature x-vga\n");
-            return;
+            return false;
          }
      }

@@ -2821,6 +2821,8 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)

                      "Could not enable error recovery for the device",
                      vbasedev->name);
      }
+
+    return true;
  }

static void vfio_pci_put_device(VFIOPCIDevice *vdev)

@@ -3036,8 +3038,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
          goto error;
      }

- vfio_populate_device(vdev, &err);

-    if (err) {
+    if (!vfio_populate_device(vdev, &err)) {

why not pass errp directly and avoid error_propagate() ?

Thanks,

C.

          error_propagate(errp, err);
          goto error;
      }