[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: |
Thu, 16 Apr 2009 17:44:03 -0300 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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.
- [Qemu-devel] [PATCH 6/9] Remove NICInfo from e1000 and mipsnet state, (continued)
- [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, 2009/04/15
- 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 <=
- 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
- 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/29
- Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup(), Avi Kivity, 2009/04/30