Re: [PATCH 02/16] vfio/display: Make vfio_display_*() return bool

[Cédric Le Goater]

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

This is to follow the coding standand in qapi/error.h to return bool
for bool-valued functions.

Suggested-by: Cédric Le Goater <c...@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>

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

One comment below,

---
  hw/vfio/pci.h     |  2 +-
  hw/vfio/display.c | 20 ++++++++++----------
  hw/vfio/pci.c     |  3 +--
  3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 92cd62d115..a5ac9efd4b 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -232,7 +232,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
                                 Error **errp);

void vfio_display_reset(VFIOPCIDevice *vdev);

-int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
+bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
  void vfio_display_finalize(VFIOPCIDevice *vdev);

extern const VMStateDescription vfio_display_vmstate;

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 57c5ae0b2a..b562f4be74 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -346,11 +346,11 @@ static const GraphicHwOps vfio_display_dmabuf_ops = {
      .ui_info    = vfio_display_edid_ui_info,
  };

-static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)

+static bool vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
  {
      if (!display_opengl) {
          error_setg(errp, "vfio-display-dmabuf: opengl not available");
-        return -1;
+        return false;
      }

vdev->dpy = g_new0(VFIODisplay, 1);

@@ -360,11 +360,11 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, 
Error **errp)
      if (vdev->enable_ramfb) {
          vdev->dpy->ramfb = ramfb_setup(errp);
          if (!vdev->dpy->ramfb) {
-            return -EINVAL;
+            return false;
          }
      }
      vfio_display_edid_init(vdev);

vfio_display_edid_init() can fail for many reasons and does it silently.
It would be good to report the error in a future patch.

Thanks,

C.

-    return 0;
+    return true;
  }

static void vfio_display_dmabuf_exit(VFIODisplay *dpy)

@@ -481,7 +481,7 @@ static const GraphicHwOps vfio_display_region_ops = {
      .gfx_update = vfio_display_region_update,
  };

-static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)

+static bool vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
  {
      vdev->dpy = g_new0(VFIODisplay, 1);
      vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
@@ -490,10 +490,10 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, 
Error **errp)
      if (vdev->enable_ramfb) {
          vdev->dpy->ramfb = ramfb_setup(errp);
          if (!vdev->dpy->ramfb) {
-            return -EINVAL;
+            return false;
          }
      }
-    return 0;
+    return true;
  }

static void vfio_display_region_exit(VFIODisplay *dpy)

@@ -508,7 +508,7 @@ static void vfio_display_region_exit(VFIODisplay *dpy)

/* ---------------------------------------------------------------------- */ -int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)

+bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
  {
      struct vfio_device_gfx_plane_info probe;
      int ret;
@@ -531,11 +531,11 @@ int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)

if (vdev->display == ON_OFF_AUTO_AUTO) {

          /* not an error in automatic mode */
-        return 0;
+        return true;
      }

error_setg(errp, "vfio: device doesn't support any (known) display method");

-    return -1;
+    return false;
  }

void vfio_display_finalize(VFIOPCIDevice *vdev)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c1adef5cf8..a447013a1d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3200,8 +3200,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
      }

if (vdev->display != ON_OFF_AUTO_OFF) {

-        ret = vfio_display_probe(vdev, errp);
-        if (ret) {
+        if (!vfio_display_probe(vdev, errp)) {
              goto out_deregister;
          }
      }