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

[Erik Skultety]

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 at 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 at redhat.com>