12:30 a.m.
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.
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);
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);
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."));
goto cleanup;
+ }
if (virDomainPinIOThread(dom, iothread_id,
cpumap, cpumaplen, flags) != 0)
--
1.8.3.1
6:28 a.m.
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
8:09 a.m.
On 05/06/2015 07:28 PM, Ján Tomko wrote:
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.
Oh, thanks for pointing out that, i missed these place when i read the code.
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.
Got it and since virBitmap* error is not good enough error as a virsh
output error in some case,
mostly is when the lastcpu of cpulist greater than host cpu.
> 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.
I see, I haven't thought about the coverity when i check this place, I
will remove this change in next version.
> 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).
Yes, and i think 'invalid cpulist' still not good enough when the last
cpu of the cpulist greater than the host CPUs (but better than 'Failed
to parse bitmap' )
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.
Maybe we can remove the maxcpu parameter (pass 1024 as the bitmapSize to
virBitmapParse() ) and move the check for maxcpu out vshParseCPUList(),
make the caller to check the maxcpu (can use virBitmapLastSetBit() ),
and output a more friendly error.
> 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.
Yes, I will fix this place in next version.
Thanks a lot for your quick review and clearly guide and explain.
Jan
3523
days inactive
3523
days old
2 comments
2 participants
participants (2)
-
Ján Tomko -
Luyao Huang