[libvirt] [PATCH] virsh nodecpustats returns incorrect stats of cpu on Linux when the number of online cpu exceed 9.
From: Bing Bu Cao <mars@linux.vnet.ibm.com> To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function use STRPREFIX() to match the cpuid with the cpuid read from /proc/cpustat, it will cause obvious error. For example: 'virsh nodecpustats 12' will display stats of cpu1 if the latter is online. This patch fixes this bug. Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 05bc038..6d7926c 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -715,8 +715,9 @@ linuxNodeGetCPUStats(FILE *procstat, while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; + char **buf_header = virStringSplit(buf, " ", 2); - if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i; if (sscanf(buf, @@ -775,6 +776,7 @@ linuxNodeGetCPUStats(FILE *procstat, ret = 0; goto cleanup; } + virStringFreeList(buf_header); } virReportInvalidArg(cpuNum, @@ -782,6 +784,7 @@ linuxNodeGetCPUStats(FILE *procstat, __FUNCTION__); cleanup: + virStringFreeList(buf_header); return ret; } -- 1.7.7.6
On 01/13/2014 11:28 AM, mars@linux.vnet.ibm.com wrote:
From: Bing Bu Cao <mars@linux.vnet.ibm.com>
To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function use STRPREFIX() to match the cpuid with the cpuid read from /proc/cpustat, it will cause obvious error.
For example: 'virsh nodecpustats 12' will display stats of cpu1 if the latter is online.
This patch fixes this bug.
Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 05bc038..6d7926c 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -715,8 +715,9 @@ linuxNodeGetCPUStats(FILE *procstat,
while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; + char **buf_header = virStringSplit(buf, " ", 2);
- if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i;
if (sscanf(buf,
I think it would be simpler to just add a space at the end of cpu_header and keep using STRPREFIX than splitting the string. Jan
On 01/14/2014 09:30 PM, Ján Tomko wrote:
On 01/13/2014 11:28 AM, mars@linux.vnet.ibm.com wrote:
From: Bing Bu Cao <mars@linux.vnet.ibm.com>
To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function use STRPREFIX() to match the cpuid with the cpuid read from /proc/cpustat, it will cause obvious error.
For example: 'virsh nodecpustats 12' will display stats of cpu1 if the latter is online.
This patch fixes this bug.
Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 05bc038..6d7926c 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -715,8 +715,9 @@ linuxNodeGetCPUStats(FILE *procstat,
while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; + char **buf_header = virStringSplit(buf, " ", 2);
- if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i;
if (sscanf(buf,
I think it would be simpler to just add a space at the end of cpu_header and keep using STRPREFIX than splitting the string. Good point, thank you.
Jan
-- Best Regards, Bing Bu Cao
On Mon, Jan 13, 2014 at 06:28:07PM +0800, mars@linux.vnet.ibm.com wrote:
From: Bing Bu Cao <mars@linux.vnet.ibm.com>
To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function use STRPREFIX() to match the cpuid with the cpuid read from /proc/cpustat, it will cause obvious error.
For example: 'virsh nodecpustats 12' will display stats of cpu1 if the latter is online.
This patch fixes this bug.
Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
Can you also provide a test case to validate this. You just need to get a suitable /proc/cpustat file and save it in the tests/ dir and then include linuxNodeGetCPUStats() using that file. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/14/2014 09:40 PM, Daniel P. Berrange wrote:
On Mon, Jan 13, 2014 at 06:28:07PM +0800, mars@linux.vnet.ibm.com wrote:
From: Bing Bu Cao <mars@linux.vnet.ibm.com>
To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function use STRPREFIX() to match the cpuid with the cpuid read from /proc/cpustat, it will cause obvious error.
For example: 'virsh nodecpustats 12' will display stats of cpu1 if the latter is online.
This patch fixes this bug.
Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
Can you also provide a test case to validate this. You just need to get a suitable /proc/cpustat file and save it in the tests/ dir and then include linuxNodeGetCPUStats() using that file. I found there is no test case for linuxNodeGetCPUStats() right now, it is 'static' there. I think adding a new test case to validate linuxNodeGetCPUStats() should not only cover this patch.
What do you think posting another patch to adding the test case?
Daniel
-- Best Regards, Bing Bu Cao
On 01/16/2014 01:09 AM, Bing Bu Cao wrote:
Can you also provide a test case to validate this. You just need to get a suitable /proc/cpustat file and save it in the tests/ dir and then include linuxNodeGetCPUStats() using that file. I found there is no test case for linuxNodeGetCPUStats() right now, it is 'static' there. I think adding a new test case to validate linuxNodeGetCPUStats() should not only cover this patch.
What do you think posting another patch to adding the test case?
Adding patches to enhance the testsuite is always a good idea. Go for it! -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Jan 16, 2014 at 06:06:57AM -0700, Eric Blake wrote:
On 01/16/2014 01:09 AM, Bing Bu Cao wrote:
Can you also provide a test case to validate this. You just need to get a suitable /proc/cpustat file and save it in the tests/ dir and then include linuxNodeGetCPUStats() using that file. I found there is no test case for linuxNodeGetCPUStats() right now, it is 'static' there.
For something which is static, the approach would be to add a new header file 'nodeinfopriv.h' which exports this. The header would only be used by the test suite
I think adding a new test case to validate linuxNodeGetCPUStats() should not only cover this patch.
What do you think posting another patch to adding the test case?
Adding patches to enhance the testsuite is always a good idea. Go for it!
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (5)
-
Bing Bu Cao -
Daniel P. Berrange -
Eric Blake -
Ján Tomko -
mars＠linux.vnet.ibm.com