[libvirt] [PATCH] virsh: fix no error output when parse cpulist fail

[Ján Tomko]

On Wed, May 06, 2015 at 01:30:55PM +0800, Luyao Huang wrote:
> When we pass a invalid cpulist or the lastcpu in the
> cpulist exceed the maxcpu, we cannot get any error.
> like this:
> 
>  # virsh vcpupin test3 1 aaa
> 
>  # virsh vcpupin test3 1 1000
> 
> Because virBitmapParse() use virReportError() to set
> the error message, virsh client use vshError to output error.
> If we want get the error which set by virReportError(), we need
> virGetLastError/virGetLastErrorMessage to help us.

vshCommandRun would output the error in vshReportError, but in the
meantime it is overwriten by the virResetLastError in virDomainFree.

We have a vshSaveLibvirtError helper that can be used here to save
the error from virBitmap* APIs from being reset by virDomainFree,
if we decide to use it.

> However instead of do this, i chose use vshError to output
> error when parse failed.
> 
> Signed-off-by: Luyao Huang <lhuang at redhat.com>
> ---
>  tools/virsh-domain.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 14e1e35..64bfbfd 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6362,9 +6362,7 @@ vshParseCPUList(int *cpumaplen, const char *cpulist, int maxcpu)
>              return NULL;
>      }
>  
> -    if (virBitmapToData(map, &cpumap, cpumaplen) < 0)
> -        goto cleanup;
> -
> +    virBitmapToData(map, &cpumap, cpumaplen);

This change is unrelated to the rest of the patch and while it looks
nicer I am afraid that leaving the return value unchecked here would make
coverity complain.

I wrote it with the redundant 'goto cleanup', because I didn't want to
leave it unchecked and I don't like the ignore_value macro.

>   cleanup:
>      virBitmapFree(map);
>      return cpumap;
> @@ -6458,8 +6456,10 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>          }
>      } else {
>          /* Pin mode: pinning specified vcpu to specified physical cpus*/
> -        if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu)))
> +        if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu))) {
> +            vshError(ctl, _("vcpupin: invalid cpulist '%s'"), cpulist);

We do not include the command name for other errors.

If we reused the error from virBitmapParse, we'd get:
error: invalid argument: Failed to parse bitmap '11'

It's a bit more confusing than 'invalid cpulist' (especially since 11
is a wrong bitmap because I only have 4 host CPUs).

I wonder if it's better to fix virBitmapParse to report better errors
(e.g. number out of range) and use it here, or just report generic
"invalid cpulist" for all cases.

>              goto cleanup;
> +        }
>  
>          if (flags == -1) {
>              if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0)
> @@ -6577,8 +6577,10 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
>      }
>  
>      /* Pin mode: pinning emulator threads to specified physical cpus*/
> -    if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu)))
> +    if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu))) {
> +        vshError(ctl, _("emulatorpin: invalid cpulist '%s'"), cpulist);
>          goto cleanup;
> +    }
>  
>      if (flags == -1)
>          flags = VIR_DOMAIN_AFFECT_LIVE;
> @@ -6854,16 +6856,14 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd)
>          goto cleanup;
>      }
>  
> -    if (vshCommandOptString(cmd, "cpulist", &cpulist) < 0) {
> -        vshError(ctl, "%s", _("iothreadpin: invalid cpulist."));
> -        goto cleanup;
> -    }
> -
>      if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0)
>          goto cleanup;
>  
> -    if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu)))
> +    if ((vshCommandOptString(cmd, "cpulist", &cpulist) < 0) ||
> +        !(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu))) {
> +        vshError(ctl, "%s", _("iothreadpin: invalid cpulist."));

This one does not print the wrong string.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150506/9d7712e4/attachment-0001.sig>