On 08/26/2014 08:14 AM, Peter Krempa wrote:
> Add "domstats" command that excercises both of the new APIs depending if
> you specify a domain list or not. The output is printed as a key=value
> list of the returned parameters.
>
> Man page section will be added in a separate patch.
Trying to stretch those release deadlines as long as possible, aren't you ;)
> ---
> tools/virsh-domain-monitor.c | 140 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 140 insertions(+)
>
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index 8bd58ad..8eb3a21 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1954,6 +1954,140 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
> }
> #undef FILTER
>
> +/*
> + * "domstats" command
> + */
> +static const vshCmdInfo info_domstats[] = {
> + {.name = "help",
> + .data = N_("get statistics about one or multiple domains")
> + },
> + {.name = "desc",
> + .data = N_("Gets statistics about one or more (or all) domains")
> + },
> + {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_domstats[] = {
> + {.name = "stats",
> + .type = VSH_OT_INT,
> + .flags = VSH_OFLAG_REQ_OPT,
> + .help = N_("bit map of stats to retrieve"),
> + },
Ewww - you're seriously going to make the user pass in a raw integer?
I'd much rather you provide a series of VSH_OT_BOOL options, each of
which enables a bit in the stats bitmask: the user should be able to say
--state, rather than guessing that --stats=1 does what they want.
> + {.name = "allstats",
> + .type = VSH_OT_BOOL,
> + .help = N_("report all stats supported by the hypervisor"),
Is that necessary? Shouldn't that just be the default you get when
stats is 0? And if you do boolean flags for each possible category,
omitting all of the booleans leaves you with stats==0 to begin with.
I'd drop this one.
> + },
> + {.name = "state",
> + .type = VSH_OT_BOOL,
> + .help = N_("report domain state"),
> + },
Wait - so you have --stats=INT _and_ --state as a bool? That's too
confusing. I'd drop --stats.
> + {.name = "domains",
> + .type = VSH_OT_ARGV,
> + .flags = VSH_OFLAG_NONE,
> + .help = N_("list of domains to get stats for"),
> + },
Yay - this one works.
> +static bool
> +vshDomainStatsPrint(vshControl *ctl ATTRIBUTE_UNUSED,
> + virDomainStatsRecordPtr record)
> +{
> + char *param;
> + size_t i;
> +
> + vshPrint(ctl, "Domain: '%s'\n", virDomainGetName(record->dom));
> +
> + for (i = 0; i < record->nparams; i++) {
> + if (!(param = vshGetTypedParamValue(ctl, record->params + i)))
> + return false;
> +
> + vshPrint(ctl, " %s=%s\n", record->params[i].field, param);
Hmm - should we attempt to pretty-print things, such as
state.status=running instead of state.status=1? But to do that, we'd
have to special-case which record->paramsi].field names are worth
pretty-printing. It can be done as a followup patch.
In fact, I'm wondering if we should have a --raw (and/or a --pretty)
that forces the output to be raw int vs. pretty name at the user's
choice (probably pretty by default, raw by request), if it makes
machine-parsing of the output any easier. But now that I've typed that,
I'm not sure if it is easier - as long as we don't have any potential
for ambiguous output, a machine can parse 'state.status=running' just
about as easily as 'state.status=1'.
> + if (vshCommandOptUInt(cmd, "stats", &stats) < 0) {
> + vshError(ctl, "%s", _("Unable to parse stats bitmap"));
> + return false;
> + }
I don't think exposing this to the user as a raw int provides any
benefits. Drop it.
> +
> + if (vshCommandOptBool(cmd, "allstats"))
> + stats |= VIR_DOMAIN_STATS_ALL;
Probably don't need this one either, once stats==0 implies all stats.
> +
> + while ((opt = vshCommandOptArgv(cmd, opt))) {
> + if (!(dom = vshLookupDomainBy(ctl, opt->data,
> + VSH_BYID | VSH_BYUUID | VSH_BYNAME)))
> + goto cleanup;
> +
> + if (VIR_INSERT_ELEMENT(domlist, 0, ndoms, dom) < 0)
Why are you inserting at the front? If I call 'domstats a b', I'd
expect the stats for 'a' to be output first, not last.
> + cleanup:
> + if (records)
> + virDomainStatsRecordListFree(records);
Useless if, if you take my suggestion in patch 1.
--
Eric Blake eblake redhat com +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