| Message ID | 20240522044015.412951-20-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFIO: misc cleanups part2 | expand |
On 5/22/24 06:40, Zhenzhong Duan wrote: > Use @errp to fetch error information directly and drop the local > variable @err. > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/vfio/ccw.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 2600e62e37..168c9e5973 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -574,17 +574,17 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) > > static void vfio_ccw_realize(DeviceState *dev, Error **errp) > { > + ERRP_GUARD(); > S390CCWDevice *cdev = S390_CCW_DEVICE(dev); > VFIOCCWDevice *vcdev = VFIO_CCW(cdev); > S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > VFIODevice *vbasedev = &vcdev->vdev; > - Error *err = NULL; > > /* Call the class init function for subchannel. */ > if (cdc->realize) { > - cdc->realize(cdev, vcdev->vdev.sysfsdev, &err); > - if (err) { > - goto out_err_propagate; > + cdc->realize(cdev, vcdev->vdev.sysfsdev, errp); > + if (*errp) { > + return; We should change the realize() return value to bool also. this is more work and it should be addressed in its own patchset I think and ... > } > } > > @@ -597,27 +597,28 @@ 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; > } > } > > - if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err)) { > + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, 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); > + error_report_err(*errp); This should use a local Error variable and be a warn_report_err instead. Let's address these changes in another series. I can take care of it later if no one does. Thanks, C. > + *errp = NULL; > } > > return; > @@ -635,8 +636,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)
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize() > >On 5/22/24 06:40, Zhenzhong Duan wrote: >> Use @errp to fetch error information directly and drop the local >> variable @err. >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/vfio/ccw.c | 21 ++++++++++----------- >> 1 file changed, 10 insertions(+), 11 deletions(-) >> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >> index 2600e62e37..168c9e5973 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c >> @@ -574,17 +574,17 @@ static void >vfio_ccw_put_region(VFIOCCWDevice *vcdev) >> >> static void vfio_ccw_realize(DeviceState *dev, Error **errp) >> { >> + ERRP_GUARD(); >> S390CCWDevice *cdev = S390_CCW_DEVICE(dev); >> VFIOCCWDevice *vcdev = VFIO_CCW(cdev); >> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); >> VFIODevice *vbasedev = &vcdev->vdev; >> - Error *err = NULL; >> >> /* Call the class init function for subchannel. */ >> if (cdc->realize) { >> - cdc->realize(cdev, vcdev->vdev.sysfsdev, &err); >> - if (err) { >> - goto out_err_propagate; >> + cdc->realize(cdev, vcdev->vdev.sysfsdev, errp); >> + if (*errp) { >> + return; > >We should change the realize() return value to bool also. this is more >work and it should be addressed in its own patchset I think and ... > >> } >> } >> >> @@ -597,27 +597,28 @@ 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; >> } >> } >> >> - if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, >&err)) { >> + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, >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); >> + error_report_err(*errp); > >This should use a local Error variable and be a warn_report_err instead. Yes. > >Let's address these changes in another series. I can take care of it >later if no one does. OK, leave it to you
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 2600e62e37..168c9e5973 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -574,17 +574,17 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) static void vfio_ccw_realize(DeviceState *dev, Error **errp) { + ERRP_GUARD(); S390CCWDevice *cdev = S390_CCW_DEVICE(dev); VFIOCCWDevice *vcdev = VFIO_CCW(cdev); S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); VFIODevice *vbasedev = &vcdev->vdev; - Error *err = NULL; /* Call the class init function for subchannel. */ if (cdc->realize) { - cdc->realize(cdev, vcdev->vdev.sysfsdev, &err); - if (err) { - goto out_err_propagate; + cdc->realize(cdev, vcdev->vdev.sysfsdev, errp); + if (*errp) { + return; } } @@ -597,27 +597,28 @@ 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; } } - if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err)) { + if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, 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); + error_report_err(*errp); + *errp = NULL; } return; @@ -635,8 +636,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)
Use @errp to fetch error information directly and drop the local variable @err. Suggested-by: Cédric Le Goater <clg@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/ccw.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)