Re: [PATCH v2 2/2] Add error handling to optional arguments in cmdCPUStats

[Ján Tomko <jtomko@xxxxxxxxxx>]

On 04/08/2013 05:35 PM, John Ferlan wrote:
> ---
>  tools/virsh-domain.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index e6e6877..6d760f2 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6144,15 +6144,35 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd)
>      bool show_total = false, show_per_cpu = false;
>      unsigned int flags = 0;
>      bool ret = false;
> +    int rv = 0;
>  
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return false;
>  
>      show_total = vshCommandOptBool(cmd, "total");
> -    if (vshCommandOptInt(cmd, "start", &cpu) > 0)
> +
> +    if ((rv = vshCommandOptInt(cmd, "start", &cpu)) < 0) {
> +        vshError(ctl, "%s", _("Unable to parse integer parameter for start"));
> +        goto cleanup;
> +    } else if (rv > 0) {
> +        if (cpu < 0) {
> +            vshError(ctl, "%s", _("Invalid value for start CPU"));
> +            goto cleanup;
> +        }

Since we don't allow negative -start values now, you can initialize cpu
to 0 instead of -1 and get rid of this hunk:

     /* check cpu, show_count, and ignore wrong argument */
     if (cpu < 0)
         cpu = 0;

>          show_per_cpu = true;
> -    if (vshCommandOptInt(cmd, "count", &show_count) > 0)
> +    }
> +
> +    if ((rv = vshCommandOptInt(cmd, "count", &show_count)) < 0) {
> +        vshError(ctl, "%s",
> +                 _("Unable to parse integer parameter for CPUs to show"));
> +        goto cleanup;
> +    } else if (rv > 0) {
> +        if (show_count < 0) {
> +            vshError(ctl, "%s", _("Invalid value for number of CPUs to show"));
> +            goto cleanup;
> +        }
>          show_per_cpu = true;
> +    }
>  
>      /* default show per_cpu and total */
>      if (!show_total && !show_per_cpu) {
> @@ -6170,8 +6190,11 @@ 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) {
> +        if (show_count > max_id)
> +            vshPrint(ctl, _("Only %d CPUs available to show\n"), max_id);
>          show_count = max_id;
> +    }
>  
>      /* get percpu information */
>      if ((nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, flags)) < 0)
> 

ACK

Jan

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list