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

[Michal Privoznik]

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.

Michal