[PATCH 0/7] s390x/ccw: Error reporting cleanups

[PATCH 0/7] s390x/ccw: Error reporting cleanups

[Cédric Le Goater]

Hello,

The first patches of this series simply apply the practices described
in the Rules section of the qapi/error.h file for routines taking an
'Error **' argument. The remaining patches are a fixup in the error
path of vfio_ccw_realize() and some error reporting adjustements.

Applies on top of this vfio PR :

  https://lore.kernel.org/qemu-devel/20240522095442.195243-1-clg@redhat.com

Thanks,

C.

Cédric Le Goater (6):
  hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool
  s390x/css: Make CCWDeviceClass::realize return bool
  hw/s390x/ccw: Remove local Error variable from s390_ccw_realize()
  s390x/css: Make S390CCWDeviceClass::realize return bool
  vfio/ccw: Use the 'Error **errp' argument of vfio_ccw_realize()
  vfio/{ap,ccw}: Use warn_report_err() for IRQ notifier registration
    errors

Zhenzhong Duan (1):
  vfio/ccw: Fix the missed unrealize() call in error path

 hw/s390x/ccw-device.h       |  2 +-
 include/hw/s390x/s390-ccw.h |  2 +-
 hw/s390x/ccw-device.c       |  3 ++-
 hw/s390x/s390-ccw.c         | 29 +++++++++++++----------------
 hw/vfio/ap.c                |  2 +-
 hw/vfio/ccw.c               | 18 ++++++++----------
 6 files changed, 26 insertions(+), 30 deletions(-)

-- 
2.45.1

[PATCH 1/7] hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool

[Cédric Le Goater]

Since s390_ccw_get_dev_info() takes an 'Error **' argument, best
practices suggest to return a bool. See the qapi/error.h Rules
section. While at it, modify the call in s390_ccw_realize().

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/s390x/s390-ccw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 5261e66724f1cc3157b9413b0d5fdf5289c92503..a06e91dfb318e3500324851488c56806fa46c08d 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -71,7 +71,7 @@ IOInstEnding s390_ccw_store(SubchDev *sch)
     return ret;
 }
 
-static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
+static bool s390_ccw_get_dev_info(S390CCWDevice *cdev,
                                   char *sysfsdev,
                                   Error **errp)
 {
@@ -84,12 +84,12 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
         error_setg(errp, "No host device provided");
         error_append_hint(errp,
                           "Use -device vfio-ccw,sysfsdev=PATH_TO_DEVICE\n");
-        return;
+        return false;
     }
 
     if (!realpath(sysfsdev, dev_path)) {
         error_setg_errno(errp, errno, "Host device '%s' not found", sysfsdev);
-        return;
+        return false;
     }
 
     cdev->mdevid = g_path_get_basename(dev_path);
@@ -98,13 +98,14 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
     tmp = g_path_get_basename(tmp_dir);
     if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) {
         error_setg_errno(errp, errno, "Failed to read %s", tmp);
-        return;
+        return false;
     }
 
     cdev->hostid.cssid = cssid;
     cdev->hostid.ssid = ssid;
     cdev->hostid.devid = devid;
     cdev->hostid.valid = true;
+    return true;
 }
 
 static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
@@ -116,8 +117,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
     int ret;
     Error *err = NULL;
 
-    s390_ccw_get_dev_info(cdev, sysfsdev, &err);
-    if (err) {
+    if (!s390_ccw_get_dev_info(cdev, sysfsdev, &err)) {
         goto out_err_propagate;
     }
 
-- 
2.45.1

[PATCH 2/7] s390x/css: Make CCWDeviceClass::realize return bool

[Cédric Le Goater]

Since the realize() handler of CCWDeviceClass takes an 'Error **'
argument, best practices suggest to return a bool. See the api/error.h
Rules section. While at it, modify the call in s390_ccw_realize().

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/s390x/ccw-device.h | 2 +-
 hw/s390x/ccw-device.c | 3 ++-
 hw/s390x/s390-ccw.c   | 3 +--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
index 6dff95225df11c63f9b66975019026b215c8c448..5feeb0ee7a268b8709043b5bbc56b06e707a448d 100644
--- a/hw/s390x/ccw-device.h
+++ b/hw/s390x/ccw-device.h
@@ -36,7 +36,7 @@ extern const VMStateDescription vmstate_ccw_dev;
 struct CCWDeviceClass {
     DeviceClass parent_class;
     void (*unplug)(HotplugHandler *, DeviceState *, Error **);
-    void (*realize)(CcwDevice *, Error **);
+    bool (*realize)(CcwDevice *, Error **);
     void (*refill_ids)(CcwDevice *);
 };
 
diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index fb8c1acc64d5002c861a4913f292d8346dbef192..a7d682e5af9ce90e7e2fad8c24b30e39328c7cf4 100644
--- a/hw/s390x/ccw-device.c
+++ b/hw/s390x/ccw-device.c
@@ -31,9 +31,10 @@ static void ccw_device_refill_ids(CcwDevice *dev)
     dev->subch_id.valid = true;
 }
 
-static void ccw_device_realize(CcwDevice *dev, Error **errp)
+static bool ccw_device_realize(CcwDevice *dev, Error **errp)
 {
     ccw_device_refill_ids(dev);
+    return true;
 }
 
 static Property ccw_device_properties[] = {
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index a06e91dfb318e3500324851488c56806fa46c08d..4b8ede701df90949720262b6fc1b65f4e505e34d 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -137,8 +137,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
         goto out_err;
     }
 
-    ck->realize(ccw_dev, &err);
-    if (err) {
+    if (!ck->realize(ccw_dev, &err)) {
         goto out_err;
     }
 
-- 
2.45.1

[PATCH 3/7] hw/s390x/ccw: Remove local Error variable from s390_ccw_realize()

[Cédric Le Goater]

Use the 'Error **errp' argument of s390_ccw_realize() instead and
remove the error_propagate() call.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/s390x/s390-ccw.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 4b8ede701df90949720262b6fc1b65f4e505e34d..b3d14c61d732880a651edcf28a040ca723cb9f5b 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -115,13 +115,12 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
     DeviceState *parent = DEVICE(ccw_dev);
     SubchDev *sch;
     int ret;
-    Error *err = NULL;
 
-    if (!s390_ccw_get_dev_info(cdev, sysfsdev, &err)) {
-        goto out_err_propagate;
+    if (!s390_ccw_get_dev_info(cdev, sysfsdev, errp)) {
+        return;
     }
 
-    sch = css_create_sch(ccw_dev->devno, &err);
+    sch = css_create_sch(ccw_dev->devno, errp);
     if (!sch) {
         goto out_mdevid_free;
     }
@@ -132,12 +131,12 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
     ccw_dev->sch = sch;
     ret = css_sch_build_schib(sch, &cdev->hostid);
     if (ret) {
-        error_setg_errno(&err, -ret, "%s: Failed to build initial schib",
+        error_setg_errno(errp, -ret, "%s: Failed to build initial schib",
                          __func__);
         goto out_err;
     }
 
-    if (!ck->realize(ccw_dev, &err)) {
+    if (!ck->realize(ccw_dev, errp)) {
         goto out_err;
     }
 
@@ -151,8 +150,6 @@ out_err:
     g_free(sch);
 out_mdevid_free:
     g_free(cdev->mdevid);
-out_err_propagate:
-    error_propagate(errp, err);
 }
 
 static void s390_ccw_unrealize(S390CCWDevice *cdev)
-- 
2.45.1

[PATCH 4/7] s390x/css: Make S390CCWDeviceClass::realize return bool

[Cédric Le Goater]

Since the realize() handler of S390CCWDeviceClass takes an 'Error **'
argument, best practices suggest to return a bool. See the api/error.h
Rules section. While at it, modify the call in vfio_ccw_realize().

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/hw/s390x/s390-ccw.h | 2 +-
 hw/s390x/s390-ccw.c         | 7 ++++---
 hw/vfio/ccw.c               | 3 +--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h
index 2c807ee3a1ae8d85460fe65be8a62c64f212fe4b..2e0a70998132070996d6b0d083b8ddba5b9b87dc 100644
--- a/include/hw/s390x/s390-ccw.h
+++ b/include/hw/s390x/s390-ccw.h
@@ -31,7 +31,7 @@ struct S390CCWDevice {
 
 struct S390CCWDeviceClass {
     CCWDeviceClass parent_class;
-    void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
+    bool (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
     void (*unrealize)(S390CCWDevice *dev);
     IOInstEnding (*handle_request) (SubchDev *sch);
     int (*handle_halt) (SubchDev *sch);
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index b3d14c61d732880a651edcf28a040ca723cb9f5b..3c0975055089c3629dd76ce2e1484a4ef66d8d41 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -108,7 +108,7 @@ static bool s390_ccw_get_dev_info(S390CCWDevice *cdev,
     return true;
 }
 
-static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
+static bool s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
 {
     CcwDevice *ccw_dev = CCW_DEVICE(cdev);
     CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
@@ -117,7 +117,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
     int ret;
 
     if (!s390_ccw_get_dev_info(cdev, sysfsdev, errp)) {
-        return;
+        return false;
     }
 
     sch = css_create_sch(ccw_dev->devno, errp);
@@ -142,7 +142,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
 
     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
                           parent->hotplugged, 1);
-    return;
+    return true;
 
 out_err:
     css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
@@ -150,6 +150,7 @@ out_err:
     g_free(sch);
 out_mdevid_free:
     g_free(cdev->mdevid);
+    return false;
 }
 
 static void s390_ccw_unrealize(S390CCWDevice *cdev)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 2600e62e37238779800dc2b3a0bd315d7633017b..9a8e052711fe2f7c067c52808b2af30d0ebfee0c 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -582,8 +582,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
 
     /* Call the class init function for subchannel. */
     if (cdc->realize) {
-        cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
-        if (err) {
+        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, &err)) {
             goto out_err_propagate;
         }
     }
-- 
2.45.1

[PATCH 5/7] vfio/ccw: Use the 'Error **errp' argument of vfio_ccw_realize()

[Cédric Le Goater]

The local error variable is kept for vfio_ccw_register_irq_notifier()
because it is not considered as a failing condition. We will change
how error reporting is done in following changes.

Remove the error_propagate() call.

Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/ccw.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 9a8e052711fe2f7c067c52808b2af30d0ebfee0c..a468fa2342b97e0ee36bd5fb8443025cc90a0453 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -582,8 +582,8 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
 
     /* Call the class init function for subchannel. */
     if (cdc->realize) {
-        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, &err)) {
-            goto out_err_propagate;
+        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, errp)) {
+            return;
         }
     }
 
@@ -596,17 +596,17 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         goto out_attach_dev_err;
     }
 
-    if (!vfio_ccw_get_region(vcdev, &err)) {
+    if (!vfio_ccw_get_region(vcdev, errp)) {
         goto out_region_err;
     }
 
-    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err)) {
+    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, errp)) {
         goto out_io_notifier_err;
     }
 
     if (vcdev->crw_region) {
         if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX,
-                                            &err)) {
+                                            errp)) {
             goto out_irq_notifier_err;
         }
     }
@@ -634,8 +634,6 @@ out_attach_dev_err:
     if (cdc->unrealize) {
         cdc->unrealize(cdev);
     }
-out_err_propagate:
-    error_propagate(errp, err);
 }
 
 static void vfio_ccw_unrealize(DeviceState *dev)
-- 
2.45.1

[PATCH 6/7] vfio/ccw: Fix the missed unrealize() call in error path

[Cédric Le Goater]

From: Zhenzhong Duan <zhenzhong.duan@intel.com>

When get name failed, we should call unrealize() so that
vfio_ccw_realize() is self contained.

Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a file handle")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/ccw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index a468fa2342b97e0ee36bd5fb8443025cc90a0453..36f2677a448c5e31523dcc3de7d973ec70e4a13c 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -588,7 +588,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
     }
 
     if (!vfio_device_get_name(vbasedev, errp)) {
-        return;
+        goto out_unrealize;
     }
 
     if (!vfio_attach_device(cdev->mdevid, vbasedev,
@@ -631,6 +631,7 @@ out_region_err:
     vfio_detach_device(vbasedev);
 out_attach_dev_err:
     g_free(vbasedev->name);
+out_unrealize:
     if (cdc->unrealize) {
         cdc->unrealize(cdev);
     }
-- 
2.45.1

[PATCH 7/7] vfio/{ap, ccw}: Use warn_report_err() for IRQ notifier registration errors

[Cédric Le Goater]

vfio_ccw_register_irq_notifier() and vfio_ap_register_irq_notifier()
errors are currently reported using error_report_err(). Since they are
not considered as failing conditions, using warn_report_err() is more
appropriate.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/ap.c  | 2 +-
 hw/vfio/ccw.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index c12531a7886a2fe87598be0861fba5923bd2c206..0c4354e3e70169ec072e16da0919936647d1d351 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -172,7 +172,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
          * Report this error, but do not make it a failing condition.
          * Lack of this IRQ in the host does not prevent normal operation.
          */
-        error_report_err(err);
+        warn_report_err(err);
     }
 
     return;
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 36f2677a448c5e31523dcc3de7d973ec70e4a13c..1f8e1272c7555cd0a770481d1ae92988f6e2e62e 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -616,7 +616,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
          * Report this error, but do not make it a failing condition.
          * Lack of this IRQ in the host does not prevent normal operation.
          */
-        error_report_err(err);
+        warn_report_err(err);
     }
 
     return;
-- 
2.45.1

RE: [PATCH 1/7] hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: [PATCH 1/7] hw/s390x/ccw: Make s390_ccw_get_dev_info() return
>a bool
>
>Since s390_ccw_get_dev_info() takes an 'Error **' argument, best
>practices suggest to return a bool. See the qapi/error.h Rules
>section. While at it, modify the call in s390_ccw_realize().
>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> hw/s390x/s390-ccw.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>index
>5261e66724f1cc3157b9413b0d5fdf5289c92503..a06e91dfb318e35003248
>51488c56806fa46c08d 100644
>--- a/hw/s390x/s390-ccw.c
>+++ b/hw/s390x/s390-ccw.c
>@@ -71,7 +71,7 @@ IOInstEnding s390_ccw_store(SubchDev *sch)
>     return ret;
> }
>
>-static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>+static bool s390_ccw_get_dev_info(S390CCWDevice *cdev,
>                                   char *sysfsdev,
>                                   Error **errp)
> {
>@@ -84,12 +84,12 @@ static void s390_ccw_get_dev_info(S390CCWDevice
>*cdev,
>         error_setg(errp, "No host device provided");
>         error_append_hint(errp,
>                           "Use -device vfio-ccw,sysfsdev=PATH_TO_DEVICE\n");
>-        return;
>+        return false;
>     }
>
>     if (!realpath(sysfsdev, dev_path)) {
>         error_setg_errno(errp, errno, "Host device '%s' not found", sysfsdev);
>-        return;
>+        return false;
>     }
>
>     cdev->mdevid = g_path_get_basename(dev_path);
>@@ -98,13 +98,14 @@ static void s390_ccw_get_dev_info(S390CCWDevice
>*cdev,
>     tmp = g_path_get_basename(tmp_dir);
>     if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) {
>         error_setg_errno(errp, errno, "Failed to read %s", tmp);
>-        return;
>+        return false;
>     }
>
>     cdev->hostid.cssid = cssid;
>     cdev->hostid.ssid = ssid;
>     cdev->hostid.devid = devid;
>     cdev->hostid.valid = true;
>+    return true;
> }
>
> static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error
>**errp)
>@@ -116,8 +117,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev,
>char *sysfsdev, Error **errp)
>     int ret;
>     Error *err = NULL;
>
>-    s390_ccw_get_dev_info(cdev, sysfsdev, &err);
>-    if (err) {
>+    if (!s390_ccw_get_dev_info(cdev, sysfsdev, &err)) {
>         goto out_err_propagate;
>     }
>
>--
>2.45.1

RE: [PATCH 2/7] s390x/css: Make CCWDeviceClass::realize return bool

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: [PATCH 2/7] s390x/css: Make CCWDeviceClass::realize return bool
>
>Since the realize() handler of CCWDeviceClass takes an 'Error **'
>argument, best practices suggest to return a bool. See the api/error.h
>Rules section. While at it, modify the call in s390_ccw_realize().
>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> hw/s390x/ccw-device.h | 2 +-
> hw/s390x/ccw-device.c | 3 ++-
> hw/s390x/s390-ccw.c   | 3 +--
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
>index
>6dff95225df11c63f9b66975019026b215c8c448..5feeb0ee7a268b8709043b
>5bbc56b06e707a448d 100644
>--- a/hw/s390x/ccw-device.h
>+++ b/hw/s390x/ccw-device.h
>@@ -36,7 +36,7 @@ extern const VMStateDescription vmstate_ccw_dev;
> struct CCWDeviceClass {
>     DeviceClass parent_class;
>     void (*unplug)(HotplugHandler *, DeviceState *, Error **);
>-    void (*realize)(CcwDevice *, Error **);
>+    bool (*realize)(CcwDevice *, Error **);
>     void (*refill_ids)(CcwDevice *);
> };
>
>diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
>index
>fb8c1acc64d5002c861a4913f292d8346dbef192..a7d682e5af9ce90e7e2fad8
>c24b30e39328c7cf4 100644
>--- a/hw/s390x/ccw-device.c
>+++ b/hw/s390x/ccw-device.c
>@@ -31,9 +31,10 @@ static void ccw_device_refill_ids(CcwDevice *dev)
>     dev->subch_id.valid = true;
> }
>
>-static void ccw_device_realize(CcwDevice *dev, Error **errp)
>+static bool ccw_device_realize(CcwDevice *dev, Error **errp)
> {
>     ccw_device_refill_ids(dev);
>+    return true;
> }
>
> static Property ccw_device_properties[] = {
>diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>index
>a06e91dfb318e3500324851488c56806fa46c08d..4b8ede701df9094972026
>2b6fc1b65f4e505e34d 100644
>--- a/hw/s390x/s390-ccw.c
>+++ b/hw/s390x/s390-ccw.c
>@@ -137,8 +137,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev,
>char *sysfsdev, Error **errp)
>         goto out_err;
>     }
>
>-    ck->realize(ccw_dev, &err);
>-    if (err) {
>+    if (!ck->realize(ccw_dev, &err)) {
>         goto out_err;
>     }
>
>--
>2.45.1

RE: [PATCH 3/7] hw/s390x/ccw: Remove local Error variable from s390_ccw_realize()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: [PATCH 3/7] hw/s390x/ccw: Remove local Error variable from
>s390_ccw_realize()
>
>Use the 'Error **errp' argument of s390_ccw_realize() instead and
>remove the error_propagate() call.
>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> hw/s390x/s390-ccw.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
>diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>index
>4b8ede701df90949720262b6fc1b65f4e505e34d..b3d14c61d732880a651ed
>cf28a040ca723cb9f5b 100644
>--- a/hw/s390x/s390-ccw.c
>+++ b/hw/s390x/s390-ccw.c
>@@ -115,13 +115,12 @@ static void s390_ccw_realize(S390CCWDevice
>*cdev, char *sysfsdev, Error **errp)
>     DeviceState *parent = DEVICE(ccw_dev);
>     SubchDev *sch;
>     int ret;
>-    Error *err = NULL;
>
>-    if (!s390_ccw_get_dev_info(cdev, sysfsdev, &err)) {
>-        goto out_err_propagate;
>+    if (!s390_ccw_get_dev_info(cdev, sysfsdev, errp)) {
>+        return;
>     }
>
>-    sch = css_create_sch(ccw_dev->devno, &err);
>+    sch = css_create_sch(ccw_dev->devno, errp);
>     if (!sch) {
>         goto out_mdevid_free;
>     }
>@@ -132,12 +131,12 @@ static void s390_ccw_realize(S390CCWDevice
>*cdev, char *sysfsdev, Error **errp)
>     ccw_dev->sch = sch;
>     ret = css_sch_build_schib(sch, &cdev->hostid);
>     if (ret) {
>-        error_setg_errno(&err, -ret, "%s: Failed to build initial schib",
>+        error_setg_errno(errp, -ret, "%s: Failed to build initial schib",
>                          __func__);
>         goto out_err;
>     }
>
>-    if (!ck->realize(ccw_dev, &err)) {
>+    if (!ck->realize(ccw_dev, errp)) {
>         goto out_err;
>     }
>
>@@ -151,8 +150,6 @@ out_err:
>     g_free(sch);
> out_mdevid_free:
>     g_free(cdev->mdevid);
>-out_err_propagate:
>-    error_propagate(errp, err);
> }
>
> static void s390_ccw_unrealize(S390CCWDevice *cdev)
>--
>2.45.1

RE: [PATCH 4/7] s390x/css: Make S390CCWDeviceClass::realize return bool

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: [PATCH 4/7] s390x/css: Make S390CCWDeviceClass::realize return
>bool
>
>Since the realize() handler of S390CCWDeviceClass takes an 'Error **'
>argument, best practices suggest to return a bool. See the api/error.h
>Rules section. While at it, modify the call in vfio_ccw_realize().
>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> include/hw/s390x/s390-ccw.h | 2 +-
> hw/s390x/s390-ccw.c         | 7 ++++---
> hw/vfio/ccw.c               | 3 +--
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h
>index
>2c807ee3a1ae8d85460fe65be8a62c64f212fe4b..2e0a70998132070996d6b
>0d083b8ddba5b9b87dc 100644
>--- a/include/hw/s390x/s390-ccw.h
>+++ b/include/hw/s390x/s390-ccw.h
>@@ -31,7 +31,7 @@ struct S390CCWDevice {
>
> struct S390CCWDeviceClass {
>     CCWDeviceClass parent_class;
>-    void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
>+    bool (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
>     void (*unrealize)(S390CCWDevice *dev);
>     IOInstEnding (*handle_request) (SubchDev *sch);
>     int (*handle_halt) (SubchDev *sch);
>diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>index
>b3d14c61d732880a651edcf28a040ca723cb9f5b..3c0975055089c3629dd76
>ce2e1484a4ef66d8d41 100644
>--- a/hw/s390x/s390-ccw.c
>+++ b/hw/s390x/s390-ccw.c
>@@ -108,7 +108,7 @@ static bool s390_ccw_get_dev_info(S390CCWDevice
>*cdev,
>     return true;
> }
>
>-static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error
>**errp)
>+static bool s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error
>**errp)
> {
>     CcwDevice *ccw_dev = CCW_DEVICE(cdev);
>     CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
>@@ -117,7 +117,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev,
>char *sysfsdev, Error **errp)
>     int ret;
>
>     if (!s390_ccw_get_dev_info(cdev, sysfsdev, errp)) {
>-        return;
>+        return false;
>     }
>
>     sch = css_create_sch(ccw_dev->devno, errp);
>@@ -142,7 +142,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev,
>char *sysfsdev, Error **errp)
>
>     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
>                           parent->hotplugged, 1);
>-    return;
>+    return true;
>
> out_err:
>     css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
>@@ -150,6 +150,7 @@ out_err:
>     g_free(sch);
> out_mdevid_free:
>     g_free(cdev->mdevid);
>+    return false;
> }
>
> static void s390_ccw_unrealize(S390CCWDevice *cdev)
>diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>index
>2600e62e37238779800dc2b3a0bd315d7633017b..9a8e052711fe2f7c067c
>52808b2af30d0ebfee0c 100644
>--- a/hw/vfio/ccw.c
>+++ b/hw/vfio/ccw.c
>@@ -582,8 +582,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error
>**errp)
>
>     /* Call the class init function for subchannel. */
>     if (cdc->realize) {
>-        cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
>-        if (err) {
>+        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, &err)) {
>             goto out_err_propagate;
>         }
>     }
>--
>2.45.1

RE: [PATCH 5/7] vfio/ccw: Use the 'Error **errp' argument of vfio_ccw_realize()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: [PATCH 5/7] vfio/ccw: Use the 'Error **errp' argument of
>vfio_ccw_realize()
>
>The local error variable is kept for vfio_ccw_register_irq_notifier()
>because it is not considered as a failing condition. We will change
>how error reporting is done in following changes.
>
>Remove the error_propagate() call.
>
>Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> hw/vfio/ccw.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
>diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>index
>9a8e052711fe2f7c067c52808b2af30d0ebfee0c..a468fa2342b97e0ee36bd5f
>b8443025cc90a0453 100644
>--- a/hw/vfio/ccw.c
>+++ b/hw/vfio/ccw.c
>@@ -582,8 +582,8 @@ static void vfio_ccw_realize(DeviceState *dev, Error
>**errp)
>
>     /* Call the class init function for subchannel. */
>     if (cdc->realize) {
>-        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, &err)) {
>-            goto out_err_propagate;
>+        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, errp)) {
>+            return;
>         }
>     }
>
>@@ -596,17 +596,17 @@ static void vfio_ccw_realize(DeviceState *dev,
>Error **errp)
>         goto out_attach_dev_err;
>     }
>
>-    if (!vfio_ccw_get_region(vcdev, &err)) {
>+    if (!vfio_ccw_get_region(vcdev, errp)) {
>         goto out_region_err;
>     }
>
>-    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX,
>&err)) {
>+    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX,
>errp)) {
>         goto out_io_notifier_err;
>     }
>
>     if (vcdev->crw_region) {
>         if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX,
>-                                            &err)) {
>+                                            errp)) {
>             goto out_irq_notifier_err;
>         }
>     }
>@@ -634,8 +634,6 @@ out_attach_dev_err:
>     if (cdc->unrealize) {
>         cdc->unrealize(cdev);
>     }
>-out_err_propagate:
>-    error_propagate(errp, err);
> }
>
> static void vfio_ccw_unrealize(DeviceState *dev)
>--
>2.45.1

RE: [PATCH 7/7] vfio/{ap, ccw}: Use warn_report_err() for IRQ notifier registration errors

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: [PATCH 7/7] vfio/{ap,ccw}: Use warn_report_err() for IRQ notifier
>registration errors
>
>vfio_ccw_register_irq_notifier() and vfio_ap_register_irq_notifier()
>errors are currently reported using error_report_err(). Since they are
>not considered as failing conditions, using warn_report_err() is more
>appropriate.
>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> hw/vfio/ap.c  | 2 +-
> hw/vfio/ccw.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>index
>c12531a7886a2fe87598be0861fba5923bd2c206..0c4354e3e70169ec072e1
>6da0919936647d1d351 100644
>--- a/hw/vfio/ap.c
>+++ b/hw/vfio/ap.c
>@@ -172,7 +172,7 @@ static void vfio_ap_realize(DeviceState *dev, Error
>**errp)
>          * Report this error, but do not make it a failing condition.
>          * Lack of this IRQ in the host does not prevent normal operation.
>          */
>-        error_report_err(err);
>+        warn_report_err(err);
>     }
>
>     return;
>diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>index
>36f2677a448c5e31523dcc3de7d973ec70e4a13c..1f8e1272c7555cd0a77048
>1d1ae92988f6e2e62e 100644
>--- a/hw/vfio/ccw.c
>+++ b/hw/vfio/ccw.c
>@@ -616,7 +616,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error
>**errp)
>          * Report this error, but do not make it a failing condition.
>          * Lack of this IRQ in the host does not prevent normal operation.
>          */
>-        error_report_err(err);
>+        warn_report_err(err);
>     }
>
>     return;
>--
>2.45.1

Re: [PATCH 6/7] vfio/ccw: Fix the missed unrealize() call in error path

[Cédric Le Goater]

On 5/22/24 19:01, Cédric Le Goater wrote:
> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
> 
> When get name failed, we should call unrealize() so that
> vfio_ccw_realize() is self contained.
> 
> Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a file handle")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>


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

Thanks,

C.


> ---
>   hw/vfio/ccw.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index a468fa2342b97e0ee36bd5fb8443025cc90a0453..36f2677a448c5e31523dcc3de7d973ec70e4a13c 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -588,7 +588,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>       }
>   
>       if (!vfio_device_get_name(vbasedev, errp)) {
> -        return;
> +        goto out_unrealize;
>       }
>   
>       if (!vfio_attach_device(cdev->mdevid, vbasedev,
> @@ -631,6 +631,7 @@ out_region_err:
>       vfio_detach_device(vbasedev);
>   out_attach_dev_err:
>       g_free(vbasedev->name);
> +out_unrealize:
>       if (cdc->unrealize) {
>           cdc->unrealize(cdev);
>       }

Re: [PATCH 0/7] s390x/ccw: Error reporting cleanups

[Eric Farman]

On Wed, 2024-05-22 at 19:01 +0200, Cédric Le Goater wrote:
> Hello,
> 
> The first patches of this series simply apply the practices described
> in the Rules section of the qapi/error.h file for routines taking an
> 'Error **' argument. The remaining patches are a fixup in the error
> path of vfio_ccw_realize() and some error reporting adjustements.
> 
> Applies on top of this vfio PR :
> 
>  
> https://lore.kernel.org/qemu-devel/20240522095442.195243-1-clg@redhat.com
> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (6):
>   hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool
>   s390x/css: Make CCWDeviceClass::realize return bool
>   hw/s390x/ccw: Remove local Error variable from s390_ccw_realize()
>   s390x/css: Make S390CCWDeviceClass::realize return bool
>   vfio/ccw: Use the 'Error **errp' argument of vfio_ccw_realize()
>   vfio/{ap,ccw}: Use warn_report_err() for IRQ notifier registration
>     errors
> 
> Zhenzhong Duan (1):
>   vfio/ccw: Fix the missed unrealize() call in error path
> 
>  hw/s390x/ccw-device.h       |  2 +-
>  include/hw/s390x/s390-ccw.h |  2 +-
>  hw/s390x/ccw-device.c       |  3 ++-
>  hw/s390x/s390-ccw.c         | 29 +++++++++++++----------------
>  hw/vfio/ap.c                |  2 +-
>  hw/vfio/ccw.c               | 18 ++++++++----------
>  6 files changed, 26 insertions(+), 30 deletions(-)
> 

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [PATCH 1/7] hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool

[Anthony Krowiak]

On 5/22/24 1:01 PM, Cédric Le Goater wrote:
> Since s390_ccw_get_dev_info() takes an 'Error **' argument, best
> practices suggest to return a bool. See the qapi/error.h Rules
> section. While at it, modify the call in s390_ccw_realize().
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>


Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>


> ---
>   hw/s390x/s390-ccw.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index 5261e66724f1cc3157b9413b0d5fdf5289c92503..a06e91dfb318e3500324851488c56806fa46c08d 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -71,7 +71,7 @@ IOInstEnding s390_ccw_store(SubchDev *sch)
>       return ret;
>   }
>   
> -static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
> +static bool s390_ccw_get_dev_info(S390CCWDevice *cdev,
>                                     char *sysfsdev,
>                                     Error **errp)
>   {
> @@ -84,12 +84,12 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>           error_setg(errp, "No host device provided");
>           error_append_hint(errp,
>                             "Use -device vfio-ccw,sysfsdev=PATH_TO_DEVICE\n");
> -        return;
> +        return false;
>       }
>   
>       if (!realpath(sysfsdev, dev_path)) {
>           error_setg_errno(errp, errno, "Host device '%s' not found", sysfsdev);
> -        return;
> +        return false;
>       }
>   
>       cdev->mdevid = g_path_get_basename(dev_path);
> @@ -98,13 +98,14 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>       tmp = g_path_get_basename(tmp_dir);
>       if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) {
>           error_setg_errno(errp, errno, "Failed to read %s", tmp);
> -        return;
> +        return false;
>       }
>   
>       cdev->hostid.cssid = cssid;
>       cdev->hostid.ssid = ssid;
>       cdev->hostid.devid = devid;
>       cdev->hostid.valid = true;
> +    return true;
>   }
>   
>   static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
> @@ -116,8 +117,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
>       int ret;
>       Error *err = NULL;
>   
> -    s390_ccw_get_dev_info(cdev, sysfsdev, &err);
> -    if (err) {
> +    if (!s390_ccw_get_dev_info(cdev, sysfsdev, &err)) {
>           goto out_err_propagate;
>       }
>   

Re: [PATCH 2/7] s390x/css: Make CCWDeviceClass::realize return bool

[Anthony Krowiak]

On 5/22/24 1:01 PM, Cédric Le Goater wrote:
> Since the realize() handler of CCWDeviceClass takes an 'Error **'
> argument, best practices suggest to return a bool. See the api/error.h
> Rules section. While at it, modify the call in s390_ccw_realize().
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>


Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>


> ---
>   hw/s390x/ccw-device.h | 2 +-
>   hw/s390x/ccw-device.c | 3 ++-
>   hw/s390x/s390-ccw.c   | 3 +--
>   3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
> index 6dff95225df11c63f9b66975019026b215c8c448..5feeb0ee7a268b8709043b5bbc56b06e707a448d 100644
> --- a/hw/s390x/ccw-device.h
> +++ b/hw/s390x/ccw-device.h
> @@ -36,7 +36,7 @@ extern const VMStateDescription vmstate_ccw_dev;
>   struct CCWDeviceClass {
>       DeviceClass parent_class;
>       void (*unplug)(HotplugHandler *, DeviceState *, Error **);
> -    void (*realize)(CcwDevice *, Error **);
> +    bool (*realize)(CcwDevice *, Error **);
>       void (*refill_ids)(CcwDevice *);
>   };
>   
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index fb8c1acc64d5002c861a4913f292d8346dbef192..a7d682e5af9ce90e7e2fad8c24b30e39328c7cf4 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -31,9 +31,10 @@ static void ccw_device_refill_ids(CcwDevice *dev)
>       dev->subch_id.valid = true;
>   }
>   
> -static void ccw_device_realize(CcwDevice *dev, Error **errp)
> +static bool ccw_device_realize(CcwDevice *dev, Error **errp)
>   {
>       ccw_device_refill_ids(dev);
> +    return true;
>   }
>   
>   static Property ccw_device_properties[] = {
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index a06e91dfb318e3500324851488c56806fa46c08d..4b8ede701df90949720262b6fc1b65f4e505e34d 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -137,8 +137,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
>           goto out_err;
>       }
>   
> -    ck->realize(ccw_dev, &err);
> -    if (err) {
> +    if (!ck->realize(ccw_dev, &err)) {
>           goto out_err;
>       }
>   

Re: [PATCH 7/7] vfio/{ap, ccw}: Use warn_report_err() for IRQ notifier registration errors

[Anthony Krowiak]

On 5/22/24 1:01 PM, Cédric Le Goater wrote:
> vfio_ccw_register_irq_notifier() and vfio_ap_register_irq_notifier()
> errors are currently reported using error_report_err(). Since they are
> not considered as failing conditions, using warn_report_err() is more
> appropriate.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>


Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>


> ---
>   hw/vfio/ap.c  | 2 +-
>   hw/vfio/ccw.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index c12531a7886a2fe87598be0861fba5923bd2c206..0c4354e3e70169ec072e16da0919936647d1d351 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -172,7 +172,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>            * Report this error, but do not make it a failing condition.
>            * Lack of this IRQ in the host does not prevent normal operation.
>            */
> -        error_report_err(err);
> +        warn_report_err(err);
>       }
>   
>       return;
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 36f2677a448c5e31523dcc3de7d973ec70e4a13c..1f8e1272c7555cd0a770481d1ae92988f6e2e62e 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -616,7 +616,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>            * Report this error, but do not make it a failing condition.
>            * Lack of this IRQ in the host does not prevent normal operation.
>            */
> -        error_report_err(err);
> +        warn_report_err(err);
>       }
>   
>       return;

Re: [PATCH 3/7] hw/s390x/ccw: Remove local Error variable from s390_ccw_realize()

[Anthony Krowiak]

On 5/22/24 1:01 PM, Cédric Le Goater wrote:
> Use the 'Error **errp' argument of s390_ccw_realize() instead and
> remove the error_propagate() call.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>


Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>


> ---
>   hw/s390x/s390-ccw.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index 4b8ede701df90949720262b6fc1b65f4e505e34d..b3d14c61d732880a651edcf28a040ca723cb9f5b 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -115,13 +115,12 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
>       DeviceState *parent = DEVICE(ccw_dev);
>       SubchDev *sch;
>       int ret;
> -    Error *err = NULL;
>   
> -    if (!s390_ccw_get_dev_info(cdev, sysfsdev, &err)) {
> -        goto out_err_propagate;
> +    if (!s390_ccw_get_dev_info(cdev, sysfsdev, errp)) {
> +        return;
>       }
>   
> -    sch = css_create_sch(ccw_dev->devno, &err);
> +    sch = css_create_sch(ccw_dev->devno, errp);
>       if (!sch) {
>           goto out_mdevid_free;
>       }
> @@ -132,12 +131,12 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
>       ccw_dev->sch = sch;
>       ret = css_sch_build_schib(sch, &cdev->hostid);
>       if (ret) {
> -        error_setg_errno(&err, -ret, "%s: Failed to build initial schib",
> +        error_setg_errno(errp, -ret, "%s: Failed to build initial schib",
>                            __func__);
>           goto out_err;
>       }
>   
> -    if (!ck->realize(ccw_dev, &err)) {
> +    if (!ck->realize(ccw_dev, errp)) {
>           goto out_err;
>       }
>   
> @@ -151,8 +150,6 @@ out_err:
>       g_free(sch);
>   out_mdevid_free:
>       g_free(cdev->mdevid);
> -out_err_propagate:
> -    error_propagate(errp, err);
>   }
>   
>   static void s390_ccw_unrealize(S390CCWDevice *cdev)

Re: [PATCH 4/7] s390x/css: Make S390CCWDeviceClass::realize return bool

[Anthony Krowiak]

On 5/22/24 1:01 PM, Cédric Le Goater wrote:
> Since the realize() handler of S390CCWDeviceClass takes an 'Error **'
> argument, best practices suggest to return a bool. See the api/error.h
> Rules section. While at it, modify the call in vfio_ccw_realize().
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>


Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>


> ---
>   include/hw/s390x/s390-ccw.h | 2 +-
>   hw/s390x/s390-ccw.c         | 7 ++++---
>   hw/vfio/ccw.c               | 3 +--
>   3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h
> index 2c807ee3a1ae8d85460fe65be8a62c64f212fe4b..2e0a70998132070996d6b0d083b8ddba5b9b87dc 100644
> --- a/include/hw/s390x/s390-ccw.h
> +++ b/include/hw/s390x/s390-ccw.h
> @@ -31,7 +31,7 @@ struct S390CCWDevice {
>   
>   struct S390CCWDeviceClass {
>       CCWDeviceClass parent_class;
> -    void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
> +    bool (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
>       void (*unrealize)(S390CCWDevice *dev);
>       IOInstEnding (*handle_request) (SubchDev *sch);
>       int (*handle_halt) (SubchDev *sch);
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index b3d14c61d732880a651edcf28a040ca723cb9f5b..3c0975055089c3629dd76ce2e1484a4ef66d8d41 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -108,7 +108,7 @@ static bool s390_ccw_get_dev_info(S390CCWDevice *cdev,
>       return true;
>   }
>   
> -static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
> +static bool s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
>   {
>       CcwDevice *ccw_dev = CCW_DEVICE(cdev);
>       CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
> @@ -117,7 +117,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
>       int ret;
>   
>       if (!s390_ccw_get_dev_info(cdev, sysfsdev, errp)) {
> -        return;
> +        return false;
>       }
>   
>       sch = css_create_sch(ccw_dev->devno, errp);
> @@ -142,7 +142,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
>   
>       css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
>                             parent->hotplugged, 1);
> -    return;
> +    return true;
>   
>   out_err:
>       css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
> @@ -150,6 +150,7 @@ out_err:
>       g_free(sch);
>   out_mdevid_free:
>       g_free(cdev->mdevid);
> +    return false;
>   }
>   
>   static void s390_ccw_unrealize(S390CCWDevice *cdev)
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 2600e62e37238779800dc2b3a0bd315d7633017b..9a8e052711fe2f7c067c52808b2af30d0ebfee0c 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -582,8 +582,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>   
>       /* Call the class init function for subchannel. */
>       if (cdc->realize) {
> -        cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
> -        if (err) {
> +        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, &err)) {
>               goto out_err_propagate;
>           }
>       }

Re: [PATCH 5/7] vfio/ccw: Use the 'Error **errp' argument of vfio_ccw_realize()

[Anthony Krowiak]

On 5/22/24 1:01 PM, Cédric Le Goater wrote:
> The local error variable is kept for vfio_ccw_register_irq_notifier()
> because it is not considered as a failing condition. We will change
> how error reporting is done in following changes.
>
> Remove the error_propagate() call.
>
> Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>


Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>


> ---
>   hw/vfio/ccw.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 9a8e052711fe2f7c067c52808b2af30d0ebfee0c..a468fa2342b97e0ee36bd5fb8443025cc90a0453 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -582,8 +582,8 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>   
>       /* Call the class init function for subchannel. */
>       if (cdc->realize) {
> -        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, &err)) {
> -            goto out_err_propagate;
> +        if (!cdc->realize(cdev, vcdev->vdev.sysfsdev, errp)) {
> +            return;
>           }
>       }
>   
> @@ -596,17 +596,17 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>           goto out_attach_dev_err;
>       }
>   
> -    if (!vfio_ccw_get_region(vcdev, &err)) {
> +    if (!vfio_ccw_get_region(vcdev, errp)) {
>           goto out_region_err;
>       }
>   
> -    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err)) {
> +    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, errp)) {
>           goto out_io_notifier_err;
>       }
>   
>       if (vcdev->crw_region) {
>           if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX,
> -                                            &err)) {
> +                                            errp)) {
>               goto out_irq_notifier_err;
>           }
>       }
> @@ -634,8 +634,6 @@ out_attach_dev_err:
>       if (cdc->unrealize) {
>           cdc->unrealize(cdev);
>       }
> -out_err_propagate:
> -    error_propagate(errp, err);
>   }
>   
>   static void vfio_ccw_unrealize(DeviceState *dev)

Re: [PATCH 0/7] s390x/ccw: Error reporting cleanups

[Thomas Huth]

On 22/05/2024 19.01, Cédric Le Goater wrote:
> Hello,
> 
> The first patches of this series simply apply the practices described
> in the Rules section of the qapi/error.h file for routines taking an
> 'Error **' argument. The remaining patches are a fixup in the error
> path of vfio_ccw_realize() and some error reporting adjustements.
> 
> Applies on top of this vfio PR :
> 
>    https://lore.kernel.org/qemu-devel/20240522095442.195243-1-clg@redhat.com
> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (6):
>    hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool
>    s390x/css: Make CCWDeviceClass::realize return bool
>    hw/s390x/ccw: Remove local Error variable from s390_ccw_realize()
>    s390x/css: Make S390CCWDeviceClass::realize return bool
>    vfio/ccw: Use the 'Error **errp' argument of vfio_ccw_realize()
>    vfio/{ap,ccw}: Use warn_report_err() for IRQ notifier registration
>      errors

Series
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/7] s390x/ccw: Error reporting cleanups

[Cédric Le Goater]

On 5/27/24 08:23, Thomas Huth wrote:
> On 22/05/2024 19.01, Cédric Le Goater wrote:
>> Hello,
>>
>> The first patches of this series simply apply the practices described
>> in the Rules section of the qapi/error.h file for routines taking an
>> 'Error **' argument. The remaining patches are a fixup in the error
>> path of vfio_ccw_realize() and some error reporting adjustements.
>>
>> Applies on top of this vfio PR :
>>
>>    https://lore.kernel.org/qemu-devel/20240522095442.195243-1-clg@redhat.com
>>
>> Thanks,
>>
>> C.
>>
>> Cédric Le Goater (6):
>>    hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool
>>    s390x/css: Make CCWDeviceClass::realize return bool
>>    hw/s390x/ccw: Remove local Error variable from s390_ccw_realize()
>>    s390x/css: Make S390CCWDeviceClass::realize return bool
>>    vfio/ccw: Use the 'Error **errp' argument of vfio_ccw_realize()
>>    vfio/{ap,ccw}: Use warn_report_err() for IRQ notifier registration
>>      errors
> 
> Series
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 


Applied to vfio-next.

Thanks,

C.