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@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