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

Re: [libvirt] [PATCH 3/5] add virDomainQemuAgentCommand() to qemu driver and remote driver



On 08/07/2012 06:05 PM, MATSUDA, Daiki wrote:
>     Add virDomainQemuAgentCommand() to qemu driver and remote driver
>     to support qemuAgentCommand().
> 
>  daemon/remote.c                |   35 ++++++++++++++++++++
>  include/libvirt/libvirt-qemu.h |    3 +
>  src/driver.h                   |    6 +++
>  src/libvirt-qemu.c             |   58 +++++++++++++++++++++++++++++++++
>  src/libvirt_qemu.syms          |    5 ++
>  src/qemu/qemu_driver.c         |   71 +++++++++++++++++++++++++++++++++++++++++
>  src/qemu_protocol-structs      |   10 +++++
>  src/remote/qemu_protocol.x     |   14 +++++++-
>  src/remote/remote_driver.c     |   41 +++++++++++++++++++++++
>  9 files changed, 242 insertions(+), 1 deletion(-)

You are mixing the addition of new API (include/, src/driver.h,
src/libvirt-qemu.{c,syms}) with two implementations of two drivers
(daemon and src/remote for the RPC, and src/qemu for the qemu
implementation).  If this were an API in libvirt.h, I'd split this to
three patches, but since it is for libvirt-qemu.h, I'm 50-50 whether the
split makes sense.

Compilation failure; you need to rebase to latest libvirt.git:

  CC     libvirt_driver_qemu_impl_la-qemu_driver.lo
qemu/qemu_driver.c: In function 'qemuDomainQemuAgentCommand':
qemu/qemu_driver.c:13228:9: error: implicit declaration of function
'qemuReportError' [-Werror=implicit-function-declaration]
qemu/qemu_driver.c:13228:9: error: nested extern declaration of
'qemuReportError' [-Werror=nested-externs]

> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 832307e..4fd5c9b 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -3948,6 +3948,41 @@ cleanup:
>      return rv;
>  }
> 
> +static int
> +qemuDispatchAgentCommand(virNetServerPtr server ATTRIBUTE_UNUSED,
> +                              virNetServerClientPtr client ATTRIBUTE_UNUSED,
> +                              virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +                              virNetMessageErrorPtr rerr,
> +                              qemu_agent_command_args *args,
> +                              qemu_agent_command_ret *ret)

Indentation is off.

This function looks pretty simple; any reason we can't autogenerate it
from the .x file?

> @@ -1048,6 +1053,7 @@ struct _virDriver {
>      virDrvDomainGetDiskErrors           domainGetDiskErrors;
>      virDrvDomainSetMetadata             domainSetMetadata;
>      virDrvDomainGetMetadata             domainGetMetadata;
> +    virDrvDomainQemuAgentCommand        qemuDomainQemuAgentCommand;

I'd group this line next to the other virDomainQemu callback.

> +++ b/src/libvirt-qemu.c
> @@ -185,3 +185,61 @@ error:
>      virDispatchError(conn);
>      return NULL;
>  }
> +
> +/**
> + * virDomainQemuAgentCommand:
> + * @domain: a domain object
> + * @cmd: the guest agent command string
> + * @result: a string returned by @cmd
> + * @timeout: timeout seconds
> + * @flags: execution flags
> + *
> + * Execution Guest Agent Command

Execute an arbitrary Guest Agent command.

> + *
> + * Issue @cmd to the guest agent running in @domain.
> + * If @result is NULL, then don't wait for a result (and @timeout
> + * must be 0).  Otherwise, wait for @timeout seconds for a
> + * successful result to be populated into * result, with
> + * VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK (-1) meaning to block
> + * forever waiting for a result.
> + *
> + * Returns 0 if succeeded, -1 in failing.

s/if succeeded/on success/; s/in failing/on failure/

> + */
> +int
> +virDomainQemuAgentCommand(virDomainPtr domain,
> +                          const char *cmd,
> +                          char **result,
> +                          int timeout,
> +                          unsigned int flags)
> +{
> +    virConnectPtr conn;

> +    conn = domain->conn;
> +
> +    virCheckNonNullArgGoto(result, error);

Wrong - we allow a NULL result argument, to issue a command with no
expected response.

> @@ -13369,6 +13439,7 @@ static virDriver qemuDriver = {
>      .domainPMSuspendForDuration = qemuDomainPMSuspendForDuration, /* 0.9.11 */
>      .domainPMWakeup = qemuDomainPMWakeup, /* 0.9.11 */
>      .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */
> +    .qemuDomainQemuAgentCommand = qemuDomainQemuAgentCommand, /* 0.10.0 */

Again, sticking this next to the other qemuDomainQemu callback would be
nicer.

> +++ b/src/remote/qemu_protocol.x
> @@ -47,6 +47,17 @@ struct qemu_domain_attach_ret {
>      remote_nonnull_domain dom;
>  };
> 
> +struct qemu_agent_command_args {
> +    remote_nonnull_domain dom;
> +    remote_nonnull_string cmd;
> +    int timeout;
> +    unsigned int flags;

Missing a field to pass to the other end of the connection whether the
result pointer was given as NULL.

> +};
> +
> +struct qemu_agent_command_ret {
> +    remote_nonnull_string result;

If the result pointer was NULL, then there is no result to return; I
think this has to be remote_string.

> +};
> +
>  /* Define the program number, protocol version and procedure numbers here. */
>  const QEMU_PROGRAM = 0x20008087;
>  const QEMU_PROTOCOL_VERSION = 1;
> @@ -61,5 +72,6 @@ enum qemu_procedure {
>       * are some exceptions to this rule, e.g. domainDestroy. Other APIs MAY
>       * be marked as high priority. If in doubt, it's safe to choose low. */
>      QEMU_PROC_MONITOR_COMMAND = 1, /* skipgen skipgen priority:low */
> -    QEMU_PROC_DOMAIN_ATTACH = 2 /* autogen autogen priority:low */
> +    QEMU_PROC_DOMAIN_ATTACH = 2, /* autogen autogen priority:low */
> +    QEMU_PROC_AGENT_COMMAND = 3 /* skipgen skipgen priority:low */

Again, can you use autogen instead of skipgen?

> +++ b/src/remote/remote_driver.c
> @@ -5191,6 +5191,46 @@ make_nonnull_domain_snapshot (remote_nonnull_domain_snapshot *snapshot_dst, virD
>      make_nonnull_domain(&snapshot_dst->dom, snapshot_src->domain);
>  }
> 
> +static int
> +remoteQemuDomainQemuAgentCommand (virDomainPtr domain, const char *cmd,
> +                                  char **result, int timeout,
> +                                  unsigned int flags)
> +{
> +    int rv = -1;
> +    qemu_agent_command_args args;
> +    qemu_agent_command_ret ret;
> +    struct private_data *priv = domain->conn->privateData;
> +
> +    remoteDriverLock(priv);
> +
> +    make_nonnull_domain(&args.dom, domain);
> +    args.cmd = (char *)cmd;
> +    args.timeout = timeout;
> +    args.flags = flags;
> +
> +    memset (&ret, 0, sizeof(ret));
> +
> +    if (call (domain->conn, priv, REMOTE_CALL_QEMU, QEMU_PROC_AGENT_COMMAND,
> +              (xdrproc_t) xdr_qemu_agent_command_args, (char *) &args,
> +              (xdrproc_t) xdr_qemu_agent_command_ret, (char *) &ret) == -1)
> +        goto done;
> +
> +    *result = strdup(ret.result);

You need to account for the case when result is NULL (maybe you can't
autogen after all, if the generator can't handle an optionally NULL
argument).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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