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

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



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

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


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