Re: [Qemu-devel] [PATCH 3/5] spapr: Add DRC release method

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]

On Tue, Jun 20, 2017 at 02:24:08PM -0500, Michael Roth wrote:
> Quoting Greg Kurz (2017-06-20 11:51:45)
> > On Tue, 20 Jun 2017 09:53:30 +0800
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > At the moment, spapr_drc_release() has an ugly switch on the DRC type to
> > > call the right, device-specific release function.  This cleans it up by
> > > doing that via a proper QOM method.
> > > 
> > > It's still arguably an abstraction violation for the DRC code to call into
> > > the specific device code, but one mess at a time.
> > > 
> > 
> > Maybe a second step would be to move DRC subclasses to spapr.c (CPU, LMB) and
> > spapr_pci.c (PCI) ? This would allow each device-specific release function to
> > have local scope at least.

I was already thinking of doing that in a future patch :)

> I kind of prefer the proposed approach of registering a callback
> function via drc_new(). This would make it easier to implement
> unit tests without needing to rely on stub functions, and keeps the
> type-specific handling constrained to matters relating to the DRC
> itself and not the resources associated with them.

True.  We went from the callback to this ugly switch when we made the
DRCs migratable (well, sort of, except for the many remaining
migration problems these cleanups are addressing amongst other
things).  But moving the callback registration from attach time to new
time would do just as well.  Well I'll think about it and do one or
the other in future.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]