[libvirt] [PATCH v2 0/3] IOThread algorithm followups
v1 here: http://www.redhat.com/archives/libvir-list/2015-April/msg01382.html v2 changes: Patch 1 - Add a !STRPREFIX(tmp, "iothread") of the returned iothreads to ignore anything that doesn't start with "iothread". Patch 2 - already ACKed, just difficult to extract. Patch 3 - NEW: remove qemuMonitorIOThreadInfoFree as requested from review of patch 1. John Ferlan (3): qemu: Remove need for qemuDomainParseIOThreadAlias qemu: qemuProcessDetectIOThreadPIDs invert checks qemu: Remove need for qemuMonitorIOThreadInfoFree src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_command.h | 3 --- src/qemu/qemu_driver.c | 15 +++++---------- src/qemu/qemu_monitor.c | 10 ---------- src/qemu/qemu_monitor.h | 4 +--- src/qemu/qemu_monitor_json.c | 22 +++++++++++++++------- src/qemu/qemu_process.c | 25 +++++++++++-------------- tests/qemumonitorjsontest.c | 14 +++++++------- 8 files changed, 39 insertions(+), 71 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 | 20 ++++++++++++++------ src/qemu/qemu_process.c | 11 ++++------- tests/qemumonitorjsontest.c | 12 ++++++------ 8 files changed, 27 insertions(+), 48 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 74dcb0a..d0147e9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5918,16 +5918,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; @@ -6292,7 +6287,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..c02ef47 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6481,20 +6481,28 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, const char *tmp; qemuMonitorIOThreadInfoPtr info; - if (VIR_ALLOC(info) < 0) - goto cleanup; - - infolist[i] = info; - if (!(tmp = virJSONValueObjectGetString(child, "id"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-iothreads reply data was missing 'id'")); goto cleanup; } - if (VIR_STRDUP(info->name, tmp) < 0) + if (!STRPREFIX(tmp, "iothread")) + continue; + + if (VIR_ALLOC(info) < 0) goto cleanup; + infolist[i] = info; + + 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) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 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 Tue, Apr 28, 2015 at 06:40:30 -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 | 20 ++++++++++++++------ src/qemu/qemu_process.c | 11 ++++------- tests/qemumonitorjsontest.c | 12 ++++++------ 8 files changed, 27 insertions(+), 48 deletions(-)
ACK, 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 Tue, Apr 28, 2015 at 06:40:31 -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.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
This was already acked and separate. You should have pushed it already. Peter
Replace with just VIR_FREE. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 +++--- src/qemu/qemu_monitor.c | 9 --------- src/qemu/qemu_monitor.h | 2 -- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 2 +- tests/qemumonitorjsontest.c | 2 +- 6 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d0147e9..80463f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5950,7 +5950,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver, } if (iothreads) { for (i = 0; i < niothreads; i++) - qemuMonitorIOThreadInfoFree(iothreads[i]); + VIR_FREE(iothreads[i]); VIR_FREE(iothreads); } @@ -6330,7 +6330,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, cleanup: if (new_iothreads) { for (idx = 0; idx < new_niothreads; idx++) - qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads[idx]); VIR_FREE(new_iothreads); } VIR_FREE(mem_mask); @@ -6416,7 +6416,7 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, cleanup: if (new_iothreads) { for (idx = 0; idx < new_niothreads; idx++) - qemuMonitorIOThreadInfoFree(new_iothreads[idx]); + VIR_FREE(new_iothreads[idx]); VIR_FREE(new_iothreads); } virDomainAuditIOThread(vm, orig_niothreads, new_niothreads, diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 48bfeb0..6be3ca2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3813,15 +3813,6 @@ qemuMonitorGetIOThreads(qemuMonitorPtr mon, } -void -qemuMonitorIOThreadInfoFree(qemuMonitorIOThreadInfoPtr iothread) -{ - if (!iothread) - return; - VIR_FREE(iothread); -} - - /** * qemuMonitorGetMemoryDeviceInfo: * @mon: pointer to the monitor diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index bce8031..a07e43b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -883,8 +883,6 @@ struct _qemuMonitorIOThreadInfo { int qemuMonitorGetIOThreads(qemuMonitorPtr mon, qemuMonitorIOThreadInfoPtr **iothreads); -void qemuMonitorIOThreadInfoFree(qemuMonitorIOThreadInfoPtr iothread); - typedef struct _qemuMonitorMemoryDeviceInfo qemuMonitorMemoryDeviceInfo; typedef qemuMonitorMemoryDeviceInfo *qemuMonitorMemoryDeviceInfoPtr; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c02ef47..e281140 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6518,7 +6518,7 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, cleanup: if (ret < 0 && infolist) { for (i = 0; i < n; i++) - qemuMonitorIOThreadInfoFree(infolist[i]); + VIR_FREE(infolist[i]); VIR_FREE(infolist); } virJSONValueFree(cmd); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d8a747c..605b3c6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2265,7 +2265,7 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, cleanup: if (iothreads) { for (i = 0; i < niothreads; i++) - qemuMonitorIOThreadInfoFree(iothreads[i]); + VIR_FREE(iothreads[i]); VIR_FREE(iothreads); } return ret; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 39eeaa7..c6379b6 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2295,7 +2295,7 @@ testQemuMonitorJSONGetIOThreads(const void *data) cleanup: qemuMonitorTestFree(test); for (i = 0; i < ninfo; i++) - qemuMonitorIOThreadInfoFree(info[i]); + VIR_FREE(info[i]); VIR_FREE(info); return ret; -- 2.1.0
On Tue, Apr 28, 2015 at 06:40:32 -0400, John Ferlan wrote:
Replace with just VIR_FREE.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 +++--- src/qemu/qemu_monitor.c | 9 --------- src/qemu/qemu_monitor.h | 2 -- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 2 +- tests/qemumonitorjsontest.c | 2 +- 6 files changed, 6 insertions(+), 17 deletions(-)
ACK, Peter
On 04/28/2015 06:40 AM, John Ferlan wrote:
v1 here: http://www.redhat.com/archives/libvir-list/2015-April/msg01382.html
v2 changes:
Patch 1 - Add a !STRPREFIX(tmp, "iothread") of the returned iothreads to ignore anything that doesn't start with "iothread".
Patch 2 - already ACKed, just difficult to extract.
Patch 3 - NEW: remove qemuMonitorIOThreadInfoFree as requested from review of patch 1.
John Ferlan (3): qemu: Remove need for qemuDomainParseIOThreadAlias qemu: qemuProcessDetectIOThreadPIDs invert checks qemu: Remove need for qemuMonitorIOThreadInfoFree
src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_command.h | 3 --- src/qemu/qemu_driver.c | 15 +++++---------- src/qemu/qemu_monitor.c | 10 ---------- src/qemu/qemu_monitor.h | 4 +--- src/qemu/qemu_monitor_json.c | 22 +++++++++++++++------- src/qemu/qemu_process.c | 25 +++++++++++-------------- tests/qemumonitorjsontest.c | 14 +++++++------- 8 files changed, 39 insertions(+), 71 deletions(-)
Thanks for the reviews - now pushed. John FYI: This cover letter touched on why patch 2 was a repeat... It was just easier to not extract it - perhaps the git merge would have worked, perhaps it would not have.
On Tue, Apr 28, 2015 at 07:23:02 -0400, John Ferlan wrote:
On 04/28/2015 06:40 AM, John Ferlan wrote:
v1 here: http://www.redhat.com/archives/libvir-list/2015-April/msg01382.html
v2 changes:
Patch 1 - Add a !STRPREFIX(tmp, "iothread") of the returned iothreads to ignore anything that doesn't start with "iothread".
Patch 2 - already ACKed, just difficult to extract.
Patch 3 - NEW: remove qemuMonitorIOThreadInfoFree as requested from review of patch 1.
...
Thanks for the reviews - now pushed.
John
FYI: This cover letter touched on why patch 2 was a repeat... It was just easier to not extract it - perhaps the git merge would have worked, perhaps it would not have.
I've tried it just now and both interactive rebase and cherry-pick didn't have problem to move commit 2 to the beginning of the series. The patches don't share any context. Peter
participants (2)
-
John Ferlan -
Peter Krempa