RE: [PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize()
>-Original Message-
>From: Cédric Le Goater
>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
>> Signed-off-by: Zhenzhong Duan
>> ---
>> 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 = >vdev;
>> -Error *err = NULL;
>>
>> /* Call the class init function for subchannel. */
>> if (cdc->realize) {
>> -cdc->realize(cdev, vcdev->vdev.sysfsdev, );
>> -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, )) {
>> +if (!vfio_ccw_get_region(vcdev, errp)) {
>> goto out_region_err;
>> }
>>
>> -if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX,
>)) {
>> +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,
>> -)) {
>> +errp)) {
>> goto out_irq_notifier_err;
>> }
>> }
>>
>> -if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX,
>)) {
>> +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
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
Signed-off-by: Zhenzhong Duan
---
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 = >vdev;
-Error *err = NULL;
/* Call the class init function for subchannel. */
if (cdc->realize) {
-cdc->realize(cdev, vcdev->vdev.sysfsdev, );
-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, )) {
+if (!vfio_ccw_get_region(vcdev, errp)) {
goto out_region_err;
}
-if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, )) {
+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,
-)) {
+errp)) {
goto out_irq_notifier_err;
}
}
-if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, )) {
+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)
[PATCH v2 19/20] vfio/ccw: Drop local @err in vfio_ccw_realize()
Use @errp to fetch error information directly and drop the local
variable @err.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
---
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 = >vdev;
-Error *err = NULL;
/* Call the class init function for subchannel. */
if (cdc->realize) {
-cdc->realize(cdev, vcdev->vdev.sysfsdev, );
-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, )) {
+if (!vfio_ccw_get_region(vcdev, errp)) {
goto out_region_err;
}
-if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, )) {
+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,
-)) {
+errp)) {
goto out_irq_notifier_err;
}
}
-if (!vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, )) {
+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)
--
2.34.1
