[libvirt] [PATCH 5/5] Add qemu-agent-command command to virsh

[Eric Blake]

On 08/07/2012 06:05 PM, MATSUDA, Daiki wrote:
>     Add qemu-agent-command command to virsh to support virDomainQemuAgentCommand().
> 
>  virsh-host.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)

Missing documentation in virsh.pod.

> 
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index d9d09b4..b9180f3 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c

Odd (I would have expected virsh-domain.c, since this is a command
related to an online domain), but pre-existing (qemu-monitor-command is
also here, so your patch did the right thing in being in the same location).

> @@ -633,6 +633,74 @@ cleanup:
>  }
> 
>  /*
> + * "qemu-agent-command" command
> + */
> +static const vshCmdInfo info_qemu_agent_command[] = {
> +    {"help", N_("Qemu Guest Agent Command")},

s/Qemu/QEMU/ or the qemu folks will be on our case for misspelling it
(besides, you should match the style of qemu-monitor-command).

> +    {"desc", N_("Qemu Guest Agent Command")},

Here, you are copying from a poor example (so fix qemu-monitor-command
in the meantime); I'd suggest:

Run an arbitrary qemu guest agent command; use at your own risk

and yes, I really do suggest the 'at your own risk' phrase, since we
provide this strictly as a debugging aid rather than a supported API.
Which means qemu-monitor-command desc should list:

Run an arbitrary qemu monitor command; use at your own risk

> +    {NULL, NULL}
> +};
> +
> +static const vshCmdOptDef opts_qemu_agent_command[] = {
> +    {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> +    {"timeout", VSH_OT_INT, VSH_OFLAG_REQ_OPT, N_("timeout")},
> +    {"cmd", VSH_OT_ARGV, VSH_OFLAG_REQ, N_("command")},

When the user does not specify --timeout, do we normally want to provide
a decent default timeout, or do we want to issue a non-blocking call
where we don't wait for an answer?  That almost argues that we need yet
another bool option that lets the user choose to be --async (no waiting)
vs omitted (block for an answer, and missing --timeout implies a sane
default, such as 5 seconds, rather than 0).

> +    {NULL, 0, 0, NULL}
> +};
> +
> +static bool
> +cmdQemuAgentCommand(vshControl *ctl, const vshCmd *cmd)
> +{
> +    virDomainPtr dom = NULL;
> +    bool ret = false;
> +    char *guest_agent_cmd = NULL;
> +    char *result = NULL;
> +    int timeout = 0;
> +    unsigned int flags = 0;
> +    const vshCmdOpt *opt = NULL;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    bool pad = false;
> +
> +    if (!vshConnectionUsability(ctl, ctl->conn))
> +        goto cleanup;
> +
> +    dom = vshCommandOptDomain(ctl, cmd, NULL);
> +    if (dom == NULL)
> +        goto cleanup;
> +
> +    while ((opt = vshCommandOptArgv(cmd, opt))) {
> +        if (pad)
> +            virBufferAddChar(&buf, ' ');
> +        pad = true;
> +        virBufferAdd(&buf, opt->data, -1);
> +    }

This mess with pad comes because qemu-monitor-command was written before
virBufferTrim.  I'd simplify this to:

while ((opt = vshCommandOptArgv(cmd, opt)))
    virBufferAsprintf(&buf, "%s ", opt->data)
virBufferTrim(&buf, " ", 1);

> +    if (virBufferError(&buf)) {
> +        vshPrint(ctl, "%s", _("Failed to collect command"));
> +        goto cleanup;
> +    }
> +    guest_agent_cmd = virBufferContentAndReset(&buf);
> +
> +    if (vshCommandOptInt(cmd, "timeout", &timeout) < 0) {
> +        timeout = 0;

Wrong. If vshCommandOptInt is < 0, you need to fail, because the user
had a syntax error on the command line.  If it is equal to 0, then
timeout is already 0 (because you initialized it earlier); but I was
arguing that you should have pre-initialized to 5 instead of 0.

> +    }
> +
> +    if (virDomainQemuAgentCommand(dom, guest_agent_cmd, &result,
> +                                  timeout, flags) < 0)

Again, you need to handle the case where the user doesn't want to wait
for output, so you need to be able to pass in NULL instead of &result.

> @@ -832,6 +900,8 @@ static const vshCmdDef hostAndHypervisorCmds[] = {
>      {"qemu-attach", cmdQemuAttach, opts_qemu_attach, info_qemu_attach, 0},
>      {"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command,
>       info_qemu_monitor_command, 0},
> +    {"qemu-agent-command", cmdQemuAgentCommand, opts_qemu_agent_command,
> +     info_qemu_agent_command, 0},

Sorting is off.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120809/af626b6e/attachment-0001.sig>