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

[Marcelo Tosatti]

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.