[libvirt] [PATCH] Add error handling to optional arguments in cmdCPUStats
Ján Tomko
jtomko at redhat.com
Mon Apr 8 13:39:12 UTC 2013
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
More information about the libvir-list
mailing list