----- Original Message -----

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@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 comparison is between max cpu id and start cpu id. But max_id is the value of total cpu number, got from nodeGetCPUCount(). So it failed to deal with boundary value. And I file a related bug https://bugzilla.redhat.com/show_bug.cgi?id=1070680

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) { virReportError(VIR_ERR_INVALID_ARG, _("start_cpu %d larger than maximum of %d"), - start_cpu, max_id); + start_cpu, total_num - 1); goto cleanup; }

@@ -2873,8 +2873,8 @@ virCgroupGetPercpuStats(virCgroupPtr group, 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/tools/virsh-domain.c b/tools/virsh-domain.c index 3e73340..0ead80f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6331,7 +6331,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; virTypedParameterPtr params = NULL; - int pos, max_id, cpu = 0, show_count = -1, nparams = 0; + int pos, total_num, cpu = 0, show_count = -1, nparams = 0; size_t i, j; bool show_total = false, show_per_cpu = false; unsigned int flags = 0; @@ -6376,12 +6376,12 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) goto do_show_total;

/* get number of cpus on the node */ - if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, flags)) < 0) + if ((total_num = virDomainGetCPUStats(dom, NULL, 0, 0, 0, flags)) < 0) goto failed_stats; - if (show_count < 0 || show_count > max_id) { - if (show_count > max_id) - vshPrint(ctl, _("Only %d CPUs available to show\n"), max_id); - show_count = max_id; + if (show_count < 0 || show_count > total_num) { + if (show_count > total_num) + vshPrint(ctl, _("Only %d CPUs available to show\n"), total_num); + show_count = total_num; }

/* get percpu information */ -- 1.8.5.3