Re: [libvirt] [PATCH v2] qemu: Connect to guest agent iff needed

On Tue, Dec 22, 2015 at 15:41:16 +0100, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1293351 > > I've came across a bit of a silly scenario, but long story short: > after domain was resumed, the virDomainSetTime() API hung for 5 > seconds after which it failed with an error. Problem was, that > there is no guest agent installed in the domain. But this got me > thinking and digging into the code. So even though we do listen > to VSERPORT_CHANGE events and connect and disconnect ourselves to > guest agent, we still do connect to the guest agent at both > domain startup and resume. This is a bit silly as it produces the > delay - basically, because we connect to the guest agent, > priv->agent is not NULL. Therefore qemuDomainAgentAvailable() > will return true. And the place where we hang? Well, > historically, when there was no VSERPORT_CHANGE event, we used a > dummy ping command to the guest which has 5 seconds timeout. And > it's still there and effective. So there's where the delay comes > from. > What's the resolution? Well, I'm introducing new capability > QEMU_CAPS_VSERPORT_CHANGE that is enabled iff qemu is capable of > the event. And, during domain startup, reconnect after resume and > attach we do connect to the agent socket iff the capability is > NOT set. I'd welcome a less verbose commit message containig the technical details and omitting the story around. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_capabilities.c | 2 ++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_domain.c | 11 ++++------- > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_driver.c | 2 +- > src/qemu/qemu_process.c | 25 +++++++++++++++++++------ > src/qemu/qemu_process.h | 4 +++- > tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + > tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + > tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + > 10 files changed, 34 insertions(+), 16 deletions(-) > [...] > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index f274068..3aca227 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -198,16 +198,29 @@ static qemuAgentCallbacks agentCallbacks = { > > > int > -qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) > +qemuConnectAgent(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + bool force) > { > qemuDomainObjPrivatePtr priv = vm->privateData; > int ret = -1; > qemuAgentPtr agent = NULL; > - virDomainChrSourceDefPtr config = qemuFindAgentConfig(vm->def); > + virDomainChrDefPtr config = qemuFindAgentConfig(vm->def); > > if (!config) > return 0; > > + if (!force && > + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE) && AFAIK the capability bit check won't be necessary here, since the event was introduced precisely at the same time as the ability to query the state via monitor and thus config->state will be _DISCONNECTED only if that exists.

> + config->state == VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED) { > + /* With newer qemus capable of VSERPORT_CHANGE event, we are connecting and > + * disconnecting to the guest agent as it connects or disconnects to the > + * channel within the guest. So, it's important to connect here iff we are > + * running qemu not capable of the event or we are called from the event > + * callback (@force == true). */ So basically this gets called with @force true just from the callback. But in the callback you call this function if and only if the serial port state is _CONNECTED. All other callers call this with unknown port state. The @force argument thus doesn't make much sense.