[libvirt] [PATCHv3 0/4] Add a test for linuxNodeGetCPUStats
v1: https://www.redhat.com/archives/libvir-list/2014-January/msg00951.html v2: https://www.redhat.com/archives/libvir-list/2014-January/msg00992.html use ticks instead of nsecs in test data v3: use nodeinfopriv.h instead of disgusting extern declarations touch up the impossible error message Ján Tomko (4): Move test-local declarations to nodeinfopriv.h Add test for linuxNodeGetCPUStats Simplify linuxNodeGetCPUStats Reword error message for oversized cpu time fields src/Makefile.am | 2 +- src/libvirt_linux.syms | 1 + src/nodeinfo.c | 87 ++++++---------- src/nodeinfopriv.h | 38 +++++++ tests/nodeinfodata/linux-cpustat-24cpu.out | 150 ++++++++++++++++++++++++++++ tests/nodeinfodata/linux-cpustat-24cpu.stat | 25 +++++ tests/nodeinfotest.c | 130 +++++++++++++++++++++++- 7 files changed, 373 insertions(+), 60 deletions(-) create mode 100644 src/nodeinfopriv.h create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.out create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.stat -- 1.8.3.2
linuxNodeInfoCPUPopulate is only used in the nodeinfo.c file and in the test suite. --- src/Makefile.am | 2 +- src/nodeinfo.c | 7 +------ src/nodeinfopriv.h | 33 +++++++++++++++++++++++++++++++++ tests/nodeinfotest.c | 6 +----- 4 files changed, 36 insertions(+), 12 deletions(-) create mode 100644 src/nodeinfopriv.h diff --git a/src/Makefile.am b/src/Makefile.am index 8f77658..e33cf8a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -168,7 +168,7 @@ util/virkeymaps.h: $(srcdir)/util/keymaps.csv \ EXTRA_DIST += util/virthreadpthread.c util/virthreadwin32.c # Internal generic driver infrastructure -NODE_INFO_SOURCES = nodeinfo.h nodeinfo.c +NODE_INFO_SOURCES = nodeinfo.h nodeinfo.c nodeinfopriv.h DATATYPES_SOURCES = datatypes.h datatypes.c DRIVER_SOURCES = \ driver.c driver.h \ diff --git a/src/nodeinfo.c b/src/nodeinfo.c index cba2fc1..775ea75 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -40,7 +40,7 @@ #include "c-ctype.h" #include "viralloc.h" -#include "nodeinfo.h" +#include "nodeinfopriv.h" #include "physmem.h" #include "virlog.h" #include "virerror.h" @@ -189,11 +189,6 @@ freebsdNodeGetMemoryStats(virNodeMemoryStatsPtr params, # define LINUX_NB_MEMORY_STATS_ALL 4 # define LINUX_NB_MEMORY_STATS_CELL 2 -/* NB, this is not static as we need to call it from the testsuite */ -int linuxNodeInfoCPUPopulate(FILE *cpuinfo, - const char *sysfs_dir, - virNodeInfoPtr nodeinfo); - /* Return the positive decimal contents of the given * DIR/cpu%u/FILE, or -1 on error. If DEFAULT_VALUE is non-negative * and the file could not be found, return that instead of an error; diff --git a/src/nodeinfopriv.h b/src/nodeinfopriv.h new file mode 100644 index 0000000..9aa3262 --- /dev/null +++ b/src/nodeinfopriv.h @@ -0,0 +1,33 @@ +/* + * nodeinfopriv.h: internal APIs for testing nodeinfo code + * + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#ifndef __NODEINFO_PRIV_H__ +# define __NODEINFO_PRIV_H__ + +# include "nodeinfo.h" + +# ifdef __linux__ +int linuxNodeInfoCPUPopulate(FILE *cpuinfo, + const char *sysfs_dir, + virNodeInfoPtr nodeinfo); +# endif + +#endif /* __NODEINFO_PRIV_H__ */ diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index 74f6d4d..a6247ce 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -7,7 +7,7 @@ #include "testutils.h" #include "internal.h" -#include "nodeinfo.h" +#include "nodeinfopriv.h" #include "virfile.h" #include "virstring.h" @@ -27,10 +27,6 @@ main(void) #else -extern int linuxNodeInfoCPUPopulate(FILE *cpuinfo, - char *sysfs_dir, - virNodeInfoPtr nodeinfo); - static int linuxTestCompareFiles(const char *cpuinfofile, char *sysfs_dir, -- 1.8.3.2
On 01/22/2014 06:37 AM, Ján Tomko wrote:
linuxNodeInfoCPUPopulate is only used in the nodeinfo.c file and in the test suite. --- src/Makefile.am | 2 +- src/nodeinfo.c | 7 +------ src/nodeinfopriv.h | 33 +++++++++++++++++++++++++++++++++ tests/nodeinfotest.c | 6 +----- 4 files changed, 36 insertions(+), 12 deletions(-) create mode 100644 src/nodeinfopriv.h
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Check if cpu stats are read correctly from a sample /proc/stat collected from a 24 CPU machine. --- src/libvirt_linux.syms | 1 + src/nodeinfo.c | 2 +- src/nodeinfopriv.h | 5 + tests/nodeinfodata/linux-cpustat-24cpu.out | 150 ++++++++++++++++++++++++++++ tests/nodeinfodata/linux-cpustat-24cpu.stat | 25 +++++ tests/nodeinfotest.c | 124 +++++++++++++++++++++++ 6 files changed, 306 insertions(+), 1 deletion(-) create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.out create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.stat diff --git a/src/libvirt_linux.syms b/src/libvirt_linux.syms index 3500898..b3b2384 100644 --- a/src/libvirt_linux.syms +++ b/src/libvirt_linux.syms @@ -3,6 +3,7 @@ # # nodeinfo.h +linuxNodeGetCPUStats; linuxNodeInfoCPUPopulate; # util/virstatslinux.h diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 775ea75..12495f6 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -676,7 +676,7 @@ cleanup: # define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) -static int +int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, virNodeCPUStatsPtr params, diff --git a/src/nodeinfopriv.h b/src/nodeinfopriv.h index 9aa3262..6692363 100644 --- a/src/nodeinfopriv.h +++ b/src/nodeinfopriv.h @@ -28,6 +28,11 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, const char *sysfs_dir, virNodeInfoPtr nodeinfo); + +int linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virNodeCPUStatsPtr params, + int *nparams); # endif #endif /* __NODEINFO_PRIV_H__ */ diff --git a/tests/nodeinfodata/linux-cpustat-24cpu.out b/tests/nodeinfodata/linux-cpustat-24cpu.out new file mode 100644 index 0000000..0a1a5bc --- /dev/null +++ b/tests/nodeinfodata/linux-cpustat-24cpu.out @@ -0,0 +1,150 @@ +cpu: +kernel: 8751170 +user: 14128079 +idle: 1816344522 +iowait: 81323 + +cpu0: +kernel: 447603 +user: 749021 +idle: 75399242 +iowait: 5295 + +cpu1: +kernel: 167215 +user: 337326 +idle: 76178612 +iowait: 1121 + +cpu2: +kernel: 308930 +user: 666889 +idle: 75649696 +iowait: 4298 + +cpu3: +kernel: 227674 +user: 328464 +idle: 76131634 +iowait: 1219 + +cpu4: +kernel: 299514 +user: 583915 +idle: 75746383 +iowait: 3997 + +cpu5: +kernel: 112287 +user: 231867 +idle: 76336319 +iowait: 798 + +cpu6: +kernel: 546590 +user: 896252 +idle: 75132665 +iowait: 7210 + +cpu7: +kernel: 177715 +user: 342337 +idle: 76154889 +iowait: 1933 + +cpu8: +kernel: 452773 +user: 772479 +idle: 75359327 +iowait: 5845 + +cpu9: +kernel: 1050230 +user: 1079258 +idle: 74532776 +iowait: 3340 + +cpu10: +kernel: 535495 +user: 847295 +idle: 75202362 +iowait: 4038 + +cpu11: +kernel: 171635 +user: 323891 +idle: 76181622 +iowait: 993 + +cpu12: +kernel: 331031 +user: 683257 +idle: 75587176 +iowait: 5174 + +cpu13: +kernel: 112686 +user: 230633 +idle: 76345295 +iowait: 1367 + +cpu14: +kernel: 251393 +user: 547599 +idle: 75824554 +iowait: 5195 + +cpu15: +kernel: 199044 +user: 260673 +idle: 76230586 +iowait: 1379 + +cpu16: +kernel: 244158 +user: 463357 +idle: 75923993 +iowait: 6211 + +cpu17: +kernel: 88571 +user: 189253 +idle: 76411610 +iowait: 1388 + +cpu18: +kernel: 546539 +user: 875655 +idle: 75096896 +iowait: 5756 + +cpu19: +kernel: 186366 +user: 348768 +idle: 76137323 +iowait: 1299 + +cpu20: +kernel: 449460 +user: 765202 +idle: 75348938 +iowait: 4389 + +cpu21: +kernel: 1045076 +user: 1116075 +idle: 74500557 +iowait: 2411 + +cpu22: +kernel: 534125 +user: 847779 +idle: 75178185 +iowait: 5632 + +cpu23: +kernel: 265029 +user: 640815 +idle: 75753872 +iowait: 1026 + diff --git a/tests/nodeinfodata/linux-cpustat-24cpu.stat b/tests/nodeinfodata/linux-cpustat-24cpu.stat new file mode 100644 index 0000000..bc9d449 --- /dev/null +++ b/tests/nodeinfodata/linux-cpustat-24cpu.stat @@ -0,0 +1,25 @@ +cpu 14126233 1846 7764352 1816344522 81323 395581 591237 0 5880634 0 +cpu0 748997 24 320851 75399242 5295 22050 104702 0 331814 0 +cpu1 337325 1 140909 76178612 1121 8962 17344 0 166726 0 +cpu2 666860 29 269302 75649696 4298 18473 21155 0 272094 0 +cpu3 328387 77 211400 76131634 1219 9701 6573 0 115551 0 +cpu4 583896 19 265185 75746383 3997 17525 16804 0 253387 0 +cpu5 231867 0 100660 76336319 798 6856 4771 0 118465 0 +cpu6 896023 229 472933 75132665 7210 25811 47846 0 410328 0 +cpu7 342336 1 159567 76154889 1933 8675 9473 0 204523 0 +cpu8 772415 64 382065 75359327 5845 22810 47898 0 347169 0 +cpu9 1078771 487 1007467 74532776 3340 28419 14344 0 150374 0 +cpu10 847174 121 461786 75202362 4038 25206 48503 0 370309 0 +cpu11 323890 1 153521 76181622 993 9462 8652 0 199566 0 +cpu12 683237 20 290483 75587176 5174 19287 21261 0 293663 0 +cpu13 230633 0 100001 76345295 1367 7171 5514 0 103907 0 +cpu14 547593 6 220118 75824554 5195 14963 16312 0 207464 0 +cpu15 260648 25 185128 76230586 1379 8448 5468 0 76655 0 +cpu16 463328 29 214199 75923993 6211 14403 15556 0 184943 0 +cpu17 189247 6 79317 76411610 1388 5455 3799 0 85456 0 +cpu18 875552 103 470237 75096896 5756 25159 51143 0 408446 0 +cpu19 348767 1 167550 76137323 1299 8813 10003 0 208604 0 +cpu20 765169 33 380697 75348938 4389 21782 46981 0 353323 0 +cpu21 1115675 400 1003579 74500557 2411 28146 13351 0 162678 0 +cpu22 847629 150 463239 75178185 5632 24933 45953 0 376150 0 +cpu23 640804 11 244148 75753872 1026 13061 7820 0 479032 0 diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index a6247ce..7cafd11 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -79,6 +79,93 @@ fail: return ret; } +# define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) + +static int +linuxCPUStatsToBuf(virBufferPtr buf, + int cpu, + virNodeCPUStatsPtr param, + size_t nparams) +{ + size_t i = 0; + + if (cpu < 0) + virBufferAddLit(buf, "cpu:\n"); + else + virBufferAsprintf(buf, "cpu%d:\n", cpu); + + for (i = 0; i < nparams; i++) + virBufferAsprintf(buf, "%s: %llu\n", param[i].field, + param[i].value / TICK_TO_NSEC); + + virBufferAddChar(buf, '\n'); + return 0; +} + +static int +linuxCPUStatsCompareFiles(const char *cpustatfile, + size_t ncpus, + const char *outfile) +{ + int ret = -1; + char *actualData = NULL; + char *expectData = NULL; + FILE *cpustat = NULL; + virNodeCPUStatsPtr params = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + size_t i; + int nparams = 0; + + if (virtTestLoadFile(outfile, &expectData) < 0) + goto fail; + + if (!(cpustat = fopen(cpustatfile, "r"))) { + virReportSystemError(errno, "failed to open '%s': ", cpustatfile); + goto fail; + } + + if (linuxNodeGetCPUStats(NULL, 0, NULL, &nparams) < 0) + goto fail; + + if (VIR_ALLOC_N(params, nparams) < 0) + goto fail; + + if (linuxNodeGetCPUStats(cpustat, VIR_NODE_CPU_STATS_ALL_CPUS, params, + &nparams) < 0) + goto fail; + + if (linuxCPUStatsToBuf(&buf, VIR_NODE_CPU_STATS_ALL_CPUS, + params, nparams) < 0) + goto fail; + + for (i = 0; i < ncpus; i++) { + if (linuxNodeGetCPUStats(cpustat, i, params, &nparams) < 0) + goto fail; + if (linuxCPUStatsToBuf(&buf, i, params, nparams) < 0) + goto fail; + } + + if (!(actualData = virBufferContentAndReset(&buf))) { + virReportOOMError(); + goto fail; + } + + if (STRNEQ(actualData, expectData)) { + virtTestDifference(stderr, expectData, actualData); + goto fail; + } + + ret = 0; + +fail: + virBufferFreeAndReset(&buf); + VIR_FORCE_FCLOSE(cpustat); + VIR_FREE(expectData); + VIR_FREE(actualData); + VIR_FREE(params); + return ret; +} + static int linuxTestNodeInfo(const void *data) @@ -114,6 +201,34 @@ cleanup: return result; } +struct nodeCPUStatsData { + const char *name; + int ncpus; +}; + +static int +linuxTestNodeCPUStats(const void *data) +{ + const struct nodeCPUStatsData *testData = data; + int result = -1; + char *cpustatfile = NULL; + char *outfile = NULL; + + if (virAsprintf(&cpustatfile, "%s/nodeinfodata/linux-cpustat-%s.stat", + abs_srcdir, testData->name) < 0 || + virAsprintf(&outfile, "%s/nodeinfodata/linux-cpustat-%s.out", + abs_srcdir, testData->name) < 0) + goto fail; + + result = linuxCPUStatsCompareFiles(cpustatfile, + testData->ncpus, + outfile); +fail: + VIR_FREE(cpustatfile); + VIR_FREE(outfile); + return result; +} + static int mymain(void) @@ -141,6 +256,15 @@ mymain(void) if (virtTestRun(nodeData[i], linuxTestNodeInfo, nodeData[i]) != 0) ret = -1; +# define DO_TEST_CPU_STATS(name, ncpus) \ + do { \ + static struct nodeCPUStatsData data = { name, ncpus }; \ + if (virtTestRun("CPU stats " name, linuxTestNodeCPUStats, &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_CPU_STATS("24cpu", 24); + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.3.2
On 01/22/2014 06:37 AM, Ján Tomko wrote:
Check if cpu stats are read correctly from a sample /proc/stat collected from a 24 CPU machine. --- src/libvirt_linux.syms | 1 + src/nodeinfo.c | 2 +- src/nodeinfopriv.h | 5 + tests/nodeinfodata/linux-cpustat-24cpu.out | 150 ++++++++++++++++++++++++++++ tests/nodeinfodata/linux-cpustat-24cpu.stat | 25 +++++ tests/nodeinfotest.c | 124 +++++++++++++++++++++++ 6 files changed, 306 insertions(+), 1 deletion(-) create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.out create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.stat
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/25/2014 03:54 AM, Eric Blake wrote:
On 01/22/2014 06:37 AM, Ján Tomko wrote:
Check if cpu stats are read correctly from a sample /proc/stat collected from a 24 CPU machine. --- src/libvirt_linux.syms | 1 + src/nodeinfo.c | 2 +- src/nodeinfopriv.h | 5 + tests/nodeinfodata/linux-cpustat-24cpu.out | 150 ++++++++++++++++++++++++++++ tests/nodeinfodata/linux-cpustat-24cpu.stat | 25 +++++ tests/nodeinfotest.c | 124 +++++++++++++++++++++++ 6 files changed, 306 insertions(+), 1 deletion(-) create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.out create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.stat
ACK
Thanks, pushed along with 1/2. Jan
Split out the repetitive code. --- src/nodeinfo.c | 77 +++++++++++++++++++++++----------------------------------- 1 file changed, 30 insertions(+), 47 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 12495f6..9f82330 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -674,6 +674,20 @@ cleanup: return ret; } +static int +virNodeCPUStatsAssign(virNodeCPUStatsPtr param, + const char *name, + unsigned long long value) +{ + if (virStrcpyStatic(param->field, name) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field kernel cpu time too long for destination")); + return -1; + } + param->value = value; + return 0; +} + # define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) int @@ -712,8 +726,6 @@ linuxNodeGetCPUStats(FILE *procstat, char *buf = line; if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ - size_t i; - if (sscanf(buf, "%*s %llu %llu %llu %llu %llu" // user ~ iowait "%llu %llu %llu %llu %llu", // irq ~ guest_nice @@ -722,51 +734,22 @@ linuxNodeGetCPUStats(FILE *procstat, continue; } - for (i = 0; i < *nparams; i++) { - virNodeCPUStatsPtr param = ¶ms[i]; - - switch (i) { - case 0: /* fill kernel cpu time here */ - if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_KERNEL) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); - goto cleanup; - } - param->value = (sys + irq + softirq) * TICK_TO_NSEC; - break; - - case 1: /* fill user cpu time here */ - if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_USER) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); - goto cleanup; - } - param->value = (usr + ni) * TICK_TO_NSEC; - break; - - case 2: /* fill idle cpu time here */ - if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_IDLE) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); - goto cleanup; - } - param->value = idle * TICK_TO_NSEC; - break; - - case 3: /* fill iowait cpu time here */ - if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_IOWAIT) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); - goto cleanup; - } - param->value = iowait * TICK_TO_NSEC; - break; - - default: - break; - /* should not hit here */ - } - } + if (virNodeCPUStatsAssign(¶ms[0], VIR_NODE_CPU_STATS_KERNEL, + (sys + irq + softirq) * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[1], VIR_NODE_CPU_STATS_USER, + (usr + ni) * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[2], VIR_NODE_CPU_STATS_IDLE, + idle * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[3], VIR_NODE_CPU_STATS_IOWAIT, + iowait * TICK_TO_NSEC) < 0) + goto cleanup; + ret = 0; goto cleanup; } -- 1.8.3.2
On 22.01.2014 14:37, Ján Tomko wrote:
Split out the repetitive code. --- src/nodeinfo.c | 77 +++++++++++++++++++++++----------------------------------- 1 file changed, 30 insertions(+), 47 deletions(-)
ACK Michal
On 01/22/2014 06:37 AM, Ján Tomko wrote:
Split out the repetitive code. --- src/nodeinfo.c | 77 +++++++++++++++++++++++----------------------------------- 1 file changed, 30 insertions(+), 47 deletions(-)
ACK.
@@ -722,51 +734,22 @@ linuxNodeGetCPUStats(FILE *procstat, continue; }
- for (i = 0; i < *nparams; i++) { - virNodeCPUStatsPtr param = ¶ms[i];
The old loop terminates early if *nparams < 4.
+ if (virNodeCPUStatsAssign(¶ms[0], VIR_NODE_CPU_STATS_KERNEL, + (sys + irq + softirq) * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[1], VIR_NODE_CPU_STATS_USER, + (usr + ni) * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[2], VIR_NODE_CPU_STATS_IDLE, + idle * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[3], VIR_NODE_CPU_STATS_IOWAIT, + iowait * TICK_TO_NSEC) < 0) + goto cleanup;
whereas the new code blindly dereferences past the end of the array. Needs another spin that avoids that issue. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/27/2014 05:16 PM, Eric Blake wrote:
On 01/22/2014 06:37 AM, Ján Tomko wrote:
Split out the repetitive code. --- src/nodeinfo.c | 77 +++++++++++++++++++++++----------------------------------- 1 file changed, 30 insertions(+), 47 deletions(-)
ACK.
@@ -722,51 +734,22 @@ linuxNodeGetCPUStats(FILE *procstat, continue; }
- for (i = 0; i < *nparams; i++) { - virNodeCPUStatsPtr param = ¶ms[i];
The old loop terminates early if *nparams < 4.
We can't get there with *nparams != 4: if ((*nparams) != LINUX_NB_CPU_STATS) { virReportInvalidArg(*nparams, _("nparams in %s must be equal to %d"), __FUNCTION__, LINUX_NB_CPU_STATS); goto cleanup; }
+ if (virNodeCPUStatsAssign(¶ms[0], VIR_NODE_CPU_STATS_KERNEL, + (sys + irq + softirq) * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[1], VIR_NODE_CPU_STATS_USER, + (usr + ni) * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[2], VIR_NODE_CPU_STATS_IDLE, + idle * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[3], VIR_NODE_CPU_STATS_IOWAIT, + iowait * TICK_TO_NSEC) < 0) + goto cleanup;
whereas the new code blindly dereferences past the end of the array. Needs another spin that avoids that issue.
Jan
On 01/28/2014 08:50 AM, Ján Tomko wrote:
On 01/27/2014 05:16 PM, Eric Blake wrote:
On 01/22/2014 06:37 AM, Ján Tomko wrote:
Split out the repetitive code. --- src/nodeinfo.c | 77 +++++++++++++++++++++++----------------------------------- 1 file changed, 30 insertions(+), 47 deletions(-)
ACK.
@@ -722,51 +734,22 @@ linuxNodeGetCPUStats(FILE *procstat, continue; }
- for (i = 0; i < *nparams; i++) { - virNodeCPUStatsPtr param = ¶ms[i];
The old loop terminates early if *nparams < 4.
We can't get there with *nparams != 4: if ((*nparams) != LINUX_NB_CPU_STATS) { virReportInvalidArg(*nparams, _("nparams in %s must be equal to %d"), __FUNCTION__, LINUX_NB_CPU_STATS); goto cleanup; }
I've pushed this along with 4/3. Jan
--- src/nodeinfo.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 9f82330..ed7accc 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -681,7 +681,8 @@ virNodeCPUStatsAssign(virNodeCPUStatsPtr param, { if (virStrcpyStatic(param->field, name) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); + "%s", _("kernel cpu time field is too long" + " for the destination")); return -1; } param->value = value; -- 1.8.3.2
On 22.01.2014 14:37, Ján Tomko wrote:
--- src/nodeinfo.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 9f82330..ed7accc 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -681,7 +681,8 @@ virNodeCPUStatsAssign(virNodeCPUStatsPtr param, { if (virStrcpyStatic(param->field, name) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); + "%s", _("kernel cpu time field is too long" + " for the destination")); return -1; } param->value = value;
ACK Michal
On 01/22/2014 06:37 AM, Ján Tomko wrote:
--- src/nodeinfo.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK. Not like this error will ever be hit in real life.
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 9f82330..ed7accc 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -681,7 +681,8 @@ virNodeCPUStatsAssign(virNodeCPUStatsPtr param, { if (virStrcpyStatic(param->field, name) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); + "%s", _("kernel cpu time field is too long" + " for the destination")); return -1; } param->value = value;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Ján Tomko -
Michal Privoznik