[libvirt] [PATCH] virDomainGetCPUStats: fix boundary value problem of start_cpu

Jincheng Miao jmiao at redhat.com
Fri Feb 28 03:16:18 UTC 2014


On 02/27/2014 11:24 PM, Michal Privoznik wrote:
> On 26.02.2014 10:18, Jincheng Miao wrote:
>> This API has boundary value problem, if start_cpu is equal to
>> the number of cpus, no error infomation will be reported.
>>
>> This is because the confused meaning of variable max_id,
>> so change the comparision and rename the variable max_id to total_num.
>>
>> Signed-off-by: Jincheng Miao <jmiao at redhat.com>
>> ---
>>   src/qemu/qemu_driver.c | 18 +++++++++---------
>>   src/util/vircgroup.c   | 18 +++++++++---------
>>   tools/virsh-domain.c   | 12 ++++++------
>>   3 files changed, 24 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index c9a865e..54b8e5b 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -15983,7 +15983,7 @@ qemuDomainGetPercpuStats(virDomainObjPtr vm,
>>   {
>>       int rv = -1;
>>       size_t i;
>> -    int id, max_id;
>> +    int id, total_num;
>>       char *pos;
>>       char *buf = NULL;
>>       unsigned long long *sum_cpu_time = NULL;
>> @@ -15999,19 +15999,19 @@ qemuDomainGetPercpuStats(virDomainObjPtr vm,
>>           return QEMU_NB_PER_CPU_STAT_PARAM;
>>
>>       /* To parse account file, we need to know how many cpus are 
>> present.  */
>> -    max_id = nodeGetCPUCount();
>> -    if (max_id < 0)
>> +    total_num = nodeGetCPUCount();
>> +    if (total_num < 0)
>>           return rv;
>>
>> -    if (ncpus == 0) { /* returns max cpu ID */
>> -        rv = max_id;
>> +    if (ncpus == 0) { /* returns total number of cpu */
>> +        rv = total_num;
>>           goto cleanup;
>>       }
>>
>> -    if (start_cpu > max_id) {
>> +    if (start_cpu > total_num - 1) {
>
> This actually is the fix. But ...
>
>> virReportError(VIR_ERR_INVALID_ARG,
>>                          _("start_cpu %d larger than maximum of %d"),
>> -                       start_cpu, max_id);
>> +                       start_cpu, total_num - 1);
>>           goto cleanup;
>>       }
>>
>> @@ -16024,8 +16024,8 @@ qemuDomainGetPercpuStats(virDomainObjPtr vm,
>>       param_idx = 0;
>>
>>       /* number of cpus to compute */
>> -    if (start_cpu >= max_id - ncpus)
>> -        id = max_id - 1;
>> +    if (start_cpu + ncpus >= total_num)
>> +        id = total_num - 1;
>>       else
>>           id = start_cpu + ncpus - 1;
>>
>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
>> index 0f04b4d..2af06af 100644
>> --- a/src/util/vircgroup.c
>> +++ b/src/util/vircgroup.c
>> @@ -2836,7 +2836,7 @@ virCgroupGetPercpuStats(virCgroupPtr group,
>>   {
>>       int rv = -1;
>>       size_t i;
>> -    int id, max_id;
>> +    int id, total_num;
>>       char *pos;
>>       char *buf = NULL;
>>       virTypedParameterPtr ent;
>> @@ -2848,19 +2848,19 @@ virCgroupGetPercpuStats(virCgroupPtr group,
>>           return CGROUP_NB_PER_CPU_STAT_PARAM;
>>
>>       /* To parse account file, we need to know how many cpus are 
>> present.  */
>> -    max_id = nodeGetCPUCount();
>> -    if (max_id < 0)
>> +    total_num = nodeGetCPUCount();
>> +    if (total_num < 0)
>>           return rv;
>>
>> -    if (ncpus == 0) { /* returns max cpu ID */
>> -        rv = max_id;
>> +    if (ncpus == 0) { /* returns total number of cpu */
>> +        rv = total_num;
>>           goto cleanup;
>>       }
>>
>> -    if (start_cpu > max_id) {
>> +    if (start_cpu > total_num - 1) {
>
> because you had to duplicate the fix, it means, we are duplicating the 
> code too. The fix is correct, but I think we should somehow make 
> qemuDomainGetPercpuStats() call virCgroupGetPercpuStats() so we don't 
> duplicate code.

Yep, I can see this point, the code is duplicated.
Should we remove qemuDomainGetPercpuStats()? Because we could call 
virCgroupGetPercpuStats()
in qemuDomainGetCPUStats() instead.
The previous code is cross over, like this:

     if (start_cpu == -1)
         ret = virCgroupGetDomainTotalCpuStats(priv->cgroup,
                                               params, nparams);
     else
         ret = qemuDomainGetPercpuStats(vm, params, nparams,
                                        start_cpu, ncpus);

Maybe we could replace qemuDomainGetPercpuStats() to 
virCgroupGetPercpuStats().

Jincheng Miao

>
> Michal




More information about the libvir-list mailing list