Re: [libvirt] [PATCH] virsh: fix no error output when parse cpulist fail
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(a)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
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)