>-----Original Message----- >From: Eric Auger <eric.au...@redhat.com> >Subject: Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device > > > >On 9/27/23 13:52, Duan, Zhenzhong wrote: >> >>> -----Original Message----- >>> From: Eric Auger <eric.au...@redhat.com> >>> Sent: Wednesday, September 27, 2023 5:16 PM >>> Subject: Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device >>> >>> Hi Zhenzhong, >>> >>> On 9/26/23 13:32, Zhenzhong Duan wrote: >>>> From: Eric Auger <eric.au...@redhat.com> >>>> >>>> Let the vfio-ap device use vfio_attach_device() and >>>> vfio_detach_device(), hence hiding the details of the used >>>> IOMMU backend. >>>> >>>> Signed-off-by: Eric Auger <eric.au...@redhat.com> >>>> Signed-off-by: Yi Liu <yi.l....@intel.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com> >>>> --- >>>> hw/vfio/ap.c | 68 +++++++++------------------------------------------- >>>> 1 file changed, 11 insertions(+), 57 deletions(-) >>>> >>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >>>> index 6e21d1da5a..16ea7fb3c2 100644 >>>> --- a/hw/vfio/ap.c >>>> +++ b/hw/vfio/ap.c >>>> @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = { >>>> .vfio_compute_needs_reset = vfio_ap_compute_needs_reset, >>>> }; >>>> >>>> -static void vfio_ap_put_device(VFIOAPDevice *vapdev) >>>> -{ >>>> - g_free(vapdev->vdev.name); >>>> - vfio_put_base_device(&vapdev->vdev); >>>> -} >>>> - >>>> -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp) >>>> -{ >>>> - GError *gerror = NULL; >>>> - char *symlink, *group_path; >>>> - int groupid; >>>> - >>>> - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev); >>>> - group_path = g_file_read_link(symlink, &gerror); >>>> - g_free(symlink); >>>> - >>>> - if (!group_path) { >>>> - error_setg(errp, "%s: no iommu_group found for %s: %s", >>>> - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, gerror- >>message); >>>> - g_error_free(gerror); >>>> - return NULL; >>>> - } >>>> - >>>> - if (sscanf(basename(group_path), "%d", &groupid) != 1) { >>>> - error_setg(errp, "vfio: failed to read %s", group_path); >>>> - g_free(group_path); >>>> - return NULL; >>>> - } >>>> - >>>> - g_free(group_path); >>>> - >>>> - return vfio_get_group(groupid, &address_space_memory, errp); >>>> -} >>>> - >>>> static void vfio_ap_req_notifier_handler(void *opaque) >>>> { >>>> VFIOAPDevice *vapdev = opaque; >>>> @@ -189,22 +155,15 @@ static void >>> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >>>> static void vfio_ap_realize(DeviceState *dev, Error **errp) >>>> { >>>> int ret; >>>> - char *mdevid; >>>> Error *err = NULL; >>>> - VFIOGroup *vfio_group; >>>> APDevice *apdev = AP_DEVICE(dev); >>>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >>>> + VFIODevice *vbasedev = &vapdev->vdev; >>>> >>>> - vfio_group = vfio_ap_get_group(vapdev, errp); >>>> - if (!vfio_group) { >>>> - return; >>>> - } >>>> - >>>> - vapdev->vdev.ops = &vfio_ap_ops; >>>> - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP; >>>> - mdevid = basename(vapdev->vdev.sysfsdev); >>>> - vapdev->vdev.name = g_strdup_printf("%s", mdevid); >>>> - vapdev->vdev.dev = dev; >>>> + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); >>> I think we shall document in the commit msg the fact we use >> Yes, will do. >> >>> g_path_get_basename instead of basename here to match other device init >>> see 3e015d815b use g_path_get_basename instead of basename >>> >>> also leak of vbasedev->name >> I free it in vfio_ap_unrealize(). >is it called if realize fails?
My understanding is: if realize fails, err path in realize() take this responsibility, if succeed, unrealize() should do that. So as you can see, I have done it in err path. if (ret) { g_free(vbasedev->name); return; } Zhenzhong