[libvirt] [PATCH] virsh nodecpustats returns incorrect stats of cpu on Linux when the number of online cpu exceed 9.
4:28 a.m.
From: Bing Bu Cao <mars(a)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(a)linux.vnet.ibm.com>
Signed-off-by: Pradipta Kr. Banerjee <bpradip(a)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
7:30 a.m.
On 01/13/2014 11:28 AM, mars(a)linux.vnet.ibm.com wrote:
From: Bing Bu Cao <mars(a)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(a)linux.vnet.ibm.com>
Signed-off-by: Pradipta Kr. Banerjee <bpradip(a)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
11:37 p.m.
On 01/14/2014 09:30 PM, Ján Tomko wrote:
On 01/13/2014 11:28 AM, mars(a)linux.vnet.ibm.com wrote:
> From: Bing Bu Cao <mars(a)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(a)linux.vnet.ibm.com>
> Signed-off-by: Pradipta Kr. Banerjee <bpradip(a)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
7:40 a.m.
On Mon, Jan 13, 2014 at 06:28:07PM +0800, mars(a)linux.vnet.ibm.com wrote:
From: Bing Bu Cao <mars(a)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(a)linux.vnet.ibm.com>
Signed-off-by: Pradipta Kr. Banerjee <bpradip(a)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 :|
2:09 a.m.
On 01/14/2014 09:40 PM, Daniel P. Berrange wrote:
On Mon, Jan 13, 2014 at 06:28:07PM +0800, mars(a)linux.vnet.ibm.com
wrote:
> From: Bing Bu Cao <mars(a)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(a)linux.vnet.ibm.com>
> Signed-off-by: Pradipta Kr. Banerjee <bpradip(a)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
7:06 a.m.
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
7:26 a.m.
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 :|
3964
days inactive
3967
days old
6 comments
5 participants
participants (5)
-
Bing Bu Cao -
Daniel P. Berrange -
Eric Blake -
Ján Tomko -
mars＠linux.vnet.ibm.com