>-----Original Message-----
>From: Joao Martins <joao.m.mart...@oracle.com>
>Sent: Wednesday, June 21, 2023 7:08 PM
>To: Duan, Zhenzhong <zhenzhong.d...@intel.com>
>Cc: alex.william...@redhat.com; c...@redhat.com; qemu-devel@nongnu.org;
>avih...@nvidia.com; Peng, Chao P <chao.p.p...@intel.com>
>Subject: Re: [PATCH v3 1/3] vfio/pci: Fix resource leak in vfio_realize
>
>
>
>On 21/06/2023 09:02, Zhenzhong Duan wrote:
>> When adding migration blocker failed in vfio_migration_realize(),
>> hotplug will fail and we see below:
>>
>> (qemu) device_add
>> vfio-pci,host=81:11.1,id=vfio1,bus=root1,x-enable-migration=true
>> 0000:81:11.1: VFIO migration is not supported in kernel
>> Error: disallowing migration blocker (--only-migratable) for: VFIO
>> device doesn't support migration
>>
>> If we hotplug again we should see same log as above, but we see:
>> (qemu) device_add
>> vfio-pci,host=81:11.0,id=vfio0,bus=root0,x-enable-migration=true
>> Error: vfio 0000:81:11.0: device is already attached
>>
>> That's because some references to VFIO device isn't released, we
>> should check return value of vfio_migration_realize() and release the
>> references, then VFIO device will be truely released when hotplug
>> failed.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>> ---
>> hw/vfio/pci.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
>> 73874a94de12..c71b0955d81c 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3210,6 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error
>**errp)
>> ret = vfio_migration_realize(vbasedev, errp);
>> if (ret) {
>> error_report("%s: Migration disabled", vbasedev->name);
>> + goto out_deregister;
>> }
>> }
>>
>This doesn't look right. This means that failure to support migration will
>deregister the device.
In my understanding, failure to support migration but success to add migration
blocker will not deregister device. vfio_migration_realize() is successful in
this case.
But failure to add migration blocker should deregister device, because
vfio_exitfn() is never called in this case(errp set), jumping to out_deregister
is the best choice. Then vfio_migration_realize() should return failure in this
case.
> Migration "realize" should not condition as to whether
>your device finishes the realize.
What's the impact if we condition it?
Thanks
Zhenzhong
