On 08/27/2014 12:25 PM, Peter Krempa wrote:
> Add "domstats" command that excercises both of the new APIs depending if
s/excercises/exercises/
> you specify a domain list or not. The output is printed as a key=value
> list of the returned parameters.
> ---
> tools/virsh-domain-monitor.c | 191 +++++++++++++++++++++++++++++++++++++++++++
> tools/virsh.pod | 34 ++++++++
> 2 files changed, 225 insertions(+)
>
> +
> +static bool
> +vshDomainStatsPrintRecord(vshControl *ctl ATTRIBUTE_UNUSED,
> + virDomainStatsRecordPtr record,
> + bool raw ATTRIBUTE_UNUSED)
> +{
> + char *param;
> + size_t i;
> +
> + vshPrint(ctl, "Domain: '%s'\n", virDomainGetName(record->dom));
> +
> + /* XXX: Implement pretty-printing */
Yeah, that can come in a later patch; this is already big enough to be a
useful first round.
> +
> + VSH_EXCLUSIVE_OPTIONS("domains", "list-active");
> + VSH_EXCLUSIVE_OPTIONS("domains", "list-inactive");
> + VSH_EXCLUSIVE_OPTIONS("domains", "list-persistent");
> + VSH_EXCLUSIVE_OPTIONS("domains", "list-transient");
> + VSH_EXCLUSIVE_OPTIONS("domains", "list-running");
> + VSH_EXCLUSIVE_OPTIONS("domains", "list-paused");
> + VSH_EXCLUSIVE_OPTIONS("domains", "list-shutoff");
> + VSH_EXCLUSIVE_OPTIONS("domains", "list-other");
Drop this hunk, and then you can validate that my tweak to 4/5 actually
rejects combining things at the lower layer. Also, it is more
future-proof - if we later change our minds, and DO allow filtering over
a list of domain names, then we don't have to revisit the virsh command
to get things to work. [1]...
> +=item B<domstats> [[I<--list-active>] [I<--list-inactive>]
> +[I<--list-persistent>] [I<--list-transient>] [I<--list-running>]
> +[I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domains>]
I'd list this as [I<domains>...], to make it obvious that more than one
can be supported. Also, in most places where we have an ARGV option, we
tend to name it in the singular: echo --arg, snapshot-create-as
--diskspec, domfsfreeze --mountpoint. That has a ripple effect on the
rest of your patch s/domains/domain/
> +[I<--raw>] [I<--enforce>] [I<--state>]
> +
> +Get statistics for multiple or all domains. Without any argument this
> +command prints all available statistics for all domains.
> +
> +The list of domains to gather stats for can be either limited by listing
> +the domains as a space separated list, or by specifying one of the
> +filtering flags I<--list-*>. (The approaches can't be combined.)
...[1] But leave this comment in - we are just moving the enforcing to a
lower layer of the stack, and letting virsh expose more power so that we
have fuller testing over the lower layer behavior.
> +
> + $ tools/virsh domstats --state dom1 dom2
Drop 'tools/' from the example (even if it is what you pasted from your
terminal session testing it).
> + Domain: 'dom1'
> + state.state=shut off
> + state.reason=unknown
> +
> + Domain: 'dom2'
> + state.state=running
> + state.reason=booted
Doesn't match the code (yet), so how about we drop this example until we
have the followup patch for the pretty-printers.
ACK with the tweaks above.
--
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