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

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(a)redhat.com&gt; > --- > 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.

Michal