RE: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool
>-Original Message- >From: Cédric Le Goater >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 >>> 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 >>>> Signed-off-by: Zhenzhong Duan >>>> --- >>>>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 = >vdev; >>>> @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, >Error >>> **errp) >>>>return; >>>>} >>>> >>>> -ret = vfio_attach_device(vbasedev->name, vbasedev, >>>> - _space_memory, errp); >>>> -if (ret) { >>>> +if (!vfio_attach_device(vbasedev->name, vbasedev, >>>> +_space_memory, errp)) { >>>>goto error; >>>>} >>>> >>>>
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 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 Signed-off-by: Zhenzhong Duan --- 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 = >vdev; @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) return; } -ret = vfio_attach_device(vbasedev->name, vbasedev, - _space_memory, errp); -if (ret) { +if (!vfio_attach_device(vbasedev->name, vbasedev, +_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 = >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, - _space_memory, errp); -if (ret) { +if (!vfio_attach_device(cdev->mdevid, vbasedev, +_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() a
RE: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool
>-Original Message- >From: Cédric Le Goater >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 >> Signed-off-by: Zhenzhong Duan >> --- >> 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 = >vdev; >> @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, Error >**errp) >> return; >> } >> >> -ret = vfio_attach_device(vbasedev->name, vbasedev, >> - _space_memory, errp); >> -if (ret) { >> +if (!vfio_attach_device(vbasedev->name, vbasedev, >> +_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 = >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, >> - _space_memory, errp); >> -if (ret) { >> +if (!vfio_attach_device(cdev->mdevid, vbasedev, >> +_space_memory, errp))
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 Signed-off-by: Zhenzhong Duan --- 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 = >vdev; @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) return; } -ret = vfio_attach_device(vbasedev->name, vbasedev, - _space_memory, errp); -if (ret) { +if (!vfio_attach_device(vbasedev->name, vbasedev, +_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 = >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, - _space_memory, errp); -if (ret) { +if (!vfio_attach_device(cdev->mdevid, vbasedev, +_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; } diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 77bdec276e..ea3b145913 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -908,8 +908,8 @@ static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) * @name and @vbasedev->name are likely to be different depending * on the