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@redhat.com> --- 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.
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... ...
+ virObjectUnref(dmn);
Why ^here? Just because we're doing the cleanup the exact opposite way? The approach is right, but I think in general cleanup routines should be run first followed by releasing all the objects, so this should stay where it is right now - at the very end. I agree with the rest of the hunks. Reviewed-by: Erik Skultety <eskultet@redhat.com>