Re: [libvirt] [PATCH 5/5] libvirtd: Fix order of cleanup processing
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>
> ---
> 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 ;-)
> 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 someone else has
a more wonderful idea or set of patches that would avoid this... I could
add an XXX in front of which seems to be our norm of - someone someday
when they have copious free time and wants to pull their hair out could
actually fix this problem.
> + 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.
Nothing after this theoretically relies on @dmn still existing. Nothing
else uses it or has a reference to it....
John
I agree with the rest of the hunks.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>