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

[Cédric Le Goater]

On 5/22/24 06:40, 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.

By this chance, pass errp directly to vfio_populate_device() to
avoid calling error_propagate().

Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>

Reviewed-by: Cédric Le Goater <c...@redhat.com>

Thanks,

C.

---
  hw/vfio/pci.c | 21 ++++++++++-----------
  1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4fb5fd0c9f..46d3c61859 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)

@@ -2977,7 +2979,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
      VFIOPCIDevice *vdev = VFIO_PCI(pdev);
      VFIODevice *vbasedev = &vdev->vbasedev;
      char *subsys;
-    Error *err = NULL;
      int i, ret;
      bool is_mdev;
      char uuid[UUID_STR_LEN];
@@ -3036,9 +3037,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
          goto error;
      }

- vfio_populate_device(vdev, &err);

-    if (err) {
-        error_propagate(errp, err);
+    if (!vfio_populate_device(vdev, errp)) {
          goto error;
      }