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

[Luyao Huang]

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

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
>