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

[Duan, Zhenzhong]

>-----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😊

Thanks
Zhenzhong