Re: [Qemu-devel] [PATCH 1/2] ide: core: add cleanup function

[John Snow]

On 02/15/2017 04:26 AM, Li Qiang wrote:
> Hello,
> 
> 2017-02-15 7:30 GMT+08:00 John Snow <address@hidden
> <mailto:address@hidden>>:
> 
> 
> 
>     On 02/09/2017 08:22 PM, Li Qiang wrote:
>     > Hello John,
>     >
>     > 2017-02-10 3:42 GMT+08:00 John Snow <address@hidden 
> <mailto:address@hidden>
>     > <mailto:address@hidden <mailto:address@hidden>>>:
>     >
>     >
>     >
>     >     On 02/09/2017 02:04 AM, Li Qiang wrote:
>     >     > As the pci ahci can be hotplug and unplug, in the ahci unrealize
>     >     > function it should free all the resource once allocated in the
>     >     > realized function. This patch adds two cleanup function.
>     >     >
>     >     > Signed-off-by: Li Qiang <address@hidden
>     <mailto:address@hidden> <mailto:address@hidden
>     <mailto:address@hidden>>>
>     >     > ---
>     >     >  hw/ide/core.c             | 21 +++++++++++++++++++++
>     >     >  include/hw/ide/internal.h |  2 ++
>     >     >  2 files changed, 23 insertions(+)
>     >     >
>     >     > diff --git a/hw/ide/core.c b/hw/ide/core.c
>     >     > index 43709e5..8fe5896 100644
>     >     > --- a/hw/ide/core.c
>     >     > +++ b/hw/ide/core.c
>     >     > @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
>     >     >      }
>     >     >  }
>     >     >
>     >     > +void ide_unregister_restart_cb(IDEBus *bus)
>     >     > +{
>     >     > +    if (bus->dma->ops->restart_dma) {
>     >     > +        qemu_del_vm_change_state_handler(bus->vmstate);
>     >     > +    }
>     >     > +}
>     >     > +
>     >
>     >     Doesn't this conflict with qdev.c's idebus_unrealize call?
>     >
>     >
>     > As far as I can see, No conflict. Let's get a confirmation.
>     >
>     > Hello Paolo,
>     >
>     > Does this patch have any conflict/affect with the qdev?
>     >
>     > Thanks.
>     >
> 
>     They're both deleting the same VMstate handler, so as far as I can see
>     this is a redundant call.
> 
> 
> IIUC, the idebus_unrealize in qdev.c is for qdev model.
> But the added is for qom model.
> For example, if you use 'device_add ahci,id=ahci'/'device_del ahci' in
> the qmp.
> 
> The qemu will call 'pci_ich9_ahci_realize'/'pci_ich9_uninit'.
> 
> 
>  

I'm sorry, I still don't understand. Do you have some reproducer or case
where I can verify that this leaks?

It doesn't look as if you can hot-add or hot-remove an AHCI device right
now anyway, have you tested this?

Further, if the two calls AREN'T in conflict, I'd rather find some
cleanup mechanism that handles all the unrealize/uninit cases together
instead of having separate cleanup pathways.

--js