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@xxxxxxxxxx>
>> ---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
