[libvirt] [PATCH v2] Add API to change qemu agent response timeout

Michal Privoznik mprivozn at redhat.com
Fri Nov 8 10:46:08 UTC 2019


On 11/6/19 10:13 PM, Jonathon Jongsma wrote:
> Some layered products such as oVirt have requested a way to avoid being
> blocked by guest agent commands when querying a loaded vm. For example,
> many guest agent commands are polled periodically to monitor changes,
> and rather than blocking the calling process, they'd prefer to simply
> time out when an agent query is taking too long.
> 
> This patch adds a way for the user to specify a custom agent timeout
> that is applied to all agent commands.
> 
> One special case to note here is the 'guest-sync' command. 'guest-sync'
> is issued internally prior to calling any other command. (For example,
> when libvirt wants to call 'guest-get-fsinfo', we first call
> 'guest-sync' and then call 'guest-get-fsinfo').
> 
> Previously, the 'guest-sync' command used a 5-second timeout
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
> followed always blocked indefinitely
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
> custom timeout is specified that is shorter than
> 5 seconds,  this new timeout is also used for 'guest-sync'. If there is
> no custom timeout or if the custom timeout is longer than 5 seconds, we
> will continue to use the 5-second timeout.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=1705426 for additional details.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
> I've addressed most of the previous review comments from Peter, Daniel, and
> Michal but I'm unsure yet whether there's concensus on this feature. Here's a
> v2 for your consideration.
> 
> I admit that the proposed API is not perfect. It would probably be more elegant
> if each agent command instead had its own 'timeout' argument so that the caller
> could specify the timeout for each invocation. But that is not the case, and
> introducing duplicate API for each agent command (with an additional timeout
> argument) would clutter the public API. In addition, we still have the issue
> Michal mentioned elsewhere in the thread where some commands like shutdown may
> or may not use the agent, so a timeout argument could be confusing.
> 
> So: is this a viable approach, or should I rethink it?

It's on the right path, but there are some issues. See below. It's not 
this simple.

> 
> Changes in v2:
> - Make this an official public API rather than putting it in libvirt-qemu.
> - Don't use the qemuDomainObjEnterAgent()/ExitAgent() API, which expects you
>    to acquire an agent job. Instead, introduce
>    qemuDomainObjSetAgentResponseTimeout() which simply locks the agent while
>    setting the variable.
> - rename the function slightly for better descriptiveness:
>    virDomainQemuAgentSetTimeout() -> virDomainAgentSetResponseTimeout()
> - added 'flags' argument for future-proofing.
> 
>   include/libvirt/libvirt-domain.h |  9 ++++
>   include/libvirt/libvirt-qemu.h   |  8 +--
>   src/driver-hypervisor.h          |  6 +++
>   src/libvirt-domain.c             | 45 +++++++++++++++++
>   src/libvirt_public.syms          |  1 +
>   src/qemu/qemu_agent.c            | 84 ++++++++++++++++++++------------
>   src/qemu/qemu_agent.h            |  4 ++
>   src/qemu/qemu_domain.c           | 15 ++++++
>   src/qemu/qemu_domain.h           |  5 ++
>   src/qemu/qemu_driver.c           | 22 +++++++++
>   src/remote/remote_driver.c       |  1 +
>   src/remote/remote_protocol.x     | 18 ++++++-
>   src/remote_protocol-structs      |  9 ++++
>   13 files changed, 190 insertions(+), 37 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 22277b0a84..83f6c1b835 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4916,4 +4916,13 @@ int virDomainGetGuestInfo(virDomainPtr domain,
>                             int *nparams,
>                             unsigned int flags);
>   
> +typedef enum {
> +    VIR_DOMAIN_AGENT_COMMAND_BLOCK = -2,
> +    VIR_DOMAIN_AGENT_COMMAND_DEFAULT = -1,
> +    VIR_DOMAIN_AGENT_COMMAND_NOWAIT = 0,
> +} virDomainAgentCommandTimeoutValues;
> +int virDomainAgentSetResponseTimeout(virDomainPtr domain,
> +                                     int timeout,
> +                                     unsigned int flags);
> +
>   #endif /* LIBVIRT_DOMAIN_H */
> diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h
> index 891617443f..4a541167ad 100644
> --- a/include/libvirt/libvirt-qemu.h
> +++ b/include/libvirt/libvirt-qemu.h
> @@ -43,10 +43,10 @@ virDomainPtr virDomainQemuAttach(virConnectPtr domain,
>                                    unsigned int flags);
>   
>   typedef enum {
> -    VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN = -2,
> -    VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK = -2,
> -    VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT = -1,
> -    VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT = 0,
> +    VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN = VIR_DOMAIN_AGENT_COMMAND_BLOCK,
> +    VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK = VIR_DOMAIN_AGENT_COMMAND_BLOCK,
> +    VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT = VIR_DOMAIN_AGENT_COMMAND_DEFAULT,
> +    VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT = VIR_DOMAIN_AGENT_COMMAND_NOWAIT,
>       VIR_DOMAIN_QEMU_AGENT_COMMAND_SHUTDOWN = 60,
>   } virDomainQemuAgentCommandTimeoutValues;
>   
> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
> index 015b2cd01c..4afd8f6ec5 100644
> --- a/src/driver-hypervisor.h
> +++ b/src/driver-hypervisor.h
> @@ -1372,6 +1372,11 @@ typedef int
>                               int *nparams,
>                               unsigned int flags);
>   
> +typedef int
> +(*virDrvDomainAgentSetResponseTimeout)(virDomainPtr domain,
> +                                       int timeout,
> +                                       unsigned int flags);
> +
>   typedef struct _virHypervisorDriver virHypervisorDriver;
>   typedef virHypervisorDriver *virHypervisorDriverPtr;
>   
> @@ -1632,4 +1637,5 @@ struct _virHypervisorDriver {
>       virDrvDomainCheckpointGetParent domainCheckpointGetParent;
>       virDrvDomainCheckpointDelete domainCheckpointDelete;
>       virDrvDomainGetGuestInfo domainGetGuestInfo;
> +    virDrvDomainAgentSetResponseTimeout domainAgentSetResponseTimeout;
>   };
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index dcab179e6e..b4e1b18164 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -12497,3 +12497,48 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
>       virDispatchError(domain->conn);
>       return -1;
>   }
> +
> +/**
> + * virDomainAgentSetResponseTimeout:
> + * @domain: a domain object
> + * @timeout: timeout in seconds
> + * @flags: extra flags; not used yet, so callers should always pass 0
> + *
> + * Set how long to wait for a response from qemu agent commands. By default,
> + * agent commands block forever waiting for a response.
> + *
> + * @timeout must be -2, -1, 0 or positive.
> + * VIR_DOMAIN_AGENT_COMMAND_BLOCK(-2): meaning to block forever waiting for a
> + * result.
> + * VIR_DOMAIN_AGENT_COMMAND_DEFAULT(-1): use default timeout value.
> + * VIR_DOMAIN_AGENT_COMMAND_NOWAIT(0): does not wait.
> + * positive value: wait for @timeout seconds
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +virDomainAgentSetResponseTimeout(virDomainPtr domain,
> +                                 int timeout,
> +                                 unsigned int flags)
> +{
> +    virConnectPtr conn;
> +    VIR_DOMAIN_DEBUG(domain, "timeout=%i, flags=0x%x",
> +                     timeout, flags);
> +
> +    virResetLastError();
> +
> +    virCheckDomainReturn(domain, -1);
> +    conn = domain->conn;
> +
> +    if (conn->driver->domainAgentSetResponseTimeout) {
> +        if (conn->driver->domainAgentSetResponseTimeout(domain, timeout, flags) < 0)
> +            goto error;
> +        return 0;
> +    }
> +
> +    virReportUnsupportedError();
> +
> + error:
> +    virDispatchError(conn);
> +    return -1;
> +}
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 40655fbbf5..74d5e0b4b9 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -859,6 +859,7 @@ LIBVIRT_5.7.0 {
>   
>   LIBVIRT_5.8.0 {
>           virConnectSetIdentity;
> +        virDomainAgentSetResponseTimeout;
>   } LIBVIRT_5.7.0;

This needs to be updated.

>   
>   # .... define new API here using predicted next version number ....
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 4b914e6d3b..c2f9a025ba 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -127,6 +127,7 @@ struct _qemuAgent {
>        * but fire up an event on qemu monitor instead.
>        * Take that as indication of successful completion */
>       qemuAgentEvent await_event;
> +    int timeout;
>   };
>   
>   static virClassPtr qemuAgentClass;
> @@ -695,6 +696,8 @@ qemuAgentOpen(virDomainObjPtr vm,
>       if (!(mon = virObjectLockableNew(qemuAgentClass)))
>           return NULL;
>   
> +    /* agent commands block by default, user can choose different behavior */
> +    mon->timeout = VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK;

This needs to be changed, see below [1].

>       mon->fd = -1;
>       if (virCondInit(&mon->notify) < 0) {
>           virReportSystemError(errno, "%s",
> @@ -907,6 +910,12 @@ qemuAgentGuestSync(qemuAgentPtr mon)
>       int send_ret;
>       unsigned long long id;
>       qemuAgentMessage sync_msg;
> +    int timeout = VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT;
> +
> +    /* if user specified a custom agent timeout that is lower than the
> +     * default timeout, use the shorter timeout instead */
> +    if ((mon->timeout >= 0) && (mon->timeout < timeout))
> +        timeout = mon->timeout;
>   
>       memset(&sync_msg, 0, sizeof(sync_msg));
>       /* set only on first sync */
> @@ -927,8 +936,7 @@ qemuAgentGuestSync(qemuAgentPtr mon)
>   
>       VIR_DEBUG("Sending guest-sync command with ID: %llu", id);
>   
> -    send_ret = qemuAgentSend(mon, &sync_msg,
> -                             VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT);
> +    send_ret = qemuAgentSend(mon, &sync_msg, timeout);
>   
>       VIR_DEBUG("qemuAgentSend returned: %d", send_ret);
>   
> @@ -1304,8 +1312,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
>       if (!cmd)
>           goto cleanup;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>           goto cleanup;
>   
>       if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
> @@ -1342,8 +1349,7 @@ int qemuAgentFSThaw(qemuAgentPtr mon)
>       if (!cmd)
>           return -1;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>           goto cleanup;
>   
>       if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
> @@ -1380,8 +1386,7 @@ qemuAgentSuspend(qemuAgentPtr mon,
>           return -1;
>   
>       mon->await_event = QEMU_AGENT_EVENT_SUSPEND;
> -    ret = qemuAgentCommand(mon, cmd, &reply, false,
> -                           VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
> +    ret = qemuAgentCommand(mon, cmd, &reply, false, mon->timeout);
>   
>       virJSONValueFree(cmd);
>       virJSONValueFree(reply);
> @@ -1437,8 +1442,7 @@ qemuAgentFSTrim(qemuAgentPtr mon,
>       if (!cmd)
>           return ret;
>   
> -    ret = qemuAgentCommand(mon, cmd, &reply, false,
> -                           VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
> +    ret = qemuAgentCommand(mon, cmd, &reply, false, mon->timeout);
>   
>       virJSONValueFree(cmd);
>       virJSONValueFree(reply);
> @@ -1459,8 +1463,7 @@ qemuAgentGetVCPUs(qemuAgentPtr mon,
>       if (!(cmd = qemuAgentMakeCommand("guest-get-vcpus", NULL)))
>           return -1;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>           goto cleanup;
>   
>       if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
> @@ -1575,8 +1578,7 @@ qemuAgentSetVCPUsCommand(qemuAgentPtr mon,
>                                        NULL)))
>           goto cleanup;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>           goto cleanup;
>   
>       /* All negative values are invalid. Return of 0 is bogus since we wouldn't
> @@ -1731,8 +1733,7 @@ qemuAgentGetHostname(qemuAgentPtr mon,
>       if (!cmd)
>           return ret;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
>           if (qemuAgentErrorCommandUnsupported(reply))
>               ret = -2;
>           goto cleanup;
> @@ -1776,8 +1777,7 @@ qemuAgentGetTime(qemuAgentPtr mon,
>       if (!cmd)
>           return ret;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>           goto cleanup;
>   
>       if (virJSONValueObjectGetNumberUlong(reply, "return", &json_time) < 0) {
> @@ -1842,8 +1842,7 @@ qemuAgentSetTime(qemuAgentPtr mon,
>       if (!cmd)
>           return ret;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>           goto cleanup;
>   
>       ret = 0;
> @@ -2046,8 +2045,7 @@ qemuAgentGetFSInfoInternal(qemuAgentPtr mon,
>       if (!cmd)
>           return ret;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
>           if (qemuAgentErrorCommandUnsupported(reply))
>               ret = -2;
>           goto cleanup;
> @@ -2336,8 +2334,7 @@ qemuAgentGetInterfaces(qemuAgentPtr mon,
>       if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL)))
>           goto cleanup;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>           goto cleanup;
>   
>       if (!(ret_array = virJSONValueObjectGet(reply, "return"))) {
> @@ -2514,8 +2511,7 @@ qemuAgentSetUserPassword(qemuAgentPtr mon,
>                                        NULL)))
>           goto cleanup;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
>           goto cleanup;
>   
>       ret = 0;
> @@ -2546,8 +2542,7 @@ qemuAgentGetUsers(qemuAgentPtr mon,
>       if (!(cmd = qemuAgentMakeCommand("guest-get-users", NULL)))
>           return -1;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
>           if (qemuAgentErrorCommandUnsupported(reply))
>               return -2;
>           return -1;
> @@ -2636,8 +2631,7 @@ qemuAgentGetOSInfo(qemuAgentPtr mon,
>       if (!(cmd = qemuAgentMakeCommand("guest-get-osinfo", NULL)))
>           return -1;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
>           if (qemuAgentErrorCommandUnsupported(reply))
>               return -2;
>           return -1;
> @@ -2692,8 +2686,7 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
>       if (!(cmd = qemuAgentMakeCommand("guest-get-timezone", NULL)))
>           return -1;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
>           if (qemuAgentErrorCommandUnsupported(reply))
>               return -2;
>           return -1;
> @@ -2722,3 +2715,30 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
>   
>       return 0;
>   }
> +
> +/* qemuAgentSetResponseTimeout:
> + * mon: agent monitor
> + * timeout: number of seconds to wait for agent response
> + * flags: currently unused. Must pass 0
> + *
> + * the agent object must be locked prior to calling this function
> + *
> + * Returns: 0 on success
> + *          -1 otherwise
> + */
> +int
> +qemuAgentSetResponseTimeout(qemuAgentPtr mon,
> +                            int timeout,
> +                            unsigned int flags G_GNUC_UNUSED)

@flags can be dropped. They should be checked for in qemu driver. If we 
ever need to populate them here, we can modify this function to have the 
argument.

> +{
> +    if (timeout < VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("guest agent timeout '%d' is "
> +                         "less than the minimum '%d'"),
> +                       timeout, VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN);
> +        return -1;
> +    }
> +
> +    mon->timeout = timeout;
> +    return 0;
> +}
> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
> index 78e648992a..88ead4762a 100644
> --- a/src/qemu/qemu_agent.h
> +++ b/src/qemu/qemu_agent.h
> @@ -140,3 +140,7 @@ int qemuAgentGetTimezone(qemuAgentPtr mon,
>                            virTypedParameterPtr *params,
>                            int *nparams,
>                            int *maxparams);
> +
> +int qemuAgentSetResponseTimeout(qemuAgentPtr mon,
> +                                int timeout,
> +                                unsigned int flags);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 667cc89072..40d0401e79 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -15564,3 +15564,18 @@ qemuDomainSupportsCheckpointsBlockjobs(virDomainObjPtr vm)
>   
>       return 0;
>   }
> +
> +int qemuDomainObjSetAgentResponseTimeout(virDomainObjPtr vm,
> +                                         int timeout,
> +                                         unsigned int flags)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    qemuAgentPtr agent = priv->agent;
> +    int ret;
> +
> +    virObjectLock(agent);
> +    ret = qemuAgentSetResponseTimeout(agent, timeout, flags);
> +    virObjectUnlock(agent);
> +
> +    return ret;

Nope, priv->agent can be NULL.

This function is needless, you can call qemuAgentSetResponseTimeout() 
right from qemu_driver.c.

> +}
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index b23912ee98..34af55ef45 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -1225,3 +1225,8 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
>   int
>   qemuDomainSupportsCheckpointsBlockjobs(virDomainObjPtr vm)
>       G_GNUC_WARN_UNUSED_RESULT;
> +
> +int
> +qemuDomainObjSetAgentResponseTimeout(virDomainObjPtr vm,
> +                                     int seconds,
> +                                     unsigned int flags);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d17c18705b..68f7b65d81 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -22654,6 +22654,27 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
>       return ret;
>   }
>   
> +static int
> +qemuDomainAgentSetResponseTimeout(virDomainPtr dom,
> +                                  int timeout,
> +                                  unsigned int flags)
> +{
> +    virDomainObjPtr vm = NULL;
> +    int ret = -1;
> +
> +    if (!(vm = qemuDomainObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainAgentSetResponseTimeoutEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    ret = qemuDomainObjSetAgentResponseTimeout(vm, timeout, flags);

1: It's not that simple. Firstly, you need to grab an agent job 
(QEMU_AGENT_JOB_MODIFY ideally) to make sure no other thread is in the 
middle of talking to the agent. Secondly, some domains may not have an 
agent configured at all (in which case, priv->agent is going to be 
NULL). And lastly, we need to track the timeout set in domain private 
data so that if libvirtd restarts the same timeout is preserved. But 
changing private data means you have to acquire domain job too. In the 
end, I think we want something like this:


+    /* If domain has an agent, change it's timeout too. Otherwise
+     * just note down the request so that if agent appears it
+     * gets the requested timeout from the beginning. */
+    if (qemuDomainAgentAvailable(vm, false))
+        agentJob = QEMU_AGENT_JOB_MODIFY;
+
+    if (qemuDomainObjBeginJobWithAgent(driver, vm,
+                                       QEMU_JOB_MODIFY,
+                                       agentJob) < 0)
+        goto cleanup;
+
+    if (agentJob != QEMU_AGENT_JOB_NONE) {
+        qemuAgentPtr agent;
+        int rc;
+
+        if (virDomainObjCheckActive(vm) < 0)
+            goto endjob;
+
+        agent = qemuDomainObjEnterAgent(vm);
+        rc = qemuAgentSetResponseTimeout(agent, timeout);
+        qemuDomainObjExitAgent(vm, agent);
+
+        if (rc < 0)
+            goto endjob;
+    }
+
+    QEMU_DOMAIN_PRIVATE(vm)->agentTimeout = timeout;
+    ret = 0;
+
+ endjob:
+    if (agentJob != QEMU_AGENT_JOB_NONE)
+        qemuDomainObjEndJobWithAgent(driver, vm);
+    else
+        qemuDomainObjEndJob(driver, vm);


I've created a fixup commit and pushed it to my github. Feel free to 
incorporate it into v3 (you will need to implement saving/restoring of 
priv->agentTimeout to/from status XML - see 
qemuDomainObjPrivateXMLFormat() and qemuDomainObjPrivateXMLParse()):

https://github.com/zippy2/libvirt/commit/b0f38070e09366d9a09428b19ecafee3f1aab2b4

The rest looks okay. But it would be also nice if you implement virsh 
command and expose this API through it. In my patch I've named it 
"qemu-agent-timeout" but that is probably wrong since the API name is 
generic and not tied to qemu.

Michal




More information about the libvir-list mailing list