[libvirt] [PATCH 1/1] Clean redundant code about VCPU string checking
From: Li Zhang <zhlcindy@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@redhat.com> Signed-off-by: Li Zhang <zhlcindy@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
On 03/18/2013 05:57 AM, Li Zhang wrote:
From: Li Zhang <zhlcindy@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@redhat.com> Signed-off-by: Li Zhang <zhlcindy@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;
On 2013年03月19日 01:01, John Ferlan wrote:
From: Li Zhang <zhlcindy@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@redhat.com> Signed-off-by: Li Zhang <zhlcindy@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,
On 03/18/2013 05:57 AM, Li Zhang wrote: 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/18/2013 11:01 AM, John Ferlan wrote:
On 03/18/2013 05:57 AM, Li Zhang wrote:
From: Li Zhang <zhlcindy@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@redhat.com> Signed-off-by: Li Zhang <zhlcindy@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
participants (3)
-
Eric Blake -
John Ferlan -
Li Zhang