On 04.01.2016 16:28, Peter Krempa wrote: > 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@xxxxxxxxxx> >> --- >> 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. Okay if you say so. > >> + 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. Not true. We set the @config->state to _CONNECTED if and only if connecting to guest agent succeeded. Prior that @config->state is left untouched, i.e. is in _DISCONNECTED or _DEFAULT state. Moreover, I realized we will need to format @config->state into domain XML more often - e.g. during migration - we want to connect on the other side iff we are connected on the source. I have v3 patches ready. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list