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

[Michal Privoznik]

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 at redhat.com>
>> ---
>>  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