From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LuG5W-0002e5-2a for qemu-devel@nongnu.org; Wed, 15 Apr 2009 21:08:22 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LuG5R-0002ca-Nd for qemu-devel@nongnu.org; Wed, 15 Apr 2009 21:08:21 -0400 Received: from [199.232.76.173] (port=52707 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LuG5R-0002cV-JZ for qemu-devel@nongnu.org; Wed, 15 Apr 2009 21:08:17 -0400 Received: from mx2.redhat.com ([66.187.237.31]:41413) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LuG5Q-0002GY-UR for qemu-devel@nongnu.org; Wed, 15 Apr 2009 21:08:17 -0400 Date: Wed, 15 Apr 2009 22:07:25 -0300 From: Marcelo Tosatti Subject: Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup() Message-ID: <20090416010725.GA24264@amt.cnet> References: <1239812969-8320-2-git-send-email-markmc@redhat.com> <1239812969-8320-3-git-send-email-markmc@redhat.com> <1239812969-8320-4-git-send-email-markmc@redhat.com> <1239812969-8320-5-git-send-email-markmc@redhat.com> <1239812969-8320-6-git-send-email-markmc@redhat.com> <1239812969-8320-7-git-send-email-markmc@redhat.com> <1239812969-8320-8-git-send-email-markmc@redhat.com> <1239812969-8320-9-git-send-email-markmc@redhat.com> <1239812969-8320-10-git-send-email-markmc@redhat.com> <49E61A9D.2060605@siemens.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <49E61A9D.2060605@siemens.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Mark McLoughlin , Anthony Liguori , Markus Armbruster 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 > > --- > > 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.