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

[John Ferlan]

On 11/16/2017 02:58 AM, Erik Skultety wrote:
> 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 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.
>>>
>>
>> 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"...
> 

I don't disagree about implicit rationale - unfortunately as seen
in this cleanup reality is further from those logical thoughts.

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


Fair enough - point made.  I'll adjust as follows and add the extra
commentary about drivers initialized in the commit message for anyone
that cares to go back through history... FWIW, all of cleanup:

 cleanup:
    /* Keep cleanup order in inverse order of startup */
    virNetDaemonClose(dmn);

    virNetlinkEventServiceStopAll();

    if (driversInitialized) {
        /* NB: Possible issue with timing window between driversInitialized
         * setting if virNetlinkEventServerStart fails */
        driversInitialized = false;
        virStateCleanup();
    }

    virObjectUnref(adminProgram);
    virObjectUnref(srvAdm);
    virObjectUnref(qemuProgram);
    virObjectUnref(lxcProgram);
    virObjectUnref(remoteProgram);
    virObjectUnref(srv);
    virObjectUnref(dmn);

    virNetlinkShutdown();

    if (pid_file_fd != -1)
        virPidFileReleasePath(pid_file, pid_file_fd);

    VIR_FREE(run_dir);

    if (statuswrite != -1) {
        if (ret != 0) {
            /* Tell parent of daemon what failed */
            char status = ret;
            while (write(statuswrite, &status, 1) == -1 &&
                   errno == EINTR)
                ;
        }
        VIR_FORCE_CLOSE(statuswrite);
    }

    VIR_FREE(sock_file);
    VIR_FREE(sock_file_ro);
    VIR_FREE(sock_file_adm);

    VIR_FREE(pid_file);

    VIR_FREE(remote_config_file);
    daemonConfigFree(config);

    return ret;


John