Re: [PATCH RFC 3/4] libxl: implement qemu-agent-command

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux