Re: [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize()

["Cédric Le Goater" <clg@redhat.com>]

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)