Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Marcelo Tosatti <mtosatti@redhat.com>]

Hi Mark,

Nice!

On Wed, Apr 15, 2009 at 07:34:21PM +0200, Jan Kiszka wrote:
> Mark McLoughlin wrote:
> > We're currently leaking memory and file descriptors on device
> > hot-unplug.
> > 
> > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> > ---
> >  hw/e1000.c          |   12 +++++-----
> >  hw/eepro100.c       |   12 ++++++++++
> >  hw/etraxfs_eth.c    |   11 +++++++++
> >  hw/mcf_fec.c        |   18 ++++++++++++---
> >  hw/mipsnet.c        |   14 ++++++++++++
> >  hw/musicpal.c       |   18 ++++++++++++---
> >  hw/ne2000.c         |   24 +++++++++++++++++++++
> >  hw/pcnet.c          |   43 ++++++++++++++++++++++++++++++++++---
> >  hw/rtl8139.c        |   20 +++++++++++++++++
> >  hw/smc91c111.c      |   17 +++++++++++---
> >  hw/stellaris_enet.c |   20 ++++++++++++++---
> >  hw/usb-net.c        |   11 ++++++++-
> >  hw/virtio-net.c     |   14 ++++++++++++
> >  net.c               |   57 +++++++++++++++++++++++++++++++++++++-------------
> >  net.h               |    2 +
> >  tap-win32.c         |   13 +++++++++++
> >  16 files changed, 263 insertions(+), 43 deletions(-)
> > 
> > diff --git a/hw/e1000.c b/hw/e1000.c
> > index 2d16774..978f789 100644
> > --- a/hw/e1000.c
> > +++ b/hw/e1000.c
> > @@ -1033,14 +1033,14 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
> >                                       excluded_regs[i] - 4);
> >  }
> >  
> > -static int
> > -pci_e1000_uninit(PCIDevice *dev)
> > +static void
> > +e1000_cleanup(VLANClientState *vc)
> >  {
> > -    E1000State *d = (E1000State *) dev;
> > +    E1000State *d = vc->opaque;
> >  
> > -    cpu_unregister_io_memory(d->mmio_index);
> > +    unregister_savevm("e1000", d);
> >  
> > -    return 0;
> > +    cpu_unregister_io_memory(d->mmio_index);
> >  }
> >  
> >  PCIDevice *
> > @@ -1095,12 +1095,12 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn)
> >  
> >      d->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
> >                                   e1000_receive, e1000_can_receive, d);
> > +    d->vc->cleanup = e1000_cleanup;
> 
> Just to leave my comment here as well :) : I still consider this an
> important, mostly required callback that should be lifted into
> qemu_new_vlan_client(). That way, everyone who thinks (s)he doesn't need
> it will have to explicitly null'ify it.

Agreed.

> 
> >      d->vc->link_status_changed = e1000_set_link_status;
> >  
> >      qemu_format_nic_info_str(d->vc, nd->macaddr);
> >  
> >      register_savevm(info_str, -1, 2, nic_save, nic_load, d);
> > -    d->dev.unregister = pci_e1000_uninit;

I'm unsure about the fact that you consider device dependant details
such as MMIO addresses part of the "VLANClient" abstraction. Don't 
they belong to the PCI device, and as such, should be unregistered
in (PCIDevice *)->unregister?

> > +static void mcf_fec_cleanup(VLANClientState *vc)
> > +{
> > +    mcf_fec_state *s = vc->opaque;
> > +
> > +    cpu_unregister_io_memory(s->mmio_index);
> > +
> > +    qemu_free(s);
> > +}

Also the fact that you free the device structure in the non-PCI
functions, but you don't in the PCI functions (because generic PCI
code does it) is somewhat confusing.

Hum, I think abstracting away ISA devices would be a good thing.

> > +static void isa_ne2000_cleanup(VLANClientState *vc)
> > +{
> > +    NE2000State *s = vc->opaque;
> > +
> > +    unregister_savevm("ne2000", s);
> > +
> > +    isa_unassign_ioport(s->isa_io_base, 16);
> > +    isa_unassign_ioport(s->isa_io_base + 0x10, 2);
> > +    isa_unassign_ioport(s->isa_io_base + 0x1f, 1);
> > +
> > +    qemu_free(s);
> > +}
> Hot-plugging ISA device - scary... :)

Yeah :)

> > +static void ne2000_cleanup(VLANClientState *vc)
> > +{
> > +    NE2000State *s = vc->opaque;
> > +
> > +    unregister_savevm("ne2000", s);
> > +}

So unregister_savevm is common to all buses for the ne2000 chip, but
isa_unassign_ioport is not. So what about moving non-device specific
details to (VLANClientState *)->cleanup, and device specific to
(XXXDevice *)->unregister?

For example there was symmetry between lsi_scsi_unregister and
e1000_unregister before.

This would make the purpose of the interface you are creating clearer,
IMHO.