[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