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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



----- 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@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 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
> 
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]