[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()
From: |
Marcelo Tosatti |
Subject: |
Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup() |
Date: |
Wed, 15 Apr 2009 22:07:25 -0300 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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 <address@hidden>
> > ---
> > 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.
- [Qemu-devel] [PATCH 1/9] Remove stray GSO code from virtio_net, (continued)
- [Qemu-devel] [PATCH 1/9] Remove stray GSO code from virtio_net, Mark McLoughlin, 2009/04/15
- [Qemu-devel] [PATCH 2/9] struct iovec is now universally available, Mark McLoughlin, 2009/04/15
- [Qemu-devel] [PATCH 3/9] Fix error handling in net_client_init(), Mark McLoughlin, 2009/04/15
- [Qemu-devel] [PATCH 4/9] Don't fail PCI hotplug if no NIC model is supplied, Mark McLoughlin, 2009/04/15
- [Qemu-devel] [PATCH 5/9] Remove some useless malloc() checking, Mark McLoughlin, 2009/04/15
- [Qemu-devel] [PATCH 6/9] Remove NICInfo from e1000 and mipsnet state, Mark McLoughlin, 2009/04/15
- [Qemu-devel] [PATCH 7/9] Add unregister_savevm(), Mark McLoughlin, 2009/04/15
- [Qemu-devel] [PATCH 8/9] Use NICInfo::model for eepro100 savevm ID string, Mark McLoughlin, 2009/04/15
- [Qemu-devel] [PATCH 9/9] Introduce VLANClientState::cleanup(), Mark McLoughlin, 2009/04/15
- [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Jan Kiszka, 2009/04/15
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(),
Marcelo Tosatti <=
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), M. Warner Losh, 2009/04/15
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Mark McLoughlin, 2009/04/16
- [Qemu-devel] [PATCH 09/09 v2] Introduce VLANClientState::cleanup(), Mark McLoughlin, 2009/04/16
- [Qemu-devel] [PATCH 10/09] Free VLANClientState using qemu_free(), Mark McLoughlin, 2009/04/16
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Marcelo Tosatti, 2009/04/16
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Paul Brook, 2009/04/28
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Anthony Liguori, 2009/04/28
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Paul Brook, 2009/04/28
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Anthony Liguori, 2009/04/28
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Paul Brook, 2009/04/28