Re: [libvirt] [PATCH 5/5] libvirtd: Fix order of cleanup processing

On Fri, Nov 10, 2017 at 05:51:26PM -0500, John Ferlan wrote: > > > On 11/10/2017 10:23 AM, Erik Skultety wrote: >> On Tue, Nov 07, 2017 at 09:39:56PM -0500, John Ferlan wrote: >>> Current cleanup processing is ad-hoc at best - it's led to a couple of >>> strange and hard to diagnose timing problems and crashes. >>> >>> So rather than perform cleanup in a somewhat random order, let's >>> perform cleanup in the exact opposite order of startup. >>> >>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>> daemon/libvirtd.c | 42 +++++++++++++++++++++++++++++------------- >>> 1 file changed, 29 insertions(+), 13 deletions(-) >>> >>> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c >>> index 87c5b22710..92037e9070 100644 >>> --- a/daemon/libvirtd.c >>> +++ b/daemon/libvirtd.c >>> @@ -1524,9 +1524,35 @@ int main(int argc, char **argv) { >>> 0, "shutdown", NULL, NULL); >>> >>> cleanup: >>> - virNetlinkEventServiceStopAll(); >>> + /* NB: Order for cleanup should attempt to kept in the inverse order >>> + * was was used to create/start the daemon - there are some fairly >>> + * important reliances built into the startup processing that use >>> + * refcnt's in order to manage objects. Removing too early could >>> + * cause crashes. */ >> >> ^Not a very useful commentary, more suitable for the commit message - so I >> preferred if you'd drop it from here. >> > > I like it here because I won't necessarily go find the commit that added > this code in order to know that we need to be very careful and orderly > about the cleanup order. > > The only time I'd go back to a commit message is to perhaps point out > why the order is the way it is when some other patch f*ks it up ;-) Well, I think that releasing memory in the inverse order should be an implicit rationale, you could at least shorten the commentary to something as simple as "Cleanup order needs to be kept strictly in the inverse order"...

> >>> virNetDaemonClose(dmn); >>> + >>> + virNetlinkEventServiceStopAll(); >>> + >>> + if (driversInitialized) { >>> + /* Set via daemonRunStateInit thread in daemonStateInit. >>> + * NB: It is possible that virNetlinkEventServerStart fails >>> + * and we jump to cleanup before driversInitialized has >>> + * been set. That could leave things inconsistent; however, >>> + * resolution of that possibility is perhaps more trouble than >>> + * it's worth to handle. */ >> >> True, I guess, nevertheless it's not very useful either... >> >> ... >> > > It would be for someone chasing a very rogue and odd driver core. I > think it was important to know/understand that there is an open window > here which someone could drive a truck through. Maybe I'm just paranoid enough to expect such a window to exist everywhere... not a fan of the commentary though... Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;