4:57 a.m.
From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
Now that VCPU number are removed from qemu_monitor_text.c.
VCPU string checking also should be removed.
Report-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
---
src/qemu/qemu_monitor_text.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 1b6efba..3a0c55f 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -527,17 +527,10 @@ int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
*/
line = qemucpus;
do {
- char *offset = strchr(line, '#');
+ char *offset = NULL;
char *end = NULL;
int tid = 0;
- /* See if we're all done */
- if (offset == NULL)
- break;
-
- if (end == NULL || *end != ':')
- goto error;
-
/* Extract host Thread ID */
if ((offset = strstr(line, "thread_id=")) == NULL)
goto error;
--
1.7.10.1
12:01 p.m.
On 03/18/2013 05:57 AM, Li Zhang wrote:
From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
Now that VCPU number are removed from qemu_monitor_text.c.
VCPU string checking also should be removed.
Report-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
---
src/qemu/qemu_monitor_text.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
From the view point of this fix resolves the Coverity complaint/error,
this seems fine. However, one nit I saw when looking at the code...
The line:
VIR_DEBUG("pid=%d", tid);
probably should be
VIR_DEBUG("tid=%d", tid);
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 1b6efba..3a0c55f 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -527,17 +527,10 @@ int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
*/
line = qemucpus;
do {
- char *offset = strchr(line, '#');
+ char *offset = NULL;
char *end = NULL;
int tid = 0;
- /* See if we're all done */
- if (offset == NULL)
- break;
I'm not familiar with the output, but if a line didn't have the '#' in
it is there any reason not to break? Especially since now if it doesn't
have thread_id in it, we're going to go to error?
John
-
- if (end == NULL || *end != ':')
- goto error;
-
/* Extract host Thread ID */
if ((offset = strstr(line, "thread_id=")) == NULL)
goto error;
10:29 p.m.
On 2013年03月19日 01:01, John Ferlan wrote:
On 03/18/2013 05:57 AM, Li Zhang wrote:
> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
>
> Now that VCPU number are removed from qemu_monitor_text.c.
> VCPU string checking also should be removed.
>
> Report-by: John Ferlan <jferlan(a)redhat.com>
> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
> ---
> src/qemu/qemu_monitor_text.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>From the view point of this fix resolves the Coverity complaint/error,
this seems fine. However, one nit I saw when looking at the code...
The line:
VIR_DEBUG("pid=%d", tid);
probably should be
VIR_DEBUG("tid=%d", tid);
From code, tid is stored to cpupids.
I think pid should be fine here. :)
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 1b6efba..3a0c55f 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -527,17 +527,10 @@ int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
> */
> line = qemucpus;
> do {
> - char *offset = strchr(line, '#');
> + char *offset = NULL;
> char *end = NULL;
> int tid = 0;
>
> - /* See if we're all done */
> - if (offset == NULL)
> - break;
I'm not familiar with the output, but if a line didn't have the '#' in
it is there any reason not to break? Especially since now if it doesn't
have thread_id in it, we're going to go to error?
(qemu) info cpus
* CPU #0: nip=0x000000000dcd8814 thread_id=49285
CPU #4: nip=0x0000000000000100 (halted) thread_id=49286
CPU #8: nip=0x0000000000000100 (halted) thread_id=49287
CPU #12: nip=0x0000000000000100 (halted) thread_id=49288
From QEMU's code, there is one '\r\n' by the end of every line.
Have a second thought, the last position of line will be at \r or \n.
It won't be out of loop unless when thread_id can't be found.
Ah, it seems that it's necessary to keep the break.
Thanks.
> -
> - if (end == NULL || *end != ':')
> - goto error;
> -
> /* Extract host Thread ID */
> if ((offset = strstr(line, "thread_id=")) == NULL)
> goto error;
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:08 p.m.
On 03/18/2013 11:01 AM, John Ferlan wrote:
On 03/18/2013 05:57 AM, Li Zhang wrote:
> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
>
> Now that VCPU number are removed from qemu_monitor_text.c.
Mentioning commit cc78d7ba would have been helpful.
> VCPU string checking also should be removed.
>
> Report-by: John Ferlan <jferlan(a)redhat.com>
> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
> ---
> src/qemu/qemu_monitor_text.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>From the view point of this fix resolves the Coverity complaint/error,
this seems fine. However, one nit I saw when looking at the code...
The line:
VIR_DEBUG("pid=%d", tid);
probably should be
VIR_DEBUG("tid=%d", tid);
On Linux, thread ids share the same namespace as process ids, so either
works, but I like the tweak to 'tid'. It's a debug message after all,
so no real impact.
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -527,17 +527,10 @@ int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
> */
> line = qemucpus;
> do {
> - char *offset = strchr(line, '#');
> + char *offset = NULL;
> char *end = NULL;
> int tid = 0;
>
> - /* See if we're all done */
> - if (offset == NULL)
> - break;
I'm not familiar with the output, but if a line didn't have the '#' in
it is there any reason not to break? Especially since now if it doesn't
have thread_id in it, we're going to go to error?
I think we're okay here; we use QMP for new qemu, and older qemu isn't
going to change its text output (which we documented a few lines above
in a comment:
/*
* This is the gross format we're about to parse :-{
*
* (qemu) info cpus
* * CPU #0: pc=0x00000000000f0c4a thread_id=30019
* CPU #1: pc=0x00000000fffffff0 thread_id=30020
* CPU #2: pc=0x00000000fffffff0 thread_id=30021
*
*/
ACK. I went ahead and pushed this.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4266
days inactive
4268
days old
3 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Li Zhang