s390x/ccw: Error reporting cleanups

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

Posted by Cédric Le Goater 6 months ago

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

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

Posted by Anthony Krowiak 6 months ago

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 1/7] hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool

Posted by Duan, Zhenzhong 6 months ago


>-----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

[PATCH 1/7] hw/s390x/ccw: Make s390_ccw_get_dev_info() return a bool
[PATCH 2/7] s390x/css: Make CCWDeviceClass::realize return bool
[PATCH 3/7] hw/s390x/ccw: Remove local Error variable from s390_ccw_realize()
[PATCH 4/7] s390x/css: Make S390CCWDeviceClass::realize return bool
[PATCH 5/7] vfio/ccw: Use the 'Error **errp' argument of vfio_ccw_realize()
[PATCH 6/7] vfio/ccw: Fix the missed unrealize() call in error path
[PATCH 7/7] vfio/{ap, ccw}: Use warn_report_err() for IRQ notifier registration errors