6:06 a.m.
On 03/02/2017 06:46 PM, Jim Fehlig wrote:
On 02/08/2017 09:44 AM, Joao Martins wrote:
> Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
> ---
Sorry - being an RFC I let myself be a little too clumsy on the commit
messages.
This was meant to have at least the bits mentioned in the cover letter.
> src/libxl/libxl_domain.c | 239
++++++++++++++++++++++++++++++++++++++++++++++-
> src/libxl/libxl_domain.h | 16 ++++
> src/libxl/libxl_driver.c | 51 ++++++++++
> 3 files changed, 305 insertions(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index ed73cd2..6bdd0ec 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -782,6 +782,12 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
> }
> }
>
> + if (priv->agent) {
> + qemuAgentClose(priv->agent);
> + priv->agent = NULL;
> + priv->agentError = false;
> + }
> +
> if ((vm->def->nnets)) {
> size_t i;
>
> @@ -940,6 +946,228 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config
*d_config)
> return -1;
> }
>
> +/*
> + * This is a callback registered with a qemuAgentPtr instance,
> + * and to be invoked when the agent console hits an end of file
> + * condition, or error, thus indicating VM shutdown should be
> + * performed
> + */
> +static void
> +libxlHandleAgentEOF(qemuAgentPtr agent,
> + virDomainObjPtr vm)
Whitespace is off.
/nods
> +{
> + libxlDomainObjPrivatePtr priv;
> +
> + VIR_DEBUG("Received EOF from agent on %p '%s'", vm,
vm->def->name);
> +
> + virObjectLock(vm);
> +
> + priv = vm->privateData;
> +
> + if (!priv->agent) {
> + VIR_DEBUG("Agent freed already");
> + goto unlock;
> + }
> +
> + qemuAgentClose(agent);
> + priv->agent = NULL;
Should priv->agentError be reset to false?
Yeap.
> +
> + unlock:
> + virObjectUnlock(vm);
> + return;
> +}
> +
> +
> +/*
> + * This is invoked when there is some kind of error
> + * parsing data to/from the agent. The VM can continue
> + * to run, but no further agent commands will be
> + * allowed
> + */
> +static void
> +libxlHandleAgentError(qemuAgentPtr agent ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm)
Whitespace is off here too, and in a few other places below that I'll stop
nagging about.
Will fix those.
> +{
> + libxlDomainObjPrivatePtr priv;
> +
> + VIR_DEBUG("Received error from agent on %p '%s'", vm,
vm->def->name);
> +
> + virObjectLock(vm);
> +
> + priv = vm->privateData;
> +
> + priv->agentError = true;
> +
> + virObjectUnlock(vm);
> +}
> +
> +static void libxlHandleAgentDestroy(qemuAgentPtr agent,
> + virDomainObjPtr vm)
> +{
> + VIR_DEBUG("Received destroy agent=%p vm=%p", agent, vm);
> +
> + virObjectUnref(vm);
> +}
> +
> +static qemuAgentCallbacks agentCallbacks = {
> + .destroy = libxlHandleAgentDestroy,
> + .eofNotify = libxlHandleAgentEOF,
> + .errorNotify = libxlHandleAgentError,
> +};
> +
> +static virDomainChrDefPtr
> +libxlFindAgentConfig(virDomainDefPtr def)
> +{
> + size_t i;
> +
> + for (i = 0; i < def->nchannels; i++) {
> + virDomainChrDefPtr channel = def->channels[i];
> +
> + if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN)
> + continue;
> +
> + if (STREQ_NULLABLE(channel->target.name,
"org.qemu.guest_agent.0"))
> + return channel;
> + }
> +
> + return NULL;
> +}
> +
> +bool
> +libxlDomainAgentAvailable(virDomainObjPtr vm, bool reportError)
Is the reportError parameter needed? The callers in this patch and 4/4 pass
'true'.
Probably that should be removed. Being always true it is redundant.
> +{
> + libxlDomainObjPrivatePtr priv = vm->privateData;
> +
> + if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
> + if (reportError) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("domain is not running"));
> + }
> + return false;
> + }
> + if (priv->agentError) {
> + if (reportError) {
> + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
> + _("QEMU guest agent is not "
> + "available due to an error"));
> + }
> + return false;
> + }
> + if (!priv->agent) {
> + if (libxlFindAgentConfig(vm->def)) {
> + if (reportError) {
> + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
> + _("QEMU guest agent is not connected"));
> + }
> + return false;
> + } else {
> + if (reportError) {
> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("QEMU guest agent is not configured"));
> + }
> + return false;
> + }
> + }
> + return true;
> +}
> +
> +static int
> +libxlConnectAgent(virDomainObjPtr vm)
> +{
> + virDomainChrDefPtr config = libxlFindAgentConfig(vm->def);
> + libxlDomainObjPrivatePtr priv = vm->privateData;
> + qemuAgentPtr agent = NULL;
> + int ret = -1;
> +
> + if (!config)
> + return 0;
> +
> + if (priv->agent)
> + return 0;
> +
> + /* Hold an extra reference because we can't allow 'vm' to be
> + * deleted while the agent is active */
> + virObjectRef(vm);
> +
> + ignore_value(virTimeMillisNow(&priv->agentStart));
> + virObjectUnlock(vm);
> +
> + agent = qemuAgentOpen(vm, config->source, &agentCallbacks);
> +
> + virObjectLock(vm);
> + priv->agentStart = 0;
> +
> + if (agent == NULL)
> + virObjectUnref(vm);
> +
> + if (!virDomainObjIsActive(vm)) {
> + qemuAgentClose(agent);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest crashed while connecting to the guest
agent"));
> + ret = -2;
This can be dropped, right? ret is initialized to -1 and the caller only checks
for ret < 0.
Yeap.
> + goto cleanup;
> + }
> +
> + priv->agent = agent;
> +
> + if (priv->agent == NULL) {
> + VIR_INFO("Failed to connect agent for %s", vm->def->name);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + return ret;
Hmm, since there is nothing to cleanup might as well return -1 at failure points
and 0 on success.
/nods
These were all remnants of a common return point.
> +}
> +
> +/*
> + * obj must be locked before calling
> + *
> + * To be called immediately before any QEMU agent API call.
> + * Must have already called libxlDomainObjBeginJob() and checked
> + * that the VM is still active.
> + *
> + * To be followed with libxlDomainObjExitAgent() once complete
> + */
> +void
> +libxlDomainObjEnterAgent(virDomainObjPtr obj)
> +{
> + libxlDomainObjPrivatePtr priv = obj->privateData;
> +
> + VIR_DEBUG("Entering agent (agent=%p vm=%p name=%s)",
> + priv->agent, obj, obj->def->name);
> + virObjectLock(priv->agent);
> + virObjectRef(priv->agent);
> + ignore_value(virTimeMillisNow(&priv->agentStart));
> + virObjectUnlock(obj);
> +}
> +
> +
> +/* obj must NOT be locked before calling
> + *
> + * Should be paired with an earlier qemuDomainObjEnterAgent() call
> + */
> +void
> +libxlDomainObjExitAgent(virDomainObjPtr obj)
> +{
> + libxlDomainObjPrivatePtr priv = obj->privateData;
> + bool hasRefs;
> +
> + hasRefs = virObjectUnref(priv->agent);
> +
> + if (hasRefs)
> + virObjectUnlock(priv->agent);
> +
> + virObjectLock(obj);
> + VIR_DEBUG("Exited agent (agent=%p vm=%p name=%s)",
> + priv->agent, obj, obj->def->name);
> +
> + priv->agentStart = 0;
> + if (!hasRefs)
> + priv->agent = NULL;
> +}
> +
> static int
> libxlNetworkPrepareDevices(virDomainDefPtr def)
> {
> @@ -1312,8 +1540,17 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
> libxlDomainCreateIfaceNames(vm->def, &d_config);
>
> #ifdef LIBXL_HAVE_DEVICE_CHANNEL
> - if (vm->def->nchannels > 0)
> + if (vm->def->nchannels > 0) {
> libxlDomainCreateChannelPTY(vm->def, cfg->ctx);
> +
> + /* Failure to connect to agent shouldn't be fatal */
> + if (libxlConnectAgent(vm) < 0) {
> + VIR_WARN("Cannot connect to QEMU guest agent for %s",
> + vm->def->name);
> + virResetLastError();
> + priv->agentError = true;
> + }
> + }
> #endif
>
> if ((dom_xml = virDomainDefFormat(vm->def, cfg->caps, 0)) == NULL)
> diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> index 3a3890b..59f9f8d 100644
> --- a/src/libxl/libxl_domain.h
> +++ b/src/libxl/libxl_domain.h
> @@ -29,6 +29,7 @@
> # include "domain_conf.h"
> # include "libxl_conf.h"
> # include "virchrdev.h"
> +# include "virqemuagent.h"
>
> # define JOB_MASK(job) (1 << (job - 1))
> # define DEFAULT_JOB_MASK \
> @@ -62,6 +63,11 @@ typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr;
> struct _libxlDomainObjPrivate {
> virObjectLockable parent;
>
> + /* agent */
> + qemuAgentPtr agent;
> + bool agentError;
> + unsigned long long agentStart;
> +
> /* console */
> virChrdevsPtr devs;
> libxl_evgen_domain_death *deathW;
> @@ -91,6 +97,16 @@ void
> libxlDomainObjEndJob(libxlDriverPrivatePtr driver,
> virDomainObjPtr obj);
>
> +bool
> +libxlDomainAgentAvailable(virDomainObjPtr vm,
> + bool reportError);
> +
> +void
> +libxlDomainObjEnterAgent(virDomainObjPtr obj);
> +
> +void
> +libxlDomainObjExitAgent(virDomainObjPtr obj);
> +
> int
> libxlDomainJobUpdateTime(struct libxlDomainJobObj *job)
> ATTRIBUTE_RETURN_CHECK;
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 3a69720..cf5e702 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -57,6 +57,7 @@
> #include "virstring.h"
> #include "virsysinfo.h"
> #include "viraccessapicheck.h"
> +#include "viraccessapicheckqemu.h"
> #include "viratomic.h"
> #include "virhostdev.h"
> #include "network/bridge_driver.h"
> @@ -6415,6 +6416,55 @@ libxlConnectBaselineCPU(virConnectPtr conn,
> return cpu;
> }
>
> +static char *
> +libxlDomainQemuAgentCommand(virDomainPtr domain,
> + const char *cmd,
> + int timeout,
> + unsigned int flags)
> +{
> + libxlDriverPrivatePtr driver = domain->conn->privateData;
> + virDomainObjPtr vm;
> + int ret = -1;
> + char *result = NULL;
> + libxlDomainObjPrivatePtr priv;
> +
> + virCheckFlags(0, NULL);
> +
> + if (!(vm = libxlDomObjFromDomain(domain)))
> + goto cleanup;
> +
> + priv = vm->privateData;
> +
> + if (virDomainQemuAgentCommandEnsureACL(domain->conn, vm->def) < 0)
> + goto cleanup;
> +
> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto endjob;
> + }
> +
> + if (!libxlDomainAgentAvailable(vm, true))
> + goto endjob;
> +
> + libxlDomainObjEnterAgent(vm);
> + ret = qemuAgentArbitraryCommand(priv->agent, cmd, &result, timeout);
> + libxlDomainObjExitAgent(vm);
> + if (ret < 0)
> + VIR_FREE(result);
> +
> + endjob:
> + libxlDomainObjEndJob(driver, vm);
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return result;
> +}
> +
> +
> static virHypervisorDriver libxlHypervisorDriver = {
> .name = LIBXL_DRIVER_NAME,
> .connectOpen = libxlConnectOpen, /* 0.9.0 */
> @@ -6522,6 +6572,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
> .connectGetDomainCapabilities = libxlConnectGetDomainCapabilities, /* 2.0.0 */
> .connectCompareCPU = libxlConnectCompareCPU, /* 2.3.0 */
> .connectBaselineCPU = libxlConnectBaselineCPU, /* 2.3.0 */
> + .domainQemuAgentCommand = libxlDomainQemuAgentCommand, /* 3.1.0 */
Heh, let's hope for 3.2.0. Apologies for the delay.
/nods