>-----Original Message-----
>From: Cédric Le Goater <c...@redhat.com>
>Subject: Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and
>its wrapper return bool
>
>On 5/7/24 04:09, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <c...@redhat.com>
>>> Subject: Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device()
>and
>>> its wrapper return bool
>>>
>>> On 5/6/24 10:33, Zhenzhong Duan wrote:
>>>> Make VFIOIOMMUClass::attach_device() and its wrapper function
>>>> vfio_attach_device() return bool.
>>>>
>>>> This is to follow the coding standand to return bool if 'Error **'
>>>> is used to pass error.
>>>>
>>>> Suggested-by: Cédric Le Goater <c...@redhat.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>>>> ---
>>>> include/hw/vfio/vfio-common.h | 4 ++--
>>>> include/hw/vfio/vfio-container-base.h | 4 ++--
>>>> hw/vfio/ap.c | 6 ++----
>>>> hw/vfio/ccw.c | 6 ++----
>>>> hw/vfio/common.c | 4 ++--
>>>> hw/vfio/container.c | 14 +++++++-------
>>>> hw/vfio/iommufd.c | 11 +++++------
>>>> hw/vfio/pci.c | 8 +++-----
>>>> hw/vfio/platform.c | 7 +++----
>>>> 9 files changed, 28 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>> common.h
>>>> index b9da6c08ef..a7b6fc8f46 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -198,8 +198,8 @@ void vfio_region_exit(VFIORegion *region);
>>>> void vfio_region_finalize(VFIORegion *region);
>>>> void vfio_reset_handler(void *opaque);
>>>> struct vfio_device_info *vfio_get_device_info(int fd);
>>>> -int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>> - AddressSpace *as, Error **errp);
>>>> +bool vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>> + AddressSpace *as, Error **errp);
>>>> void vfio_detach_device(VFIODevice *vbasedev);
>>>>
>>>> int vfio_kvm_device_add_fd(int fd, Error **errp);
>>>> diff --git a/include/hw/vfio/vfio-container-base.h
>b/include/hw/vfio/vfio-
>>> container-base.h
>>>> index 3582d5f97a..c839cfd9cb 100644
>>>> --- a/include/hw/vfio/vfio-container-base.h
>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>> @@ -118,8 +118,8 @@ struct VFIOIOMMUClass {
>>>> int (*dma_unmap)(const VFIOContainerBase *bcontainer,
>>>> hwaddr iova, ram_addr_t size,
>>>> IOMMUTLBEntry *iotlb);
>>>> - int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>> - AddressSpace *as, Error **errp);
>>>> + bool (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>> + AddressSpace *as, Error **errp);
>>>> void (*detach_device)(VFIODevice *vbasedev);
>>>> /* migration feature */
>>>> int (*set_dirty_page_tracking)(const VFIOContainerBase
>*bcontainer,
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 7c4caa5938..d50600b702 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -156,7 +156,6 @@ static void
>>> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
>>>> static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>>> {
>>>> ERRP_GUARD();
>>>> - int ret;
>>>> Error *err = NULL;
>>>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
>>>> VFIODevice *vbasedev = &vapdev->vdev;
>>>> @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev,
>Error
>>> **errp)
>>>> return;
>>>> }
>>>>
>>>> - ret = vfio_attach_device(vbasedev->name, vbasedev,
>>>> - &address_space_memory, errp);
>>>> - if (ret) {
>>>> + if (!vfio_attach_device(vbasedev->name, vbasedev,
>>>> + &address_space_memory, errp)) {
>>>> goto error;
>>>> }
>>>>
>>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>>> index 90e4a53437..782bd4bed7 100644
>>>> --- a/hw/vfio/ccw.c
>>>> +++ b/hw/vfio/ccw.c
>>>> @@ -580,7 +580,6 @@ static void vfio_ccw_realize(DeviceState *dev,
>>> Error **errp)
>>>> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
>>>> VFIODevice *vbasedev = &vcdev->vdev;
>>>> Error *err = NULL;
>>>> - int ret;
>>>>
>>>> /* Call the class init function for subchannel. */
>>>> if (cdc->realize) {
>>>> @@ -594,9 +593,8 @@ static void vfio_ccw_realize(DeviceState *dev,
>>> Error **errp)
>>>> return;
>>>> }
>>>>
>>>> - ret = vfio_attach_device(cdev->mdevid, vbasedev,
>>>> - &address_space_memory, errp);
>>>> - if (ret) {
>>>> + if (!vfio_attach_device(cdev->mdevid, vbasedev,
>>>> + &address_space_memory, errp)) {
>>>> goto out_attach_dev_err;
>>>> }
>>>>
>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>> index 8f9cbdc026..890d30910e 100644
>>>> --- a/hw/vfio/common.c
>>>> +++ b/hw/vfio/common.c
>>>> @@ -1492,8 +1492,8 @@ retry:
>>>> return info;
>>>> }
>>>>
>>>> -int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>> - AddressSpace *as, Error **errp)
>>>> +bool vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>> + AddressSpace *as, Error **errp)
>>>> {
>>>> const VFIOIOMMUClass *ops =
>>>>
>>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>>>
>>>
>>> I think vfio_attach_device() can be cleaned up a little further :
>>>
>>> ret = ops->attach_device(name, vbasedev, as, errp);
>>> if (ret < 0) {
>>> return ret;
>>> }
>>
>> Not understand this.
>> I have both ops->attach_device() and vfio_attach_device() return bool
>> in this patch. Do you mean drop the change for ops->attach_device()?
>> Or split the two changes?
>
>The return value of ops->attach_device() should be tested as a bool now
>and not as an int, as it is still currently done.
>
>See the fix here :
> https://gitlab.com/legoater/qemu/-
>/commit/abae88dc28e723745fe2ff506ea9f0adeb47afdb
>
>Please simply fold the changes in this patch.
Clear, I'll fold it in hiod v4 series.
Thanks
Zhenzhong
