From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LuYRl-0005F5-Ki for qemu-devel@nongnu.org; Thu, 16 Apr 2009 16:44:33 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LuYRg-0005Eb-If for qemu-devel@nongnu.org; Thu, 16 Apr 2009 16:44:32 -0400 Received: from [199.232.76.173] (port=40071 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LuYRg-0005EW-8Y for qemu-devel@nongnu.org; Thu, 16 Apr 2009 16:44:28 -0400 Received: from mx2.redhat.com ([66.187.237.31]:60577) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LuYRf-0008K1-NB for qemu-devel@nongnu.org; Thu, 16 Apr 2009 16:44:28 -0400 Date: Thu, 16 Apr 2009 17:44:03 -0300 From: Marcelo Tosatti Subject: Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup() Message-ID: <20090416204403.GA16000@amt.cnet> References: <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> <1239893397.6860.190.camel@blaa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1239893397.6860.190.camel@blaa> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark McLoughlin Cc: Anthony Liguori , qemu-devel@nongnu.org, Markus Armbruster On Thu, Apr 16, 2009 at 03:49:57PM +0100, Mark McLoughlin wrote: > 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. The purpose of the pci unregister function, right. > 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. My point of view was that you have two functions that cleanup/free the device structure, and that is a little confusing. But its not a big deal, compared to the present bugs which the patch fixes. > 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. Agree.