Re: [Qemu-devel] Re: [PATCH 9/9] Introduce VLANClientState::cleanup()

[Mark McLoughlin <markmc@redhat.com>]

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.