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

On Wed, Jan 10, 2018 at 12:23:31PM -0500, John Ferlan wrote: > From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > > 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(a)redhat.com&gt; > --- > 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.