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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list