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>