On 04/05/2013 10:40 PM, John Ferlan wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=907732

Also added informational message when count value is larger than number of CPUs present. Original code commit '31047e2b' quietly changes it and continues on.

Prior to this patch, no errors were seen for following sequences

virsh cpu-stats guest xyz virsh cpu-stats guest --start xyz virsh cpu-stats guest --count xyz virsh cpu-stats guest --count 99999999999

With this patch, the following errors are displayed

error: Invalid value for start CPU error: Invalid value for start CPU error: Invalid value for number of CPUs to show error: Invalid value for number of CPUs to show

Passing a value such as 9 to count will display the following:

Only 4 CPUs available to show CPU0: cpu_time 19.860859202 seconds vcpu_time 17.551435620 seconds ... --- tools/virsh-domain.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c088468..3dbfa53 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6111,7 +6111,7 @@ static const vshCmdInfo info_cpu_stats[] = { {.name = "desc", .data = N_("Display per-CPU and total statistics about the domain's CPUs") }, - {.name = NULL}, + {.name = NULL} };

static const vshCmdOptDef opts_cpu_stats[] = { @@ -6132,7 +6132,7 @@ static const vshCmdOptDef opts_cpu_stats[] = { .type = VSH_OT_INT, .help = N_("Number of shown CPUs at most") }, - {.name = NULL}, + {.name = NULL} };

I think these two hunks would be better in a separate patch.

static bool @@ -6149,9 +6149,18 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) return false;

show_total = vshCommandOptBool(cmd, "total"); - if (vshCommandOptInt(cmd, "start", &cpu) > 0) + if (vshCommandOptInt(cmd, "start", &cpu) < 0) { + vshError(ctl, "%s", _("Invalid value for start CPU")); + goto cleanup; + } + if (cpu >= 0) show_per_cpu = true; - if (vshCommandOptInt(cmd, "count", &show_count) > 0) + + if (vshCommandOptInt(cmd, "count", &show_count) < 0) { + vshError(ctl, "%s", _("Invalid value for number of CPUs to show")); + goto cleanup; + } + if (show_count >= 0) show_per_cpu = true;

/* default show per_cpu and total */

This doesn't set show_per_cpu when a negative number was specified, shouldn't we set it based on vshCommandOptInt return value instead?

@@ -6170,8 +6179,10 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) /* get number of cpus on the node */ if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, flags)) < 0) goto failed_stats; - if (show_count < 0 || show_count > max_id) + if (show_count < 0 || show_count > max_id) { + vshPrint(ctl, _("Only %d CPUs available to show\n"), max_id);

This message will get printed even if --count wasn't specified, since show_count is -1 by default.

show_count = max_id; + }

/* get percpu information */ if ((nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, flags)) < 0)

Jan