[PATCH v2 1/4] vfio/ap: Use g_autofree variable in vfio_ap_register_irq_notifier()

[PATCH v2 1/4] vfio/ap: Use g_autofree variable in vfio_ap_register_irq_notifier()

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/ap.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 7c4caa5938636937680fec87e999249ac84a4498..03f8ffaa5e2bf13cf8daa2f44aa4cf17809abd94 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -77,7 +77,7 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
     size_t argsz;
     IOHandler *fd_read;
     EventNotifier *notifier;
-    struct vfio_irq_info *irq_info;
+    g_autofree struct vfio_irq_info *irq_info = NULL;
     VFIODevice *vdev = &vapdev->vdev;
 
     switch (irq) {
@@ -104,14 +104,14 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
     if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
               irq_info) < 0 || irq_info->count < 1) {
         error_setg_errno(errp, errno, "vfio: Error getting irq info");
-        goto out_free_info;
+        return;
     }
 
     if (event_notifier_init(notifier, 0)) {
         error_setg_errno(errp, errno,
                          "vfio: Unable to init event notifier for irq (%d)",
                          irq);
-        goto out_free_info;
+        return;
     }
 
     fd = event_notifier_get_fd(notifier);
@@ -122,10 +122,6 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
         qemu_set_fd_handler(fd, NULL, NULL, vapdev);
         event_notifier_cleanup(notifier);
     }
-
-out_free_info:
-    g_free(irq_info);
-
 }
 
 static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
-- 
2.44.0

[PATCH v2 2/4] vfio/ap: Make vfio_ap_register_irq_notifier() return a bool

[Cédric Le Goater]

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

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/ap.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 03f8ffaa5e2bf13cf8daa2f44aa4cf17809abd94..8bb024e2fde4a1d72346dee4b662d762374326b9 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -70,7 +70,7 @@ static void vfio_ap_req_notifier_handler(void *opaque)
     }
 }
 
-static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
+static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
                                           unsigned int irq, Error **errp)
 {
     int fd;
@@ -87,13 +87,13 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
         break;
     default:
         error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
-        return;
+        return false;
     }
 
     if (vdev->num_irqs < irq + 1) {
         error_setg(errp, "vfio: IRQ %u not available (number of irqs %u)",
                    irq, vdev->num_irqs);
-        return;
+        return false;
     }
 
     argsz = sizeof(*irq_info);
@@ -104,14 +104,14 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
     if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
               irq_info) < 0 || irq_info->count < 1) {
         error_setg_errno(errp, errno, "vfio: Error getting irq info");
-        return;
+        return false;
     }
 
     if (event_notifier_init(notifier, 0)) {
         error_setg_errno(errp, errno,
                          "vfio: Unable to init event notifier for irq (%d)",
                          irq);
-        return;
+        return false;
     }
 
     fd = event_notifier_get_fd(notifier);
@@ -122,6 +122,8 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
         qemu_set_fd_handler(fd, NULL, NULL, vapdev);
         event_notifier_cleanup(notifier);
     }
+
+    return true;
 }
 
 static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
@@ -167,8 +169,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
         goto error;
     }
 
-    vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err);
-    if (err) {
+    if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err)) {
         /*
          * Report this error, but do not make it a failing condition.
          * Lack of this IRQ in the host does not prevent normal operation.
-- 
2.44.0

[PATCH v2 3/4] vfio/ccw: Use g_autofree variable in vfio_ccw_register_irq_notifier()

[Cédric Le Goater]

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

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 90e4a534371684c08e112364e1537eb8979f73f4..6764388bc47a970329fce2233626ccb8178e0165 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -384,7 +384,7 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
                                            Error **errp)
 {
     VFIODevice *vdev = &vcdev->vdev;
-    struct vfio_irq_info *irq_info;
+    g_autofree struct vfio_irq_info *irq_info = NULL;
     size_t argsz;
     int fd;
     EventNotifier *notifier;
@@ -421,14 +421,14 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
     if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
               irq_info) < 0 || irq_info->count < 1) {
         error_setg_errno(errp, errno, "vfio: Error getting irq info");
-        goto out_free_info;
+        return;
     }
 
     if (event_notifier_init(notifier, 0)) {
         error_setg_errno(errp, errno,
                          "vfio: Unable to init event notifier for irq (%d)",
                          irq);
-        goto out_free_info;
+        return;
     }
 
     fd = event_notifier_get_fd(notifier);
@@ -439,9 +439,6 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
         qemu_set_fd_handler(fd, NULL, NULL, vcdev);
         event_notifier_cleanup(notifier);
     }
-
-out_free_info:
-    g_free(irq_info);
 }
 
 static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,
-- 
2.44.0

[PATCH v2 4/4] vfio/ccw: Make vfio_ccw_register_irq_notifier() return a bool

[Cédric Le Goater]

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

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

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 6764388bc47a970329fce2233626ccb8178e0165..1c630f6e9abe93ae0c2b5615d4409669f096c8c9 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -379,7 +379,7 @@ read_err:
     css_inject_io_interrupt(sch);
 }
 
-static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
+static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
                                            unsigned int irq,
                                            Error **errp)
 {
@@ -405,13 +405,13 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
         break;
     default:
         error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
-        return;
+        return false;
     }
 
     if (vdev->num_irqs < irq + 1) {
         error_setg(errp, "vfio: IRQ %u not available (number of irqs %u)",
                    irq, vdev->num_irqs);
-        return;
+        return false;
     }
 
     argsz = sizeof(*irq_info);
@@ -421,14 +421,14 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
     if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
               irq_info) < 0 || irq_info->count < 1) {
         error_setg_errno(errp, errno, "vfio: Error getting irq info");
-        return;
+        return false;
     }
 
     if (event_notifier_init(notifier, 0)) {
         error_setg_errno(errp, errno,
                          "vfio: Unable to init event notifier for irq (%d)",
                          irq);
-        return;
+        return false;
     }
 
     fd = event_notifier_get_fd(notifier);
@@ -439,6 +439,8 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
         qemu_set_fd_handler(fd, NULL, NULL, vcdev);
         event_notifier_cleanup(notifier);
     }
+
+    return true;
 }
 
 static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,
@@ -602,20 +604,18 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         goto out_region_err;
     }
 
-    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err);
-    if (err) {
+    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err)) {
         goto out_io_notifier_err;
     }
 
     if (vcdev->crw_region) {
-        vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, &err);
-        if (err) {
+        if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX,
+                                            &err)) {
             goto out_irq_notifier_err;
         }
     }
 
-    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err);
-    if (err) {
+    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err)) {
         /*
          * Report this error, but do not make it a failing condition.
          * Lack of this IRQ in the host does not prevent normal operation.
-- 
2.44.0

Re: [PATCH v2 1/4] vfio/ap: Use g_autofree variable in vfio_ap_register_irq_notifier()

[Markus Armbruster]

Cédric Le Goater <clg@redhat.com> writes:

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 2/4] vfio/ap: Make vfio_ap_register_irq_notifier() return a bool

[Markus Armbruster]

Cédric Le Goater <clg@redhat.com> writes:

> Since vfio_ap_register_irq_notifier() takes and 'Error **' argument,
> best practices suggest to return a bool. See the qapi/error.h Rules
> section.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/vfio/ap.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 03f8ffaa5e2bf13cf8daa2f44aa4cf17809abd94..8bb024e2fde4a1d72346dee4b662d762374326b9 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -70,7 +70,7 @@ static void vfio_ap_req_notifier_handler(void *opaque)
>      }
>  }
>  
> -static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
> +static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>                                            unsigned int irq, Error **errp)
>  {
>      int fd;
> @@ -87,13 +87,13 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>          break;
>      default:
>          error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
> -        return;
> +        return false;
>      }
>  
>      if (vdev->num_irqs < irq + 1) {
>          error_setg(errp, "vfio: IRQ %u not available (number of irqs %u)",
>                     irq, vdev->num_irqs);
> -        return;
> +        return false;
>      }
>  
>      argsz = sizeof(*irq_info);
> @@ -104,14 +104,14 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>      if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>                irq_info) < 0 || irq_info->count < 1) {
>          error_setg_errno(errp, errno, "vfio: Error getting irq info");
> -        return;
> +        return false;
>      }
>  
>      if (event_notifier_init(notifier, 0)) {
>          error_setg_errno(errp, errno,
>                           "vfio: Unable to init event notifier for irq (%d)",
>                           irq);
> -        return;
> +        return false;
>      }
>  
>      fd = event_notifier_get_fd(notifier);
> @@ -122,6 +122,8 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>          qemu_set_fd_handler(fd, NULL, NULL, vapdev);
>          event_notifier_cleanup(notifier);
>      }
> +
> +    return true;
>  }
>  
>  static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
> @@ -167,8 +169,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>          goto error;
>      }
>  
> -    vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err);
> -    if (err) {
> +    if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err)) {
>          /*
>           * 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);

Not this patch's problem, but here goes anyway: since this isn't an
error, we shouldn't use error_report_err().  Would warn_report_err() be
appropriate?  info_report_err() doesn't exist, but it could.

This patch is fine as is, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 3/4] vfio/ccw: Use g_autofree variable in vfio_ccw_register_irq_notifier()

[Markus Armbruster]

Cédric Le Goater <clg@redhat.com> writes:

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 4/4] vfio/ccw: Make vfio_ccw_register_irq_notifier() return a bool

[Markus Armbruster]

Cédric Le Goater <clg@redhat.com> writes:

> Since vfio_ccw_register_irq_notifier() takes an 'Error **' argument,
> best practices suggest to return a bool. See the qapi/error.h Rules
> section.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/vfio/ccw.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 6764388bc47a970329fce2233626ccb8178e0165..1c630f6e9abe93ae0c2b5615d4409669f096c8c9 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -379,7 +379,7 @@ read_err:
>      css_inject_io_interrupt(sch);
>  }
>  
> -static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
> +static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>                                             unsigned int irq,
>                                             Error **errp)
>  {
> @@ -405,13 +405,13 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>          break;
>      default:
>          error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
> -        return;
> +        return false;
>      }
>  
>      if (vdev->num_irqs < irq + 1) {
>          error_setg(errp, "vfio: IRQ %u not available (number of irqs %u)",
>                     irq, vdev->num_irqs);
> -        return;
> +        return false;
>      }
>  
>      argsz = sizeof(*irq_info);
> @@ -421,14 +421,14 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>      if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>                irq_info) < 0 || irq_info->count < 1) {
>          error_setg_errno(errp, errno, "vfio: Error getting irq info");
> -        return;
> +        return false;
>      }
>  
>      if (event_notifier_init(notifier, 0)) {
>          error_setg_errno(errp, errno,
>                           "vfio: Unable to init event notifier for irq (%d)",
>                           irq);
> -        return;
> +        return false;
>      }
>  
>      fd = event_notifier_get_fd(notifier);
> @@ -439,6 +439,8 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>          qemu_set_fd_handler(fd, NULL, NULL, vcdev);
>          event_notifier_cleanup(notifier);
>      }
> +
> +    return true;
>  }
>  
>  static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,
> @@ -602,20 +604,18 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>          goto out_region_err;
>      }
>  
> -    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err);
> -    if (err) {
> +    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err)) {

Please pass errp instead of &err.

>          goto out_io_notifier_err;
>      }
>  
>      if (vcdev->crw_region) {
> -        vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, &err);
> -        if (err) {
> +        if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX,
> +                                            &err)) {

Likewise.

>              goto out_irq_notifier_err;
>          }
>      }
>  
> -    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err);
> -    if (err) {
> +    if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err)) {
>          /*
>           * 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);

Not this patch's problem, but here goes anyway: since this isn't an
error, we shouldn't use error_report_err().  Would warn_report_err() be
appropriate?  info_report_err() doesn't exist, but it could.

Preferably with errp instead of &err (two times):
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 3/4] vfio/ccw: Use g_autofree variable in vfio_ccw_register_irq_notifier()

[Eric Farman]

On Thu, 2024-04-25 at 11:02 +0200, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/vfio/ccw.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

I realize this was just split out of v1, but probably need a commit
description here.

Regardless,

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

Re: [PATCH v2 4/4] vfio/ccw: Make vfio_ccw_register_irq_notifier() return a bool

[Eric Farman]

On Thu, 2024-04-25 at 12:56 +0200, Markus Armbruster wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
> > Since vfio_ccw_register_irq_notifier() takes an 'Error **'
> > argument,
> > best practices suggest to return a bool. See the qapi/error.h Rules
> > section.
> > 
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > ---
> >  hw/vfio/ccw.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > index
> > 6764388bc47a970329fce2233626ccb8178e0165..1c630f6e9abe93ae0c2b5615d
> > 4409669f096c8c9 100644
> > --- a/hw/vfio/ccw.c
> > +++ b/hw/vfio/ccw.c
> > @@ -379,7 +379,7 @@ read_err:
> >      css_inject_io_interrupt(sch);
> >  }
> >  
> > -static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
> > +static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
> >                                             unsigned int irq,
> >                                             Error **errp)
> >  {
> > @@ -405,13 +405,13 @@ static void
> > vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
> >          break;
> >      default:
> >          error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
> > -        return;
> > +        return false;
> >      }
> >  
> >      if (vdev->num_irqs < irq + 1) {
> >          error_setg(errp, "vfio: IRQ %u not available (number of
> > irqs %u)",
> >                     irq, vdev->num_irqs);
> > -        return;
> > +        return false;
> >      }
> >  
> >      argsz = sizeof(*irq_info);
> > @@ -421,14 +421,14 @@ static void
> > vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
> >      if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
> >                irq_info) < 0 || irq_info->count < 1) {
> >          error_setg_errno(errp, errno, "vfio: Error getting irq
> > info");
> > -        return;
> > +        return false;
> >      }
> >  
> >      if (event_notifier_init(notifier, 0)) {
> >          error_setg_errno(errp, errno,
> >                           "vfio: Unable to init event notifier for
> > irq (%d)",
> >                           irq);
> > -        return;
> > +        return false;
> >      }
> >  
> >      fd = event_notifier_get_fd(notifier);
> > @@ -439,6 +439,8 @@ static void
> > vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
> >          qemu_set_fd_handler(fd, NULL, NULL, vcdev);
> >          event_notifier_cleanup(notifier);
> >      }
> > +
> > +    return true;
> >  }
> >  
> >  static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,
> > @@ -602,20 +604,18 @@ static void vfio_ccw_realize(DeviceState
> > *dev, Error **errp)
> >          goto out_region_err;
> >      }
> >  
> > -    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX,
> > &err);
> > -    if (err) {
> > +    if (!vfio_ccw_register_irq_notifier(vcdev,
> > VFIO_CCW_IO_IRQ_INDEX, &err)) {
> 
> Please pass errp instead of &err.
> 
> >          goto out_io_notifier_err;
> >      }
> >  
> >      if (vcdev->crw_region) {
> > -        vfio_ccw_register_irq_notifier(vcdev,
> > VFIO_CCW_CRW_IRQ_INDEX, &err);
> > -        if (err) {
> > +        if (!vfio_ccw_register_irq_notifier(vcdev,
> > VFIO_CCW_CRW_IRQ_INDEX,
> > +                                            &err)) {
> 
> Likewise.
> 
> >              goto out_irq_notifier_err;
> >          }
> >      }
> >  
> > -    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX,
> > &err);
> > -    if (err) {
> > +    if (!vfio_ccw_register_irq_notifier(vcdev,
> > VFIO_CCW_REQ_IRQ_INDEX, &err)) {
> >          /*
> >           * 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);
> 
> Not this patch's problem, but here goes anyway: since this isn't an
> error, we shouldn't use error_report_err().  Would warn_report_err()
> be
> appropriate?  info_report_err() doesn't exist, but it could.
> 
> Preferably with errp instead of &err (two times):
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Don't recall why I used error_report_err() instead of something else
(or creating info_), but probably just familiarity. There's no need for
it (or the equivalent code in -ap) to be error, and could be another
cleanup.

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

Re: [PATCH v2 4/4] vfio/ccw: Make vfio_ccw_register_irq_notifier() return a bool

[Cédric Le Goater]

On 4/25/24 14:55, Eric Farman wrote:
> On Thu, 2024-04-25 at 12:56 +0200, Markus Armbruster wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>>
>>> Since vfio_ccw_register_irq_notifier() takes an 'Error **'
>>> argument,
>>> best practices suggest to return a bool. See the qapi/error.h Rules
>>> section.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   hw/vfio/ccw.c | 22 +++++++++++-----------
>>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>> index
>>> 6764388bc47a970329fce2233626ccb8178e0165..1c630f6e9abe93ae0c2b5615d
>>> 4409669f096c8c9 100644
>>> --- a/hw/vfio/ccw.c
>>> +++ b/hw/vfio/ccw.c
>>> @@ -379,7 +379,7 @@ read_err:
>>>       css_inject_io_interrupt(sch);
>>>   }
>>>   
>>> -static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>>> +static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>>>                                              unsigned int irq,
>>>                                              Error **errp)
>>>   {
>>> @@ -405,13 +405,13 @@ static void
>>> vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>>>           break;
>>>       default:
>>>           error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
>>> -        return;
>>> +        return false;
>>>       }
>>>   
>>>       if (vdev->num_irqs < irq + 1) {
>>>           error_setg(errp, "vfio: IRQ %u not available (number of
>>> irqs %u)",
>>>                      irq, vdev->num_irqs);
>>> -        return;
>>> +        return false;
>>>       }
>>>   
>>>       argsz = sizeof(*irq_info);
>>> @@ -421,14 +421,14 @@ static void
>>> vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>>>       if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>>>                 irq_info) < 0 || irq_info->count < 1) {
>>>           error_setg_errno(errp, errno, "vfio: Error getting irq
>>> info");
>>> -        return;
>>> +        return false;
>>>       }
>>>   
>>>       if (event_notifier_init(notifier, 0)) {
>>>           error_setg_errno(errp, errno,
>>>                            "vfio: Unable to init event notifier for
>>> irq (%d)",
>>>                            irq);
>>> -        return;
>>> +        return false;
>>>       }
>>>   
>>>       fd = event_notifier_get_fd(notifier);
>>> @@ -439,6 +439,8 @@ static void
>>> vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
>>>           qemu_set_fd_handler(fd, NULL, NULL, vcdev);
>>>           event_notifier_cleanup(notifier);
>>>       }
>>> +
>>> +    return true;
>>>   }
>>>   
>>>   static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,
>>> @@ -602,20 +604,18 @@ static void vfio_ccw_realize(DeviceState
>>> *dev, Error **errp)
>>>           goto out_region_err;
>>>       }
>>>   
>>> -    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX,
>>> &err);
>>> -    if (err) {
>>> +    if (!vfio_ccw_register_irq_notifier(vcdev,
>>> VFIO_CCW_IO_IRQ_INDEX, &err)) {
>>
>> Please pass errp instead of &err.
>>
>>>           goto out_io_notifier_err;
>>>       }
>>>   
>>>       if (vcdev->crw_region) {
>>> -        vfio_ccw_register_irq_notifier(vcdev,
>>> VFIO_CCW_CRW_IRQ_INDEX, &err);
>>> -        if (err) {
>>> +        if (!vfio_ccw_register_irq_notifier(vcdev,
>>> VFIO_CCW_CRW_IRQ_INDEX,
>>> +                                            &err)) {
>>
>> Likewise.
>>
>>>               goto out_irq_notifier_err;
>>>           }
>>>       }
>>>   
>>> -    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX,
>>> &err);
>>> -    if (err) {
>>> +    if (!vfio_ccw_register_irq_notifier(vcdev,
>>> VFIO_CCW_REQ_IRQ_INDEX, &err)) {
>>>           /*
>>>            * 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);
>>
>> Not this patch's problem, but here goes anyway: since this isn't an
>> error, we shouldn't use error_report_err().  Would warn_report_err()
>> be
>> appropriate?  info_report_err() doesn't exist, but it could.
>>
>> Preferably with errp instead of &err (two times):
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
> 
> Don't recall why I used error_report_err() instead of something else
> (or creating info_), but probably just familiarity. There's no need for
> it (or the equivalent code in -ap) to be error, and could be another
> cleanup.

yes. I will send an extra cleanup to replace error_... with warn_...
and another one to use errp.

Thanks,

C.


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

Re: [PATCH v2 1/4] vfio/ap: Use g_autofree variable in vfio_ap_register_irq_notifier()

[Anthony Krowiak]

On 4/25/24 5:02 AM, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/ap.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)


LGTM

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


>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 7c4caa5938636937680fec87e999249ac84a4498..03f8ffaa5e2bf13cf8daa2f44aa4cf17809abd94 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -77,7 +77,7 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>       size_t argsz;
>       IOHandler *fd_read;
>       EventNotifier *notifier;
> -    struct vfio_irq_info *irq_info;
> +    g_autofree struct vfio_irq_info *irq_info = NULL;
>       VFIODevice *vdev = &vapdev->vdev;
>   
>       switch (irq) {
> @@ -104,14 +104,14 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>       if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>                 irq_info) < 0 || irq_info->count < 1) {
>           error_setg_errno(errp, errno, "vfio: Error getting irq info");
> -        goto out_free_info;
> +        return;
>       }
>   
>       if (event_notifier_init(notifier, 0)) {
>           error_setg_errno(errp, errno,
>                            "vfio: Unable to init event notifier for irq (%d)",
>                            irq);
> -        goto out_free_info;
> +        return;
>       }
>   
>       fd = event_notifier_get_fd(notifier);
> @@ -122,10 +122,6 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>           qemu_set_fd_handler(fd, NULL, NULL, vapdev);
>           event_notifier_cleanup(notifier);
>       }
> -
> -out_free_info:
> -    g_free(irq_info);
> -
>   }
>   
>   static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,

Re: [PATCH v2 1/4] vfio/ap: Use g_autofree variable in vfio_ap_register_irq_notifier()

[Cédric Le Goater]

Applied series to vfio-next.

Thanks,

C.

On 4/25/24 11:02, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/ap.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 7c4caa5938636937680fec87e999249ac84a4498..03f8ffaa5e2bf13cf8daa2f44aa4cf17809abd94 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -77,7 +77,7 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>       size_t argsz;
>       IOHandler *fd_read;
>       EventNotifier *notifier;
> -    struct vfio_irq_info *irq_info;
> +    g_autofree struct vfio_irq_info *irq_info = NULL;
>       VFIODevice *vdev = &vapdev->vdev;
>   
>       switch (irq) {
> @@ -104,14 +104,14 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>       if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>                 irq_info) < 0 || irq_info->count < 1) {
>           error_setg_errno(errp, errno, "vfio: Error getting irq info");
> -        goto out_free_info;
> +        return;
>       }
>   
>       if (event_notifier_init(notifier, 0)) {
>           error_setg_errno(errp, errno,
>                            "vfio: Unable to init event notifier for irq (%d)",
>                            irq);
> -        goto out_free_info;
> +        return;
>       }
>   
>       fd = event_notifier_get_fd(notifier);
> @@ -122,10 +122,6 @@ static void vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>           qemu_set_fd_handler(fd, NULL, NULL, vapdev);
>           event_notifier_cleanup(notifier);
>       }
> -
> -out_free_info:
> -    g_free(irq_info);
> -
>   }
>   
>   static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,