| Message ID | 20240425090214.400194-5-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] vfio/ap: Use g_autofree variable in vfio_ap_register_irq_notifier() | expand |
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>
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>
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>
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.
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(-)