[libvirt] [PATCH 0/2] IOThread algorithm followups
During Peter's review of the recent IOThreads Add/Del series he noted two outstanding issues: 1. The qemuDomainParseIOThreadAlias should be done within the context of the qemuMonitorJSONGetIOThreads and return an iothread_id instead of a name which must be parsed by everyone 2. The order of checks in qemuProcessDetectIOThreadPIDs to compare the returned niothreads against zero, should have been done after the comparison of whether the number returned matches what the domain expects. If the domain had expected some number of iothreads and none were returned, then that needs to be noted. For review details, see: http://www.redhat.com/archives/libvir-list/2015-April/msg01361.html John Ferlan (2): qemu: Remove need for qemuDomainParseIOThreadAlias qemu: qemuProcessDetectIOThreadPIDs invert checks src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_command.h | 3 --- src/qemu/qemu_driver.c | 9 ++------- src/qemu/qemu_monitor.c | 1 - src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 7 ++++++- src/qemu/qemu_process.c | 23 ++++++++++------------- tests/qemumonitorjsontest.c | 12 ++++++------ 8 files changed, 25 insertions(+), 49 deletions(-) -- 2.1.0
Rather than have a separate routine to parse the alias of an iothread returned from qemu in order to get the iothread_id value, parse the alias when returning and just return the iothread_id in qemuMonitorIOThreadInfoPtr This set of patches removes the function, changes the "char *name" to "unsigned int" and handles all the fallout. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_command.h | 3 --- src/qemu/qemu_driver.c | 9 ++------- src/qemu/qemu_monitor.c | 1 - src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 7 ++++++- src/qemu/qemu_process.c | 11 ++++------- tests/qemumonitorjsontest.c | 12 ++++++------ 8 files changed, 19 insertions(+), 43 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 247954f..ba15dc9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -677,23 +677,6 @@ qemuOpenVhostNet(virDomainDefPtr def, return -1; } -int -qemuDomainParseIOThreadAlias(char *alias, - unsigned int *iothread_id) -{ - unsigned int idval; - - if (virStrToLong_ui(alias + strlen("iothread"), - NULL, 10, &idval) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to find iothread id for '%s'"), - alias); - return -1; - } - - *iothread_id = idval; - return 0; -} int qemuNetworkPrepareDevices(virDomainDefPtr def) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 538ccdf..a29db41 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -238,9 +238,6 @@ int qemuOpenVhostNet(virDomainDefPtr def, int *vhostfd, size_t *vhostfdSize); -int qemuDomainParseIOThreadAlias(char *alias, - unsigned int *iothread_id); - int qemuNetworkPrepareDevices(virDomainDefPtr def); /* diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 443341b..a8ce797 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5926,16 +5926,11 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver, goto endjob; for (i = 0; i < niothreads; i++) { - unsigned int iothread_id; virBitmapPtr map = NULL; - if (qemuDomainParseIOThreadAlias(iothreads[i]->name, - &iothread_id) < 0) - goto endjob; - if (VIR_ALLOC(info_ret[i]) < 0) goto endjob; - info_ret[i]->iothread_id = iothread_id; + info_ret[i]->iothread_id = iothreads[i]->iothread_id; if (virProcessGetAffinity(iothreads[i]->thread_id, &map, hostcpus) < 0) goto endjob; @@ -6300,7 +6295,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, * in the QEMU IOThread list, so we can add it to our iothreadids list */ for (idx = 0; idx < new_niothreads; idx++) { - if (STREQ(new_iothreads[idx]->name, alias)) + if (new_iothreads[idx]->iothread_id == iothread_id) break; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1e7d2ef..48bfeb0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3818,7 +3818,6 @@ qemuMonitorIOThreadInfoFree(qemuMonitorIOThreadInfoPtr iothread) { if (!iothread) return; - VIR_FREE(iothread->name); VIR_FREE(iothread); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cd4cc66..bce8031 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -877,7 +877,7 @@ typedef struct _qemuMonitorIOThreadInfo qemuMonitorIOThreadInfo; typedef qemuMonitorIOThreadInfo *qemuMonitorIOThreadInfoPtr; struct _qemuMonitorIOThreadInfo { - char *name; + unsigned int iothread_id; int thread_id; }; int qemuMonitorGetIOThreads(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3af319c..76687ff 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6492,8 +6492,13 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, goto cleanup; } - if (VIR_STRDUP(info->name, tmp) < 0) + if (virStrToLong_ui(tmp + strlen("iothread"), + NULL, 10, &info->iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to find iothread id for '%s'"), + tmp); goto cleanup; + } if (virJSONValueObjectGetNumberInt(child, "thread-id", &info->thread_id) < 0) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5ae2241..f06ec56 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2248,16 +2248,13 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, } for (i = 0; i < niothreads; i++) { - unsigned int iothread_id; virDomainIOThreadIDDefPtr iothrid; - if (qemuDomainParseIOThreadAlias(iothreads[i]->name, - &iothread_id) < 0) - goto cleanup; - - if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { + if (!(iothrid = virDomainIOThreadIDFind(vm->def, + iothreads[i]->iothread_id))) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("iothread %d not found"), iothread_id); + _("iothread %d not found"), + iothreads[i]->iothread_id); goto cleanup; } iothrid->thread_id = iothreads[i]->thread_id; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f729c7c..39eeaa7 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2269,12 +2269,12 @@ testQemuMonitorJSONGetIOThreads(const void *data) goto cleanup; } -#define CHECK(i, wantname, wantthread_id) \ +#define CHECK(i, wantiothread_id, wantthread_id) \ do { \ - if (STRNEQ(info[i]->name, (wantname))) { \ + if (info[i]->iothread_id != (wantiothread_id)) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ - "name %s is not %s", \ - info[i]->name, (wantname)); \ + "iothread_id %u is not %u", \ + info[i]->iothread_id, (wantiothread_id)); \ goto cleanup; \ } \ if (info[i]->thread_id != (wantthread_id)) { \ @@ -2285,8 +2285,8 @@ testQemuMonitorJSONGetIOThreads(const void *data) } \ } while (0) - CHECK(0, "iothread1", 30992); - CHECK(1, "iothread2", 30993); + CHECK(0, 1, 30992); + CHECK(1, 2, 30993); #undef CHECK -- 2.1.0
On Mon, Apr 27, 2015 at 14:51:04 -0400, John Ferlan wrote:
Rather than have a separate routine to parse the alias of an iothread returned from qemu in order to get the iothread_id value, parse the alias when returning and just return the iothread_id in qemuMonitorIOThreadInfoPtr
This set of patches removes the function, changes the "char *name" to "unsigned int" and handles all the fallout.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_command.h | 3 --- src/qemu/qemu_driver.c | 9 ++------- src/qemu/qemu_monitor.c | 1 - src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 7 ++++++- src/qemu/qemu_process.c | 11 ++++------- tests/qemumonitorjsontest.c | 12 ++++++------ 8 files changed, 19 insertions(+), 43 deletions(-)
...
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1e7d2ef..48bfeb0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3818,7 +3818,6 @@ qemuMonitorIOThreadInfoFree(qemuMonitorIOThreadInfoPtr iothread) { if (!iothread) return; - VIR_FREE(iothread->name); VIR_FREE(iothread);
Looks like this function can be killed and replaced with VIR_FREE().
}
...
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3af319c..76687ff 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6492,8 +6492,13 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, goto cleanup; }
- if (VIR_STRDUP(info->name, tmp) < 0) + if (virStrToLong_ui(tmp + strlen("iothread"),
You shouldn't assume that the returned thread will begin with iothread. The code should make sure that the STRPREFIX is "iothread" before moving the pointer. If the alias will be shorter it will crash.
+ NULL, 10, &info->iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to find iothread id for '%s'"), + tmp); goto cleanup; + }
if (virJSONValueObjectGetNumberInt(child, "thread-id", &info->thread_id) < 0) {
Peter
If we received zero iothreads from the monitor, but were perhaps expecting to receive something, then the code was skipping the check to ensure what's in the monitor matches our expectations. So invert the checks to check that what we get back matches expectations and then check there are zero iothreads returned Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f06ec56..d8a747c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2233,12 +2233,6 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, if (niothreads < 0) goto cleanup; - /* Nothing to do */ - if (niothreads == 0) { - ret = 0; - goto cleanup; - } - if (niothreads != vm->def->iothreads) { virReportError(VIR_ERR_INTERNAL_ERROR, _("got wrong number of IOThread pids from QEMU monitor. " @@ -2247,6 +2241,12 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, goto cleanup; } + /* Nothing to do */ + if (niothreads == 0) { + ret = 0; + goto cleanup; + } + for (i = 0; i < niothreads; i++) { virDomainIOThreadIDDefPtr iothrid; -- 2.1.0
On Mon, Apr 27, 2015 at 14:51:05 -0400, John Ferlan wrote:
If we received zero iothreads from the monitor, but were perhaps expecting to receive something, then the code was skipping the check to ensure what's in the monitor matches our expectations. So invert the checks to check that what we get back matches expectations and then check there are zero iothreads returned
Missing full stop at the end of the sentence.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
ACK, Peter
Resolve some Coverity errors with IOThread changes Signed-off-by: John Ferlan <jferlan@redhat.com> --- I ran my Coverity checker too, but I looked at the wrong results tab when I went to check the results... I looked at a previous run which was clean, not the most recent run which wasn't. Doing multiple compiles in separate tabs and just couldn't keep it all straight (<sigh> - old age I guess) src/conf/domain_conf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b18720..fc48ed5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13253,6 +13253,7 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node, error: virDomainIOThreadIDDefFree(iothrid); + iothrid = NULL; goto cleanup; } @@ -13342,7 +13343,7 @@ virDomainIOThreadPinDefParseXML(xmlNodePtr node, { int ret = -1; virDomainIOThreadIDDefPtr iothrid; - virBitmapPtr cpumask; + virBitmapPtr cpumask = NULL; xmlNodePtr oldnode = ctxt->node; unsigned int iothreadid; char *tmp = NULL; @@ -13372,6 +13373,7 @@ virDomainIOThreadPinDefParseXML(xmlNodePtr node, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Cannot find 'iothread' : %u"), iothreadid); + goto cleanup; } if (!(tmp = virXMLPropString(node, "cpuset"))) { -- 2.1.0
On Mon, Apr 27, 2015 at 15:08:42 -0400, John Ferlan wrote:
Resolve some Coverity errors with IOThread changes
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
I ran my Coverity checker too, but I looked at the wrong results tab when I went to check the results... I looked at a previous run which was clean, not the most recent run which wasn't. Doing multiple compiles in separate tabs and just couldn't keep it all straight (<sigh> - old age I guess)
src/conf/domain_conf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b18720..fc48ed5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13253,6 +13253,7 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node,
error: virDomainIOThreadIDDefFree(iothrid); + iothrid = NULL; goto cleanup; }
@@ -13342,7 +13343,7 @@ virDomainIOThreadPinDefParseXML(xmlNodePtr node, { int ret = -1; virDomainIOThreadIDDefPtr iothrid; - virBitmapPtr cpumask; + virBitmapPtr cpumask = NULL; xmlNodePtr oldnode = ctxt->node; unsigned int iothreadid; char *tmp = NULL;
I've already ACKed Roman's patch that fixes this.
@@ -13372,6 +13373,7 @@ virDomainIOThreadPinDefParseXML(xmlNodePtr node, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Cannot find 'iothread' : %u"), iothreadid); + goto cleanup; }
if (!(tmp = virXMLPropString(node, "cpuset"))) {
ACK, Peter
participants (2)
-
John Ferlan -
Peter Krempa