<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Feb 21, 2020 at 7:11 PM Mauro S. M. Rodrigues <<a href="mailto:maurosr@linux.vnet.ibm.com">maurosr@linux.vnet.ibm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">virHostCPUGetStatsLinux walks through every cpu in /proc/stat until it<br>
finds cpu%cpuNum that matches with the requested cpu.<br>
If none is found it logs the error but it should return -1, instead of 0.<br>
Otherwise virsh nodecpustats --cpu <invalid cpu number> and API bindings<br>
don't fail properly, printing a blank line instead of an error message.<br>
<br>
This patch also includes an additional test for virhostcputest to avoid<br>
this regression to happen again in the future.<br></blockquote><div><br></div><div>I reviewed and tested this in the scope of [1]</div><div><br></div><div>Before:<br>root@f:~# virsh nodecpustats --cpu 47<br>root@f:~# echo $?<br>0<br><br>After<br>root@f:~# virsh nodecpustats --cpu 47<br>error: Unable to get node cpu stats<br>error: Invalid cpuNum in virHostCPUGetStatsLinux<br>root@f-dcap-after-cap:~# echo $?<br>1<br></div><div><br></div><div>You also see the added test passing</div><div> PASS: virhostcputest</div><div>in the build log at [2]</div><div><br></div><div>Thanks Mauro, feel free to add my review and test tags:</div><div><br></div><div><span style="font-family:monospace"><span style="color:rgb(0,0,0)">Reviewed-by: Christian </span><span style="font-weight:bold;color:rgb(255,84,84)">Ehrha</span><span style="color:rgb(0,0,0)">rdt <<a href="mailto:christian.ehrhardt@canonical.com">christian.ehrhardt@canonical.com</a>>
</span><br>Tested-by: Christian <span style="font-weight:bold;color:rgb(255,84,84)">Ehrha</span><span style="color:rgb(0,0,0)">rdt <<a href="mailto:christian.ehrhardt@canonical.com">christian.ehrhardt@canonical.com</a>></span><br></span></div><div><br></div><div>[1]: <a href="https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1868528">https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1868528</a></div><div>[2]: <a href="https://launchpadlibrarian.net/470309458/buildlog_ubuntu-focal-amd64.libvirt_6.0.0-0ubuntu6~ppa2_BUILDING.txt.gz">https://launchpadlibrarian.net/470309458/buildlog_ubuntu-focal-amd64.libvirt_6.0.0-0ubuntu6~ppa2_BUILDING.txt.gz</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Reported-by: Satheesh Rajendran <<a href="mailto:satheera@in.ibm.com" target="_blank">satheera@in.ibm.com</a>><br>
Signed-off-by: Mauro S. M. Rodrigues <<a href="mailto:maurosr@linux.vnet.ibm.com" target="_blank">maurosr@linux.vnet.ibm.com</a>><br>
---<br>
src/util/virhostcpu.c | 2 +-<br>
tests/virhostcputest.c | 9 ++++++---<br>
2 files changed, 7 insertions(+), 4 deletions(-)<br>
<br>
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c<br>
index 81293eea8c..20c8d0ce6c 100644<br>
--- a/src/util/virhostcpu.c<br>
+++ b/src/util/virhostcpu.c<br>
@@ -847,7 +847,7 @@ virHostCPUGetStatsLinux(FILE *procstat,<br>
_("Invalid cpuNum in %s"),<br>
__FUNCTION__);<br>
<br>
- return 0;<br>
+ return -1;<br>
}<br>
<br>
<br>
diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c<br>
index 7865b61578..2f569d8bd4 100644<br>
--- a/tests/virhostcputest.c<br>
+++ b/tests/virhostcputest.c<br>
@@ -258,14 +258,17 @@ mymain(void)<br>
if (virTestRun(nodeData[i].testName, linuxTestHostCPU, &nodeData[i]) != 0)<br>
ret = -1;<br>
<br>
-# define DO_TEST_CPU_STATS(name, ncpus) \<br>
+# define DO_TEST_CPU_STATS(name, ncpus, shouldFail) \<br>
do { \<br>
static struct nodeCPUStatsData data = { name, ncpus }; \<br>
- if (virTestRun("CPU stats " name, linuxTestNodeCPUStats, &data) < 0) \<br>
+ if ((virTestRun("CPU stats " name, \<br>
+ linuxTestNodeCPUStats, \<br>
+ &data) < 0) != shouldFail) \<br>
ret = -1; \<br>
} while (0)<br>
<br>
- DO_TEST_CPU_STATS("24cpu", 24);<br>
+ DO_TEST_CPU_STATS("24cpu", 24, false);<br>
+ DO_TEST_CPU_STATS("24cpu", 25, true);<br>
<br>
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;<br>
}<br>
-- <br>
2.24.1<br>
<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature">Christian Ehrhardt<br>Staff Engineer, Ubuntu Server<br>Canonical Ltd</div></div>