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@xxxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list