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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
