>-----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? > > >> 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 type of the device, hence the need for passing @name >> */ >> -static int vfio_legacy_attach_device(const char *name, VFIODevice >*vbasedev, >> - AddressSpace *as, Error **errp) >> +static bool vfio_legacy_attach_device(const char *name, VFIODevice >*vbasedev, >> + AddressSpace *as, Error **errp) >> { >> int groupid = vfio_device_groupid(vbasedev, errp); >> VFIODevice *vbasedev_iter; >> @@ -918,27 +918,27 @@ static int vfio_legacy_attach_device(const char >*name, VFIODevice *vbasedev, >> int ret; >> >> if (groupid < 0) { >> - return groupid; >> + return false; >> } >> >> trace_vfio_attach_device(vbasedev->name, groupid); >> >> group = vfio_get_group(groupid, as, errp); >> if (!group) { >> - return -ENOENT; >> + return false; >> } >> >> QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >> if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >> error_setg(errp, "device is already attached"); >> vfio_put_group(group); >> - return -EBUSY; >> + return false; >> } >> } >> ret = vfio_get_device(group, name, vbasedev, errp); >> if (ret) { > >vfio_get_device() would be the next candidate for cleanup. Yes. > >> vfio_put_group(group); >> - return ret; >> + return false; >> } >> >> bcontainer = &group->container->bcontainer; >> @@ -946,7 +946,7 @@ static int vfio_legacy_attach_device(const char >*name, VFIODevice *vbasedev, >> QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, >container_next); >> QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next); >> >> - return ret; >> + return true; >> } >> >> static void vfio_legacy_detach_device(VFIODevice *vbasedev) >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index 8827ffe636..9aa0dd6d8e 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -301,8 +301,8 @@ error: >> return ret; >> } >> >> -static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, >> - AddressSpace *as, Error **errp) >> +static bool iommufd_cdev_attach(const char *name, VFIODevice >*vbasedev, >> + AddressSpace *as, Error **errp) >> { >> VFIOContainerBase *bcontainer; >> VFIOIOMMUFDContainer *container; >> @@ -317,7 +317,7 @@ static int iommufd_cdev_attach(const char *name, >VFIODevice *vbasedev, >> if (vbasedev->fd < 0) { >> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); >> if (devfd < 0) { >> - return devfd; >> + return false; >> } >> vbasedev->fd = devfd; >> } else { >> @@ -394,7 +394,6 @@ static int iommufd_cdev_attach(const char *name, >VFIODevice *vbasedev, >> memory_listener_register(&bcontainer->listener, bcontainer->space- >>as); >> >> if (bcontainer->error) { >> - ret = -1; >> error_propagate_prepend(errp, bcontainer->error, >> "memory listener initialization failed: "); >> goto err_listener_register; >> @@ -433,7 +432,7 @@ found_container: >> >> trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev- >>num_irqs, >> vbasedev->num_regions, vbasedev->flags); >> - return 0; >> + return true; >> >> err_listener_register: >> iommufd_cdev_ram_block_discard_disable(false); >> @@ -446,7 +445,7 @@ err_alloc_ioas: >> iommufd_cdev_unbind_and_disconnect(vbasedev); >> err_connect_bind: >> close(vbasedev->fd); >> - return ret; >> + return false; >> } >> >> static void iommufd_cdev_detach(VFIODevice *vbasedev) >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 64780d1b79..952e4b1a25 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2951,7 +2951,7 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> int i, ret; >> bool is_mdev; >> char uuid[UUID_STR_LEN]; >> - char *name; >> + g_autofree char *name = NULL; >> >> if (vbasedev->fd < 0 && !vbasedev->sysfsdev) { >> if (!(~vdev->host.domain || ~vdev->host.bus || >> @@ -3001,10 +3001,8 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> name = g_strdup(vbasedev->name); >> } >> >> - ret = vfio_attach_device(name, vbasedev, >> - pci_device_iommu_address_space(pdev), errp); >> - g_free(name); > >This change would deserve another patch. Will do. Thanks Zhenzhong