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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list