[libvirt] [PATCHv3 5/5] virsh: Implement command to excercise the bulk stats APIs

[Eric Blake]

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

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