[libvirt] [RFC PATCH 06/10] qemu: implement state driver shutdown function

[John Ferlan]

On 01/10/2018 01:05 PM, Daniel P. Berrange wrote:
> On Wed, Jan 10, 2018 at 12:23:31PM -0500, John Ferlan wrote:
>> From: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>>
>> Shutdown function should help API calls to finish when
>> event loop is not running anymore. For this reason let's
>> close agent and qemu monitors. These function will unblock
>> API calls wating for response from qemu process or qemu agent.
>>
>> Closing agent monitor and setting priv->agent to NULL when
>> waiting for response is normal usecase (handling EOF from
>> agent is handled the same way for example).
>>
>> However we can not do the same for qemu monitor. This monitor is normally
>> closed and unrefed during qemuProcessStop under destroy job so other threads
>> do not deal with priv->mon. But if we take extra reference to monitor
>> we are good. This can lead to double close but this function looks like to
>> be idempotent.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a203c9297..1de236cb5 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1093,6 +1093,44 @@ qemuStateStop(void)
>>      return ret;
>>  }
>>  
>> +
>> +static int
>> +qemuDomainDisconnect(virDomainObjPtr vm, void *opaque ATTRIBUTE_UNUSED)
>> +{
>> +
>> +    qemuDomainObjPrivatePtr priv;
>> +
>> +    virObjectLock(vm);
>> +    priv = vm->privateData;
>> +
>> +    if (priv->mon) {
>> +        /* Take extra reference to monitor so it won't be disposed
>> +         * by qemuMonitorClose last unref. */
>> +        virObjectRef(priv->mon);
>> +        qemuMonitorClose(priv->mon);
>> +    }
>> +
>> +    if (priv->agent) {
>> +        /* Other threads are ready for priv->agent to became NULL meanwhile */
>> +        qemuAgentClose(priv->agent);
>> +        priv->agent = NULL;
>> +    }
>> +
>> +    virObjectUnlock(vm);
>> +    return 0;
>> +}
>> +
>> +
>> +static void
>> +qemuStateShutdown(void)
>> +{
>> +    if (!qemu_driver)
>> +        return;
>> +
>> +    virDomainObjListForEach(qemu_driver->domains, qemuDomainDisconnect, NULL);
>> +}
>> +
>> +
>>  /**
>>   * qemuStateCleanup:
>>   *
>> @@ -21357,6 +21395,7 @@ static virStateDriver qemuStateDriver = {
>>      .stateCleanup = qemuStateCleanup,
>>      .stateReload = qemuStateReload,
>>      .stateStop = qemuStateStop,
>> +    .stateShutdown = qemuStateShutdown,
> 
> I'm curious why we need this separate  .stateShutdown hook. It feels like
> this code could just be placed at the start of qemuStateStop and achieve
> the same result.
> 
> 

This path is only run when compiled "WITH_DBUS" and it only matters when
(!virNetDaemonIsPrivileged(dmn)).

Also since qemuStateStop will suspend and save to disk all active
domains, it doesn't seem that's what may be desired in this case where
libvirtd is stopped.


John