From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LuSut-0007V9-Ek for qemu-devel@nongnu.org; Thu, 16 Apr 2009 10:50:15 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LuSuo-0007T5-Sb for qemu-devel@nongnu.org; Thu, 16 Apr 2009 10:50:14 -0400 Received: from [199.232.76.173] (port=46822 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LuSuo-0007T2-Bt for qemu-devel@nongnu.org; Thu, 16 Apr 2009 10:50:10 -0400 Received: from mx2.redhat.com ([66.187.237.31]:58722) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LuSun-0008MR-OI for qemu-devel@nongnu.org; Thu, 16 Apr 2009 10:50:10 -0400 Subject: Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup() From: Mark McLoughlin In-Reply-To: <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> <20090416010725.GA24264@amt.cnet> Content-Type: text/plain Date: Thu, 16 Apr 2009 15:49:57 +0100 Message-Id: <1239893397.6860.190.camel@blaa> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Reply-To: Mark McLoughlin , qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcelo Tosatti Cc: Anthony Liguori , qemu-devel@nongnu.org, Markus Armbruster Hi Marcelo, On Wed, 2009-04-15 at 22:07 -0300, Marcelo Tosatti wrote: > On Wed, Apr 15, 2009 at 07:34:21PM +0200, Jan Kiszka wrote: > > Mark McLoughlin wrote: > > > @@ -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. Oi! You're the one that introduced this: d->dev.unregister = pci_e1000_uninit; which is basically the same thing. But ... "whatever" :-) > > > 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 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. I see where you're coming from, especially with the symmetry with block devices. However, the way I see it is that the VLANClientState should "own" the PCIDevice, not the other way around - e.g. you want to free the device, you should do qemu_del_vlan_client(), rather than pci_device_unregister(). What follows from that is VLANClientState::cleanup() should call pci_device_unregister(). If we did it the other way, then PCIDevice::unregister() should do qemu_del_vlan_client() and callers should never free a PCI NIC directly using del_vlan_client(), but instead call pci_device_unregister(). Futhermore, if we took the latter approach, we'd need a similar abstraction to PCIDevice for the non-PCI NICs. As for splitting the cleanups non-device specific and device specific parts ... for most devices, no such separation exists. We mix all the state up in the structure, and it's all allocated in the one place, so separating out the cleanup seems a bit arbitrary. Following up with an updated version of the patch which uses PCIDevice::unregister() for unregister I/O memory and VLANClientState::cleanup() for cleaning everything else up. It's not perfect, by any means ... but it's a baby step in the right direction IMHO. Cheers, Mark.