[libvirt] [PATCH v3 5/5] qemu: Connect to guest agent iff needed

[Martin Kletzander]

On Mon, Jan 11, 2016 at 11:31:12AM +0100, Michal Privoznik wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1293351
>
>Since we already have virtio channel events, we know when guest
>agent within guest has (dis-)connected. Instead of us blindly
>connecting to a socket that no one is listening to, we can just
>follow what qemu-ga does. This has a nice benefit that we don't
>need to 'guest-ping' the agent just to timeout and find out
>nobody is listening.
>
>The way that this commit is implemented:
>- don't connect in qemuProcessStart directly, defer that to event
>  callback (which already follows the agent).
>- after migration is settled, before we resume vCPUs, ask qemu
>  whether somebody is listening on the socket and if so, connect
>  to it.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/qemu/qemu_driver.c    | 21 ++++++++++++++++++++-
> src/qemu/qemu_migration.c | 20 ++++++++++++++++++++
> src/qemu/qemu_process.c   |  9 +++++++++
> 3 files changed, 49 insertions(+), 1 deletion(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 8ccf68b..43ef18a 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -6650,12 +6650,13 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
>                            bool start_paused,
>                            qemuDomainAsyncJob asyncJob)
> {
>-    int ret = -1;
>+    int rc, ret = -1;
>     virObjectEventPtr event;
>     int intermediatefd = -1;
>     virCommandPtr cmd = NULL;
>     char *errbuf = NULL;
>     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>+    qemuDomainObjPrivatePtr priv = vm->privateData;
>
>     if ((header->version == 2) &&
>         (header->compressed != QEMU_SAVE_FORMAT_RAW)) {
>@@ -6710,6 +6711,24 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
>         goto cleanup;
>     }
>
>+    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE)) {
>+        /* If QEMU is capable of vserport_change events, we ought to
>+         * refresh channel states here and possibly connect to the guest
>+         * agent too. */
>+        if (qemuProcessRefreshChannelState(driver, vm) < 0)
>+            goto cleanup;
>+
>+        if ((rc = qemuConnectAgent(driver, vm)) < 0) {
>+            if (rc == -2)
>+                goto cleanup;
>+
>+            VIR_WARN("Cannot connect to QEMU guest agent for %s",
>+                     vm->def->name);
>+            virResetLastError();
>+            priv->agentError = true;
>+        }
>+    }
>+

This pattern is repeating.  If we moved the channel state refresh into
qemuConnectAgent() (it could even be called only once per domain
lifetime, all other times we would depend on the state being correct),
mainly if there is a check for the state anyway, that would help in more
cases than just this one.  And more generally, we could always skip
connecting to the agent if qemu supports VSERPORT_CHANGE, so that
condition could be moved inside qemuConnectAgent() as well (or creating
a wrapper around it).  That way your problem would be also solved for
domains we connected to after libvirt's restart and all other cases that
none of us thought about.

If there is a reason against that, which I missed, then at least
putting this into it's own function, and considering calling it from
other parts of the code where qemuConnectAgent() is being called
currently, would be nice.

ACK to 1-4, by the way.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160111/942f4eb8/attachment-0001.sig>