[libvirt] [PATCH] Add error handling to optional arguments in cmdCPUStats

[Ján Tomko]

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