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

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. >

> >> + 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