util: virhostcpu: Fail when fetching CPU Stats for invalid cpu

[PATCH] util: virhostcpu: Fail when fetching CPU Stats for invalid cpu

Posted by Mauro S. M. Rodrigues 4 years, 2 months ago

virHostCPUGetStatsLinux walks through every cpu in /proc/stat until it
finds cpu%cpuNum that matches with the requested cpu.
If none is found it logs the error but it should return -1, instead of 0.
Otherwise virsh nodecpustats --cpu <invalid cpu number> and API bindings
don't fail properly, printing a blank line instead of an error message.

This patch also includes an additional test for virhostcputest to avoid
this regression to happen again in the future.

Reported-by: Satheesh Rajendran <satheera@in.ibm.com>
Signed-off-by: Mauro S. M. Rodrigues <maurosr@linux.vnet.ibm.com>
---
 src/util/virhostcpu.c  | 2 +-
 tests/virhostcputest.c | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 81293eea8c..20c8d0ce6c 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -847,7 +847,7 @@ virHostCPUGetStatsLinux(FILE *procstat,
                         _("Invalid cpuNum in %s"),
                         __FUNCTION__);
 
-    return 0;
+    return -1;
 }
 
 
diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c
index 7865b61578..2f569d8bd4 100644
--- a/tests/virhostcputest.c
+++ b/tests/virhostcputest.c
@@ -258,14 +258,17 @@ mymain(void)
         if (virTestRun(nodeData[i].testName, linuxTestHostCPU, &nodeData[i]) != 0)
             ret = -1;
 
-# define DO_TEST_CPU_STATS(name, ncpus) \
+# define DO_TEST_CPU_STATS(name, ncpus, shouldFail) \
     do { \
         static struct nodeCPUStatsData data = { name, ncpus }; \
-        if (virTestRun("CPU stats " name, linuxTestNodeCPUStats, &data) < 0) \
+        if ((virTestRun("CPU stats " name, \
+			linuxTestNodeCPUStats, \
+			&data) < 0) != shouldFail) \
             ret = -1; \
     } while (0)
 
-    DO_TEST_CPU_STATS("24cpu", 24);
+    DO_TEST_CPU_STATS("24cpu", 24, false);
+    DO_TEST_CPU_STATS("24cpu", 25, true);
 
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.24.1

Re: [PATCH] util: virhostcpu: Fail when fetching CPU Stats for invalid cpu

Posted by Christian Ehrhardt 4 years, 1 month ago

On Fri, Feb 21, 2020 at 7:11 PM Mauro S. M. Rodrigues <
maurosr@linux.vnet.ibm.com> wrote:

> virHostCPUGetStatsLinux walks through every cpu in /proc/stat until it
> finds cpu%cpuNum that matches with the requested cpu.
> If none is found it logs the error but it should return -1, instead of 0.
> Otherwise virsh nodecpustats --cpu <invalid cpu number> and API bindings
> don't fail properly, printing a blank line instead of an error message.
>
> This patch also includes an additional test for virhostcputest to avoid
> this regression to happen again in the future.
>

I reviewed and tested this in the scope of [1]

Before:
root@f:~# virsh nodecpustats --cpu 47
root@f:~# echo $?
0

After
root@f:~# virsh nodecpustats --cpu 47
error: Unable to get node cpu stats
error: Invalid cpuNum in virHostCPUGetStatsLinux
root@f-dcap-after-cap:~# echo $?
1

You also see the added test passing
  PASS: virhostcputest
in the build log at [2]

Thanks Mauro, feel free to add my review and test tags:

Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

[1]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1868528
[2]:
https://launchpadlibrarian.net/470309458/buildlog_ubuntu-focal-amd64.libvirt_6.0.0-0ubuntu6~ppa2_BUILDING.txt.gz


> Reported-by: Satheesh Rajendran <satheera@in.ibm.com>
> Signed-off-by: Mauro S. M. Rodrigues <maurosr@linux.vnet.ibm.com>
> ---
>  src/util/virhostcpu.c  | 2 +-
>  tests/virhostcputest.c | 9 ++++++---
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 81293eea8c..20c8d0ce6c 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -847,7 +847,7 @@ virHostCPUGetStatsLinux(FILE *procstat,
>                          _("Invalid cpuNum in %s"),
>                          __FUNCTION__);
>
> -    return 0;
> +    return -1;
>  }
>
>
> diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c
> index 7865b61578..2f569d8bd4 100644
> --- a/tests/virhostcputest.c
> +++ b/tests/virhostcputest.c
> @@ -258,14 +258,17 @@ mymain(void)
>          if (virTestRun(nodeData[i].testName, linuxTestHostCPU,
> &nodeData[i]) != 0)
>              ret = -1;
>
> -# define DO_TEST_CPU_STATS(name, ncpus) \
> +# define DO_TEST_CPU_STATS(name, ncpus, shouldFail) \
>      do { \
>          static struct nodeCPUStatsData data = { name, ncpus }; \
> -        if (virTestRun("CPU stats " name, linuxTestNodeCPUStats, &data) <
> 0) \
> +        if ((virTestRun("CPU stats " name, \
> +                       linuxTestNodeCPUStats, \
> +                       &data) < 0) != shouldFail) \
>              ret = -1; \
>      } while (0)
>
> -    DO_TEST_CPU_STATS("24cpu", 24);
> +    DO_TEST_CPU_STATS("24cpu", 24, false);
> +    DO_TEST_CPU_STATS("24cpu", 25, true);
>
>      return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
> --
> 2.24.1
>
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] util: virhostcpu: Fail when fetching CPU Stats for invalid cpu

Posted by Michal Prívozník 4 years, 1 month ago

On 21. 2. 2020 19:10, Mauro S. M. Rodrigues wrote:
> virHostCPUGetStatsLinux walks through every cpu in /proc/stat until it
> finds cpu%cpuNum that matches with the requested cpu.
> If none is found it logs the error but it should return -1, instead of 0.
> Otherwise virsh nodecpustats --cpu <invalid cpu number> and API bindings
> don't fail properly, printing a blank line instead of an error message.
> 
> This patch also includes an additional test for virhostcputest to avoid
> this regression to happen again in the future.
> 
> Reported-by: Satheesh Rajendran <satheera@in.ibm.com>
> Signed-off-by: Mauro S. M. Rodrigues <maurosr@linux.vnet.ibm.com>
> ---
>  src/util/virhostcpu.c  | 2 +-
>  tests/virhostcputest.c | 9 ++++++---
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 81293eea8c..20c8d0ce6c 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -847,7 +847,7 @@ virHostCPUGetStatsLinux(FILE *procstat,
>                          _("Invalid cpuNum in %s"),
>                          __FUNCTION__);
>  
> -    return 0;
> +    return -1;
>  }

Oops, yes. This was introduced by 93af79fba3fd75a8df6b7ca608719dd97f9511a0.

>  
>  
> diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c
> index 7865b61578..2f569d8bd4 100644
> --- a/tests/virhostcputest.c
> +++ b/tests/virhostcputest.c
> @@ -258,14 +258,17 @@ mymain(void)
>          if (virTestRun(nodeData[i].testName, linuxTestHostCPU, &nodeData[i]) != 0)
>              ret = -1;
>  
> -# define DO_TEST_CPU_STATS(name, ncpus) \
> +# define DO_TEST_CPU_STATS(name, ncpus, shouldFail) \
>      do { \
>          static struct nodeCPUStatsData data = { name, ncpus }; \
> -        if (virTestRun("CPU stats " name, linuxTestNodeCPUStats, &data) < 0) \
> +        if ((virTestRun("CPU stats " name, \
> +			linuxTestNodeCPUStats, \
> +			&data) < 0) != shouldFail) \

We do it slightly differently. We pass shouldFail to the callback and
let it fix its retval so that virTestRun() interprets it properly.

>              ret = -1; \
>      } while (0)
>  
> -    DO_TEST_CPU_STATS("24cpu", 24);
> +    DO_TEST_CPU_STATS("24cpu", 24, false);
> +    DO_TEST_CPU_STATS("24cpu", 25, true);
>  
>      return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
> 

I'm fixing the test, adding the referenced commit into the commit
message and pushing.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Congratulations on your first libvirt contribution!

Michal