[libvirt] [PATCH 00/13] iothread code cleanup
This is a preparation for implementation of recently added iothread polling feature into QEMU. Pavel Hrdina (13): conf: fix indentation conf: remove redundant iothreads variable conf: move iothread parse code into its own function conf: store iothreads in order by iothread_id conf: always display iothread ids in the XML qemu_driver: check invalid iothread_id before we do anything else qemu_process: move capabilities check for iothreads qemu_process: remove unnecessary iothread check conf: move iothread XML validation from qemu_command qemu_driver: always check whether iothread is used by disk or not qemu_driver: move iothread existence check into one place qemu_driver: check whether iothread is used by controller qemu_driver: move iothread duplicate check into one place src/conf/domain_conf.c | 260 ++++++++++++++------- src/conf/domain_conf.h | 2 - src/qemu/qemu_command.c | 121 +--------- src/qemu/qemu_driver.c | 131 ++++++----- src/qemu/qemu_process.c | 61 ++--- .../qemuxml2argv-iothreads-ids-partial.args | 4 +- .../qemuxml2xmlout-cputune-iothreads.xml | 4 + ...l2xmlout-cputune-iothreadsched-zeropriority.xml | 6 + .../qemuxml2xmlout-cputune-iothreadsched.xml | 6 + .../qemuxml2xmlout-cputune-numatune.xml | 4 + .../qemuxml2xmlout-iothreads-disk-virtio-ccw.xml | 4 + .../qemuxml2xmlout-iothreads-disk.xml | 4 + .../qemuxml2xmlout-iothreads-ids-partial.xml | 2 + .../qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml | 4 + .../qemuxml2xmlout-iothreads-virtio-scsi-pci.xml | 4 + .../qemuxml2xmlout-iothreads.xml | 4 + .../qemuxml2xmlout-vcpu-placement-static.xml | 4 + 17 files changed, 334 insertions(+), 291 deletions(-) -- 2.11.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1bc72a4e90..a179c1e278 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15538,7 +15538,7 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node, if (virStrToLong_uip(tmp, NULL, 10, &iothrid->iothread_id) < 0 || iothrid->iothread_id == 0) { virReportError(VIR_ERR_XML_ERROR, - _("invalid iothread 'id' value '%s'"), tmp); + _("invalid iothread 'id' value '%s'"), tmp); goto error; } -- 2.11.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 32 +++++++++++++++++--------------- src/conf/domain_conf.h | 1 - src/qemu/qemu_driver.c | 6 ------ src/qemu/qemu_process.c | 1 - 4 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a179c1e278..71cd572a30 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2558,7 +2558,8 @@ virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, static int -virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) +virDomainIOThreadIDDefArrayInit(virDomainDefPtr def, + unsigned int iothreads) { int retval = -1; size_t i; @@ -2569,11 +2570,11 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) /* Same value (either 0 or some number), then we have none to fill in or * the iothreadid array was filled from the XML */ - if (def->iothreads == def->niothreadids) + if (iothreads == def->niothreadids) return 0; /* iothread's are numbered starting at 1, account for that */ - if (!(thrmap = virBitmapNew(def->iothreads + 1))) + if (!(thrmap = virBitmapNew(iothreads + 1))) goto error; virBitmapSetAll(thrmap); @@ -2585,11 +2586,11 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) def->iothreadids[i]->iothread_id)); /* resize array */ - if (VIR_REALLOC_N(def->iothreadids, def->iothreads) < 0) + if (VIR_REALLOC_N(def->iothreadids, iothreads) < 0) goto error; /* Populate iothreadids[] using the set bit number from thrmap */ - while (def->niothreadids < def->iothreads) { + while (def->niothreadids < iothreads) { if ((nxt = virBitmapNextSetBit(thrmap, nxt)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to populate iothreadids")); @@ -16755,8 +16756,9 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; /* Optional - iothreads */ + unsigned int iothreads; tmp = virXPathString("string(./iothreads[1])", ctxt); - if (tmp && virStrToLong_uip(tmp, NULL, 10, &def->iothreads) < 0) { + if (tmp && virStrToLong_uip(tmp, NULL, 10, &iothreads) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid iothreads count '%s'"), tmp); goto error; @@ -16767,8 +16769,8 @@ virDomainDefParseXML(xmlDocPtr xml, if ((n = virXPathNodeSet("./iothreadids/iothread", ctxt, &nodes)) < 0) goto error; - if (n > def->iothreads) - def->iothreads = n; + if (n > iothreads) + iothreads = n; if (n && VIR_ALLOC_N(def->iothreadids, n) < 0) goto error; @@ -16789,7 +16791,7 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes); - if (virDomainIOThreadIDDefArrayInit(def) < 0) + if (virDomainIOThreadIDDefArrayInit(def, iothreads) < 0) goto error; /* Extract cpu tunables. */ @@ -19487,11 +19489,11 @@ virDomainDefCheckABIStabilityFlags(virDomainDefPtr src, if (!virDomainDefVcpuCheckAbiStability(src, dst)) goto error; - if (src->iothreads != dst->iothreads) { + if (src->niothreadids != dst->niothreadids) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain iothreads count %u does not " - "match source %u"), - dst->iothreads, src->iothreads); + _("Target domain iothreads count %lu does not " + "match source %lu"), + dst->niothreadids, src->niothreadids); goto error; } @@ -23812,8 +23814,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, goto error; if (def->niothreadids > 0) { - virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", - def->iothreads); + virBufferAsprintf(buf, "<iothreads>%lu</iothreads>\n", + def->niothreadids); /* Only print out iothreadids if we read at least one */ for (i = 0; i < def->niothreadids; i++) { if (!def->iothreadids[i]->autofill) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index dd79206f69..8ff2de8c25 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2210,7 +2210,6 @@ struct _virDomainDef { int placement_mode; virBitmapPtr cpumask; - unsigned int iothreads; size_t niothreadids; virDomainIOThreadIDDefPtr *iothreadids; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index afbcded93f..661f6f5d34 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5617,10 +5617,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, _("got wrong number of IOThread ids from QEMU monitor. " "got %d, wanted %d"), new_niothreads, exp_niothreads); - vm->def->iothreads = new_niothreads; goto cleanup; } - vm->def->iothreads = exp_niothreads; /* * If we've successfully added an IOThread, find out where we added it @@ -5716,10 +5714,8 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, _("got wrong number of IOThread ids from QEMU monitor. " "got %d, wanted %d"), new_niothreads, exp_niothreads); - vm->def->iothreads = new_niothreads; goto cleanup; } - vm->def->iothreads = exp_niothreads; virDomainIOThreadIDDel(vm->def, iothread_id); @@ -5798,7 +5794,6 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, if (!virDomainIOThreadIDAdd(persistentDef, iothread_id)) goto endjob; - persistentDef->iothreads++; } else { virDomainIOThreadIDDefPtr iothrid; if (!(iothrid = virDomainIOThreadIDFind(persistentDef, @@ -5811,7 +5806,6 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, } virDomainIOThreadIDDel(persistentDef, iothread_id); - persistentDef->iothreads--; } if (virDomainSaveConfig(cfg->configDir, driver->caps, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 76f132bc8f..62f0b9b630 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2123,7 +2123,6 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, /* Remove any trace */ VIR_FREE(vm->def->iothreadids); vm->def->niothreadids = 0; - vm->def->iothreads = 0; } return 0; } -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:06 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 32 +++++++++++++++++--------------- src/conf/domain_conf.h | 1 - src/qemu/qemu_driver.c | 6 ------ src/qemu/qemu_process.c | 1 - 4 files changed, 17 insertions(+), 23 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a179c1e278..71cd572a30 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -16755,8 +16756,9 @@ virDomainDefParseXML(xmlDocPtr xml, goto error;
/* Optional - iothreads */ + unsigned int iothreads;
This will be used uninitialized ...
tmp = virXPathString("string(./iothreads[1])", ctxt); - if (tmp && virStrToLong_uip(tmp, NULL, 10, &def->iothreads) < 0) { + if (tmp && virStrToLong_uip(tmp, NULL, 10, &iothreads) < 0) {
if tmp is NULL
virReportError(VIR_ERR_XML_ERROR, _("invalid iothreads count '%s'"), tmp); goto error; @@ -16767,8 +16769,8 @@ virDomainDefParseXML(xmlDocPtr xml, if ((n = virXPathNodeSet("./iothreadids/iothread", ctxt, &nodes)) < 0) goto error;
- if (n > def->iothreads) - def->iothreads = n; + if (n > iothreads)
... here ...
+ iothreads = n;
if (n && VIR_ALLOC_N(def->iothreadids, n) < 0)
... and here.
goto error;
ACK with the above fixed.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 93 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 56 insertions(+), 37 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 71cd572a30..b303c3f46c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15555,6 +15555,61 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node, } +static int +virDomainDefParseIOThreads(virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + size_t i; + char *tmp; + int n = 0; + unsigned int iothreads = 0; + xmlNodePtr *nodes = NULL; + + tmp = virXPathString("string(./iothreads[1])", ctxt); + if (tmp && virStrToLong_uip(tmp, NULL, 10, &iothreads) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid iothreads count '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); + + /* Extract any iothread id's defined */ + if ((n = virXPathNodeSet("./iothreadids/iothread", ctxt, &nodes)) < 0) + goto error; + + if (n > iothreads) + iothreads = n; + + if (n && VIR_ALLOC_N(def->iothreadids, n) < 0) + goto error; + + for (i = 0; i < n; i++) { + virDomainIOThreadIDDefPtr iothrid = NULL; + if (!(iothrid = virDomainIOThreadIDDefParseXML(nodes[i], ctxt))) + goto error; + + if (virDomainIOThreadIDFind(def, iothrid->iothread_id)) { + virReportError(VIR_ERR_XML_ERROR, + _("duplicate iothread id '%u' found"), + iothrid->iothread_id); + virDomainIOThreadIDDefFree(iothrid); + goto error; + } + def->iothreadids[def->niothreadids++] = iothrid; + } + VIR_FREE(nodes); + + if (virDomainIOThreadIDDefArrayInit(def, iothreads) < 0) + goto error; + + return 0; + + error: + VIR_FREE(nodes); + return -1; +} + + /* Parse the XML definition for a vcpupin * * vcpupin has the form of @@ -16755,43 +16810,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainVcpuParse(def, ctxt, xmlopt) < 0) goto error; - /* Optional - iothreads */ - unsigned int iothreads; - tmp = virXPathString("string(./iothreads[1])", ctxt); - if (tmp && virStrToLong_uip(tmp, NULL, 10, &iothreads) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid iothreads count '%s'"), tmp); - goto error; - } - VIR_FREE(tmp); - - /* Extract any iothread id's defined */ - if ((n = virXPathNodeSet("./iothreadids/iothread", ctxt, &nodes)) < 0) - goto error; - - if (n > iothreads) - iothreads = n; - - if (n && VIR_ALLOC_N(def->iothreadids, n) < 0) - goto error; - - for (i = 0; i < n; i++) { - virDomainIOThreadIDDefPtr iothrid = NULL; - if (!(iothrid = virDomainIOThreadIDDefParseXML(nodes[i], ctxt))) - goto error; - - if (virDomainIOThreadIDFind(def, iothrid->iothread_id)) { - virReportError(VIR_ERR_XML_ERROR, - _("duplicate iothread id '%u' found"), - iothrid->iothread_id); - virDomainIOThreadIDDefFree(iothrid); - goto error; - } - def->iothreadids[def->niothreadids++] = iothrid; - } - VIR_FREE(nodes); - - if (virDomainIOThreadIDDefArrayInit(def, iothreads) < 0) + if (virDomainDefParseIOThreads(def, ctxt) < 0) goto error; /* Extract cpu tunables. */ -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:07 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 93 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 56 insertions(+), 37 deletions(-)
This fixes the bug from the previous patch. ACK
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 46 ++++++++++++++++++++-- .../qemuxml2argv-iothreads-ids-partial.args | 4 +- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b303c3f46c..69db692217 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2558,6 +2558,28 @@ virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, static int +virDomainIOThreadInsertGetPos(virDomainDefPtr def, + virDomainIOThreadIDDefPtr iothread) +{ + int idx; + int pos = -1; + + if (def->niothreadids == 0) + return pos; + + for (idx = def->niothreadids - 1; idx >= 0; idx--) { + if (def->iothreadids[idx]->iothread_id < iothread->iothread_id) + break; + + if (def->iothreadids[idx]->iothread_id > iothread->iothread_id) + pos = idx; + } + + return pos; +} + + +static int virDomainIOThreadIDDefArrayInit(virDomainDefPtr def, unsigned int iothreads) { @@ -2586,11 +2608,13 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def, def->iothreadids[i]->iothread_id)); /* resize array */ - if (VIR_REALLOC_N(def->iothreadids, iothreads) < 0) + if (VIR_EXPAND_N(def->iothreadids, i, iothreads - def->niothreadids) < 0) goto error; /* Populate iothreadids[] using the set bit number from thrmap */ while (def->niothreadids < iothreads) { + int pos; + if ((nxt = virBitmapNextSetBit(thrmap, nxt)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to populate iothreadids")); @@ -2600,7 +2624,12 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def, goto error; iothrid->iothread_id = nxt; iothrid->autofill = true; - def->iothreadids[def->niothreadids++] = iothrid; + + pos = virDomainIOThreadInsertGetPos(def, iothrid); + + /* VIR_INSERT_ELEMENT_INPLACE will never return an error here. */ + ignore_value(VIR_INSERT_ELEMENT_INPLACE(def->iothreadids, pos, + def->niothreadids, iothrid)); } retval = 0; @@ -15584,6 +15613,7 @@ virDomainDefParseIOThreads(virDomainDefPtr def, goto error; for (i = 0; i < n; i++) { + int pos; virDomainIOThreadIDDefPtr iothrid = NULL; if (!(iothrid = virDomainIOThreadIDDefParseXML(nodes[i], ctxt))) goto error; @@ -15595,7 +15625,12 @@ virDomainDefParseIOThreads(virDomainDefPtr def, virDomainIOThreadIDDefFree(iothrid); goto error; } - def->iothreadids[def->niothreadids++] = iothrid; + + pos = virDomainIOThreadInsertGetPos(def, iothrid); + + /* VIR_INSERT_ELEMENT_INPLACE will never return an error here. */ + ignore_value(VIR_INSERT_ELEMENT_INPLACE(def->iothreadids, pos, + def->niothreadids, iothrid)); } VIR_FREE(nodes); @@ -20149,6 +20184,7 @@ virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id) { virDomainIOThreadIDDefPtr iothrid = NULL; + int pos; if (virDomainIOThreadIDFind(def, iothread_id)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -20162,7 +20198,9 @@ virDomainIOThreadIDAdd(virDomainDefPtr def, iothrid->iothread_id = iothread_id; - if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, + pos = virDomainIOThreadInsertGetPos(def, iothrid); + + if (VIR_INSERT_ELEMENT_COPY(def->iothreadids, pos, def->niothreadids, iothrid) < 0) goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args index c44162074a..38e4c01aa6 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args @@ -10,10 +10,10 @@ QEMU_AUDIO_DRV=none \ -M pc \ -m 214 \ -smp 2,sockets=2,cores=1,threads=1 \ --object iothread,id=iothread5 \ --object iothread,id=iothread6 \ -object iothread,id=iothread1 \ -object iothread,id=iothread2 \ +-object iothread,id=iothread5 \ +-object iothread,id=iothread6 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ -nodefaults \ -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:08 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 46 ++++++++++++++++++++-- .../qemuxml2argv-iothreads-ids-partial.args | 4 +- 2 files changed, 44 insertions(+), 6 deletions(-)
I'm not sure about this. Are you sure that we can reorder them and the migration stream will restore them properly? (Or ... do they even have a state?)
On Mon, Feb 20, 2017 at 11:05:13AM +0100, Peter Krempa wrote:
On Fri, Feb 17, 2017 at 15:49:08 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 46 ++++++++++++++++++++-- .../qemuxml2argv-iothreads-ids-partial.args | 4 +- 2 files changed, 44 insertions(+), 6 deletions(-)
I'm not sure about this. Are you sure that we can reorder them and the migration stream will restore them properly? (Or ... do they even have a state?)
I've tested it and it should not break migration but this can be easily dropped so I will not push this one. Pavel
There was at first only iothreads element in the XML. Because iothreads are used by theirs ids we should always print them in the XML even if they are auto-generated. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 32 +++++----------------- src/conf/domain_conf.h | 1 - src/qemu/qemu_driver.c | 2 -- src/qemu/qemu_process.c | 5 ++-- .../qemuxml2xmlout-cputune-iothreads.xml | 4 +++ ...l2xmlout-cputune-iothreadsched-zeropriority.xml | 6 ++++ .../qemuxml2xmlout-cputune-iothreadsched.xml | 6 ++++ .../qemuxml2xmlout-cputune-numatune.xml | 4 +++ .../qemuxml2xmlout-iothreads-disk-virtio-ccw.xml | 4 +++ .../qemuxml2xmlout-iothreads-disk.xml | 4 +++ .../qemuxml2xmlout-iothreads-ids-partial.xml | 2 ++ .../qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml | 4 +++ .../qemuxml2xmlout-iothreads-virtio-scsi-pci.xml | 4 +++ .../qemuxml2xmlout-iothreads.xml | 4 +++ .../qemuxml2xmlout-vcpu-placement-static.xml | 4 +++ 15 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 69db692217..ab0e19ef16 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2623,7 +2623,6 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def, if (VIR_ALLOC(iothrid) < 0) goto error; iothrid->iothread_id = nxt; - iothrid->autofill = true; pos = virDomainIOThreadInsertGetPos(def, iothrid); @@ -20216,18 +20215,10 @@ void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id) { - size_t i, j; + size_t i; for (i = 0; i < def->niothreadids; i++) { if (def->iothreadids[i]->iothread_id == iothread_id) { - /* If we were sequential and removed a threadid in the - * beginning or middle of the list, then unconditionally - * clear the autofill flag so we don't lose these - * definitions for XML formatting. - */ - for (j = i + 1; j < def->niothreadids; j++) - def->iothreadids[j]->autofill = false; - virDomainIOThreadIDDefFree(def->iothreadids[i]); VIR_DELETE_ELEMENT(def->iothreadids, i, def->niothreadids); @@ -23873,23 +23864,14 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->niothreadids > 0) { virBufferAsprintf(buf, "<iothreads>%lu</iothreads>\n", def->niothreadids); - /* Only print out iothreadids if we read at least one */ + virBufferAddLit(buf, "<iothreadids>\n"); + virBufferAdjustIndent(buf, 2); for (i = 0; i < def->niothreadids; i++) { - if (!def->iothreadids[i]->autofill) - break; - } - if (i < def->niothreadids) { - virBufferAddLit(buf, "<iothreadids>\n"); - virBufferAdjustIndent(buf, 2); - for (i = 0; i < def->niothreadids; i++) { - if (def->iothreadids[i]->autofill) - continue; - virBufferAsprintf(buf, "<iothread id='%u'/>\n", - def->iothreadids[i]->iothread_id); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</iothreadids>\n"); + virBufferAsprintf(buf, "<iothread id='%u'/>\n", + def->iothreadids[i]->iothread_id); } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</iothreadids>\n"); } if (virDomainCputuneDefFormat(buf, def) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8ff2de8c25..63faec499a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2068,7 +2068,6 @@ typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; struct _virDomainIOThreadIDDef { - bool autofill; unsigned int iothread_id; int thread_id; virBitmapPtr cpumask; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 661f6f5d34..3f0237da48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5496,7 +5496,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virBitmapFree(iothrid->cpumask); iothrid->cpumask = cpumask; - iothrid->autofill = false; /* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -5546,7 +5545,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virBitmapFree(iothrid->cpumask); iothrid->cpumask = cpumask; - iothrid->autofill = false; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); goto endjob; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 62f0b9b630..2a65e9043c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2115,9 +2115,8 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, /* Check if the domain had defined any iothreadid elements * and supply a VIR_INFO indicating that it's being removed. */ - if (!vm->def->iothreadids[i]->autofill) - VIR_INFO("IOThreads not supported, remove iothread id '%u'", - vm->def->iothreadids[i]->iothread_id); + VIR_INFO("IOThreads not supported, remove iothread id '%u'", + vm->def->iothreadids[i]->iothread_id); virDomainIOThreadIDDefFree(vm->def->iothreadids[i]); } /* Remove any trace */ diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml index 6c95e05131..bf8fa0b1de 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreads.xml @@ -5,6 +5,10 @@ <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>2</vcpu> <iothreads>2</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + </iothreadids> <cputune> <shares>2048</shares> <period>1000000</period> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched-zeropriority.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched-zeropriority.xml index 794a52d571..6547f96835 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched-zeropriority.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched-zeropriority.xml @@ -5,6 +5,12 @@ <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>2</vcpu> <iothreads>4</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + <iothread id='3'/> + <iothread id='4'/> + </iothreadids> <cputune> <shares>2048</shares> <period>1000000</period> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml index cd1dc87b52..0c1479f199 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml @@ -5,6 +5,12 @@ <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>2</vcpu> <iothreads>4</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + <iothread id='3'/> + <iothread id='4'/> + </iothreadids> <cputune> <shares>2048</shares> <period>1000000</period> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml index ff987e7d59..a77eeab934 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml @@ -5,6 +5,10 @@ <currentMemory unit='KiB'>65536</currentMemory> <vcpu placement='auto' current='2'>6</vcpu> <iothreads>2</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + </iothreadids> <cputune> <emulatorpin cpuset='1-3'/> <iothreadpin iothread='1' cpuset='2'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-disk-virtio-ccw.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-disk-virtio-ccw.xml index f6d103978d..de5d276ecb 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-disk-virtio-ccw.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-disk-virtio-ccw.xml @@ -5,6 +5,10 @@ <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>1</vcpu> <iothreads>2</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + </iothreadids> <os> <type arch='s390x' machine='s390-ccw'>hvm</type> <boot dev='hd'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-disk.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-disk.xml index 0eb45b7d15..0b001b9fd3 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-disk.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-disk.xml @@ -5,6 +5,10 @@ <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>2</vcpu> <iothreads>2</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + </iothreadids> <os> <type arch='i686' machine='pc'>hvm</type> <boot dev='hd'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-ids-partial.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-ids-partial.xml index d6deced96a..57f3be96fc 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-ids-partial.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-ids-partial.xml @@ -6,6 +6,8 @@ <vcpu placement='static'>2</vcpu> <iothreads>4</iothreads> <iothreadids> + <iothread id='1'/> + <iothread id='2'/> <iothread id='5'/> <iothread id='6'/> </iothreadids> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml index 5e91f1c4aa..161dec9bce 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml @@ -5,6 +5,10 @@ <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>1</vcpu> <iothreads>2</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + </iothreadids> <os> <type arch='s390x' machine='s390-ccw'>hvm</type> <boot dev='hd'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-pci.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-pci.xml index d8306d4304..c3051599b2 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-pci.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-pci.xml @@ -5,6 +5,10 @@ <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>2</vcpu> <iothreads>2</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + </iothreadids> <os> <type arch='i686' machine='pc'>hvm</type> <boot dev='hd'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads.xml index 5c5d9dbdb1..ff4ae53366 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads.xml @@ -5,6 +5,10 @@ <currentMemory unit='KiB'>219136</currentMemory> <vcpu placement='static'>2</vcpu> <iothreads>2</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + </iothreadids> <os> <type arch='i686' machine='pc'>hvm</type> <boot dev='hd'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-vcpu-placement-static.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vcpu-placement-static.xml index c6471e3a07..9949b17eb6 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-vcpu-placement-static.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vcpu-placement-static.xml @@ -5,6 +5,10 @@ <currentMemory unit='KiB'>65536</currentMemory> <vcpu placement='static' current='2'>6</vcpu> <iothreads>2</iothreads> + <iothreadids> + <iothread id='1'/> + <iothread id='2'/> + </iothreadids> <cputune> <emulatorpin cpuset='1-3'/> <iothreadpin iothread='1' cpuset='2'/> -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:09 +0100, Pavel Hrdina wrote:
There was at first only iothreads element in the XML. Because iothreads are used by theirs ids we should always print them in the XML even if they are auto-generated.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 32 +++++----------------- src/conf/domain_conf.h | 1 - src/qemu/qemu_driver.c | 2 -- src/qemu/qemu_process.c | 5 ++-- .../qemuxml2xmlout-cputune-iothreads.xml | 4 +++ ...l2xmlout-cputune-iothreadsched-zeropriority.xml | 6 ++++ .../qemuxml2xmlout-cputune-iothreadsched.xml | 6 ++++ .../qemuxml2xmlout-cputune-numatune.xml | 4 +++ .../qemuxml2xmlout-iothreads-disk-virtio-ccw.xml | 4 +++ .../qemuxml2xmlout-iothreads-disk.xml | 4 +++ .../qemuxml2xmlout-iothreads-ids-partial.xml | 2 ++ .../qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml | 4 +++ .../qemuxml2xmlout-iothreads-virtio-scsi-pci.xml | 4 +++ .../qemuxml2xmlout-iothreads.xml | 4 +++ .../qemuxml2xmlout-vcpu-placement-static.xml | 4 +++ 15 files changed, 55 insertions(+), 31 deletions(-)
I'm not quite sure whether we should do this globally. I think that adding a function that will show us whether we should format the full list or not would be a better idea. That way you could add the new parameters to it and still omit the formatting of the full list in cases where nothing was specified.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 23 ++++++++++++++-------- .../qemuxml2xmlout-iothreads-ids-partial.xml | 2 ++ 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7612185df1..cef6be48c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23696,6 +23696,20 @@ virDomainCpuDefFormat(virBufferPtr buf, } +static bool +virDomainDefIothreadShouldFormat(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->niothreadids; i++) { + if (!def->iothreadids[i]->autofill) + return true; + } + + return false; +} + + /* This internal version appends to an existing buffer * (possibly with auto-indent), rather than flattening * to string. @@ -23889,17 +23903,10 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->niothreadids > 0) { virBufferAsprintf(buf, "<iothreads>%lu</iothreads>\n", def->niothreadids); - /* Only print out iothreadids if we read at least one */ - for (i = 0; i < def->niothreadids; i++) { - if (!def->iothreadids[i]->autofill) - break; - } - if (i < def->niothreadids) { + if (virDomainDefIothreadShouldFormat(def)) { virBufferAddLit(buf, "<iothreadids>\n"); virBufferAdjustIndent(buf, 2); for (i = 0; i < def->niothreadids; i++) { - if (def->iothreadids[i]->autofill) - continue; virBufferAsprintf(buf, "<iothread id='%u'/>\n", def->iothreadids[i]->iothread_id); } diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-ids-partial.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-ids-partial.xml index d6deced96a..57f3be96fc 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-ids-partial.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-ids-partial.xml @@ -6,6 +6,8 @@ <vcpu placement='static'>2</vcpu> <iothreads>4</iothreads> <iothreadids> + <iothread id='1'/> + <iothread id='2'/> <iothread id='5'/> <iothread id='6'/> </iothreadids> -- 2.11.1
On Mon, Feb 20, 2017 at 18:11:57 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 23 ++++++++++++++-------- .../qemuxml2xmlout-iothreads-ids-partial.xml | 2 ++ 2 files changed, 17 insertions(+), 8 deletions(-)
Yes, that is what I meant. ACK
On Mon, Feb 20, 2017 at 06:15:08PM +0100, Peter Krempa wrote:
On Mon, Feb 20, 2017 at 18:11:57 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 23 ++++++++++++++-------- .../qemuxml2xmlout-iothreads-ids-partial.xml | 2 ++ 2 files changed, 17 insertions(+), 8 deletions(-)
Yes, that is what I meant.
ACK
Thanks, I've pushed the series without patch 4/13. Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3f0237da48..189c10ead5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5752,12 +5752,6 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, virDomainDefPtr persistentDef; int ret = -1; - if (iothread_id == 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid value of 0 for iothread_id")); - return -1; - } - cfg = virQEMUDriverGetConfig(driver); priv = vm->privateData; @@ -5833,6 +5827,12 @@ qemuDomainAddIOThread(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if (iothread_id == 0) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid value of 0 for iothread_id")); + return -1; + } + if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -5860,6 +5860,12 @@ qemuDomainDelIOThread(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if (iothread_id == 0) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid value of 0 for iothread_id")); + return -1; + } + if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:10 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
ACK
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 31 +++++-------------------------- src/qemu/qemu_process.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c00a47a91a..7152ef9322 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2586,23 +2586,11 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, */ static bool qemuCheckSCSIControllerIOThreads(const virDomainDef *domainDef, - virDomainControllerDefPtr def, - virQEMUCapsPtr qemuCaps) + virDomainControllerDefPtr def) { if (!def->iothread) return true; - /* By this time QEMU_CAPS_OBJECT_IOTHREAD was already checked. - * We just need to check if the QEMU_CAPS_VIRTIO_SCSI_IOTHREAD - * capability is set. - */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI_IOTHREAD)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IOThreads for virtio-scsi not supported for " - "this QEMU")); - return false; - } - if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("IOThreads only supported for virtio-scsi " @@ -2681,8 +2669,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { virBufferAddLit(&buf, "virtio-scsi-ccw"); if (def->iothread) { - if (!qemuCheckSCSIControllerIOThreads(domainDef, - def, qemuCaps)) + if (!qemuCheckSCSIControllerIOThreads(domainDef, def)) goto error; virBufferAsprintf(&buf, ",iothread=iothread%u", def->iothread); @@ -2696,8 +2683,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } else { virBufferAddLit(&buf, "virtio-scsi-pci"); if (def->iothread) { - if (!qemuCheckSCSIControllerIOThreads(domainDef, - def, qemuCaps)) + if (!qemuCheckSCSIControllerIOThreads(domainDef, def)) goto error; virBufferAsprintf(&buf, ",iothread=iothread%u", def->iothread); @@ -7389,20 +7375,13 @@ qemuBuildMemCommandLine(virCommandPtr cmd, static int qemuBuildIOThreadCommandLine(virCommandPtr cmd, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) + const virDomainDef *def) { size_t i; if (def->niothreadids == 0) return 0; - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IOThreads not supported for this QEMU")); - return -1; - } - /* Create iothread objects using the defined iothreadids list * and the defined id and name from the list. These may be used * by a disk definition which will associate to an iothread by @@ -9705,7 +9684,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildSmpCommandLine(cmd, def) < 0) goto error; - if (qemuBuildIOThreadCommandLine(cmd, def, qemuCaps) < 0) + if (qemuBuildIOThreadCommandLine(cmd, def) < 0) goto error; if (virDomainNumaGetNodeCount(def->numa) && diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2a65e9043c..4710d4ca28 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4584,6 +4584,37 @@ qemuProcessStartValidateVideo(virDomainObjPtr vm, static int +qemuProcessStartValidateIOThreads(virDomainObjPtr vm, + virQEMUCapsPtr qemuCaps) +{ + size_t i; + + if (vm->def->niothreadids > 0 && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported for this QEMU")); + return -1; + } + + for (i = 0; i < vm->def->ncontrollers; i++) { + virDomainControllerDefPtr cont = vm->def->controllers[i]; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI && + cont->iothread > 0 && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads for virtio-scsi not supported for " + "this QEMU")); + return -1; + } + } + + return 0; +} + + +static int qemuProcessStartValidateXML(virQEMUDriverPtr driver, virDomainObjPtr vm, virQEMUCapsPtr qemuCaps, @@ -4659,6 +4690,9 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, if (qemuProcessStartValidateVideo(vm, qemuCaps) < 0) return -1; + if (qemuProcessStartValidateIOThreads(vm, qemuCaps) < 0) + return -1; + VIR_DEBUG("Checking for any possible (non-fatal) issues"); qemuProcessStartWarnShmem(vm); -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:11 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 31 +++++-------------------------- src/qemu/qemu_process.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c00a47a91a..7152ef9322 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2586,23 +2586,11 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, */ static bool qemuCheckSCSIControllerIOThreads(const virDomainDef *domainDef, - virDomainControllerDefPtr def, - virQEMUCapsPtr qemuCaps) + virDomainControllerDefPtr def)
I think that this complete function could be moved ... [...]
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2a65e9043c..4710d4ca28 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4584,6 +4584,37 @@ qemuProcessStartValidateVideo(virDomainObjPtr vm,
static int +qemuProcessStartValidateIOThreads(virDomainObjPtr vm, + virQEMUCapsPtr qemuCaps) +{ + size_t i; + + if (vm->def->niothreadids > 0 && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported for this QEMU")); + return -1; + } + + for (i = 0; i < vm->def->ncontrollers; i++) { + virDomainControllerDefPtr cont = vm->def->controllers[i]; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI && + cont->iothread > 0 &&
... somewhere here.
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads for virtio-scsi not supported for " + "this QEMU")); + return -1; + } + } + + return 0; +}
ACK to what you have here though
On Mon, Feb 20, 2017 at 15:57:21 +0100, Peter Krempa wrote:
On Fri, Feb 17, 2017 at 15:49:11 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 31 +++++-------------------------- src/qemu/qemu_process.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c00a47a91a..7152ef9322 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2586,23 +2586,11 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, */ static bool qemuCheckSCSIControllerIOThreads(const virDomainDef *domainDef, - virDomainControllerDefPtr def, - virQEMUCapsPtr qemuCaps) + virDomainControllerDefPtr def)
I think that this complete function could be moved ...
Umm, never mind. I just noticed that you've did this a bit differently in the next patch.
The situation covered by the removed code will not ever happen. This code is called only while starting a new QEMU process where the capabilities where already checked and while attaching to existing QEMU process where we don't even detect the iothreads. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_process.c | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4710d4ca28..522f49d8b7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2101,31 +2101,6 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, int ret = -1; size_t i; - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { - /* The following check is because at one time a domain could - * define iothreadids and start the domain - only failing the - * capability check when attempting to add a disk. Because the - * iothreads and [n]iothreadids were left untouched other code - * assumed it could use the ->thread_id value to make thread_id - * based adjustments (e.g. pinning, scheduling) which while - * succeeding would execute on the calling thread. - */ - if (vm->def->niothreadids) { - for (i = 0; i < vm->def->niothreadids; i++) { - /* Check if the domain had defined any iothreadid elements - * and supply a VIR_INFO indicating that it's being removed. - */ - VIR_INFO("IOThreads not supported, remove iothread id '%u'", - vm->def->iothreadids[i]->iothread_id); - virDomainIOThreadIDDefFree(vm->def->iothreadids[i]); - } - /* Remove any trace */ - VIR_FREE(vm->def->iothreadids); - vm->def->niothreadids = 0; - } - return 0; - } - /* Get the list of IOThreads from qemu */ if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:12 +0100, Pavel Hrdina wrote:
The situation covered by the removed code will not ever happen. This code is called only while starting a new QEMU process where the capabilities where already checked and while attaching to existing QEMU process where we don't even detect the iothreads.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_process.c | 25 ------------------------- 1 file changed, 25 deletions(-)
ACK, when called from qemuProcessLaunch we already made sure and when called from qemuProcessAttach there are no iotrheads anyways since we don't support parsing them.
This will ensure that IOThreads are properly validated while a domain is defined. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 62 ++++++++++++++++++++++++++++++-- src/qemu/qemu_command.c | 96 ------------------------------------------------- 2 files changed, 59 insertions(+), 99 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ab0e19ef16..a70084658a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4764,7 +4764,8 @@ virDomainDiskAddressDiskBusCompatibility(virDomainDiskBus bus, static int -virDomainDiskDefValidate(const virDomainDiskDef *disk) +virDomainDiskDefValidate(const virDomainDef *def, + const virDomainDiskDef *disk) { /* Validate LUN configuration */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { @@ -4794,6 +4795,24 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) return -1; } + if (disk->iothread > 0) { + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO || + (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads are only available for virtio pci and " + "virtio ccw disk")); + return -1; + } + + if (!virDomainIOThreadIDFind(def, disk->iothread)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid IOThread id '%u' for disk '%s'"), + disk->iothread, disk->dst); + return -1; + } + } + return 0; } @@ -4845,12 +4864,47 @@ virDomainNetDefValidate(const virDomainNetDef *net) static int +virDomainControllerDefValidate(const virDomainDef *def, + const virDomainControllerDef *cont) +{ + if (cont->iothread > 0) { + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI || + cont->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("IOThreads are only supported for virtio-scsi " + "controllers, model is '%s'"), + virDomainControllerModelSCSITypeToString(cont->model)); + return -1; + } + + if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads are only available for virtio pci and " + "virtio ccw controllers")); + return -1; + } + + if (!virDomainIOThreadIDFind(def, cont->iothread)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid IOThread id '%u' for controller '%s'"), + cont->iothread, + virDomainControllerTypeToString(cont->type)); + return -1; + } + } + + return 0; +} + + +static int virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, const virDomainDef *def) { switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: - return virDomainDiskDefValidate(dev->data.disk); + return virDomainDiskDefValidate(def, dev->data.disk); case VIR_DOMAIN_DEVICE_REDIRDEV: return virDomainRedirdevDefValidate(def, dev->data.redirdev); @@ -4858,6 +4912,9 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_NET: return virDomainNetDefValidate(dev->data.net); + case VIR_DOMAIN_DEVICE_CONTROLLER: + return virDomainControllerDefValidate(def, dev->data.controller); + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -4865,7 +4922,6 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_HOSTDEV: case VIR_DOMAIN_DEVICE_WATCHDOG: - case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_SMARTCARD: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7152ef9322..23438895d2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1877,49 +1877,6 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, } -static bool -qemuCheckIOThreads(const virDomainDef *def, - virDomainDiskDefPtr disk) -{ - /* Right "type" of disk" */ - switch ((virDomainDiskBus)disk->bus) { - case VIR_DOMAIN_DISK_BUS_VIRTIO: - if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IOThreads only available for virtio pci and " - "virtio ccw disk")); - return false; - } - break; - - case VIR_DOMAIN_DISK_BUS_IDE: - case VIR_DOMAIN_DISK_BUS_FDC: - case VIR_DOMAIN_DISK_BUS_SCSI: - case VIR_DOMAIN_DISK_BUS_XEN: - case VIR_DOMAIN_DISK_BUS_USB: - case VIR_DOMAIN_DISK_BUS_UML: - case VIR_DOMAIN_DISK_BUS_SATA: - case VIR_DOMAIN_DISK_BUS_SD: - case VIR_DOMAIN_DISK_BUS_LAST: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("IOThreads not available for bus %s target %s"), - virDomainDiskBusTypeToString(disk->bus), disk->dst); - return false; - } - - /* Can we find the disk iothread in the iothreadid list? */ - if (!virDomainIOThreadIDFind(def, disk->iothread)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Disk iothread '%u' not defined in iothreadid"), - disk->iothread); - return false; - } - - return true; -} - - char * qemuBuildDriveDevStr(const virDomainDef *def, virDomainDiskDefPtr disk, @@ -1938,9 +1895,6 @@ qemuBuildDriveDevStr(const virDomainDef *def, if (!qemuCheckCCWS390AddressSupport(def, disk->info, qemuCaps, disk->dst)) goto error; - if (disk->iothread && !qemuCheckIOThreads(def, disk)) - goto error; - switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_IDE: if (disk->info.addr.drive.target != 0) { @@ -2573,52 +2527,6 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, } -/* qemuCheckSCSIControllerIOThreads: - * @domainDef: Pointer to domain def - * @def: Pointer to controller def - * @qemuCaps: Capabilities - * - * If this controller definition has iothreads set, let's make sure the - * configuration is right before adding to the command line - * - * Returns true if either supported or there are no iothreads for controller; - * otherwise, returns false if configuration is not quite right. - */ -static bool -qemuCheckSCSIControllerIOThreads(const virDomainDef *domainDef, - virDomainControllerDefPtr def) -{ - if (!def->iothread) - return true; - - if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("IOThreads only supported for virtio-scsi " - "controllers model is '%s'"), - virDomainControllerModelSCSITypeToString(def->model)); - return false; - } - - if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IOThreads only available for virtio pci and " - "virtio ccw controllers")); - return false; - } - - /* Can we find the controller iothread in the iothreadid list? */ - if (!virDomainIOThreadIDFind(domainDef, def->iothread)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("controller iothread '%u' not defined in iothreadid"), - def->iothread); - return false; - } - - return true; -} - - char * qemuBuildControllerDevStr(const virDomainDef *domainDef, virDomainControllerDefPtr def, @@ -2669,8 +2577,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { virBufferAddLit(&buf, "virtio-scsi-ccw"); if (def->iothread) { - if (!qemuCheckSCSIControllerIOThreads(domainDef, def)) - goto error; virBufferAsprintf(&buf, ",iothread=iothread%u", def->iothread); } @@ -2683,8 +2589,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } else { virBufferAddLit(&buf, "virtio-scsi-pci"); if (def->iothread) { - if (!qemuCheckSCSIControllerIOThreads(domainDef, def)) - goto error; virBufferAsprintf(&buf, ",iothread=iothread%u", def->iothread); } -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:13 +0100, Pavel Hrdina wrote:
This will ensure that IOThreads are properly validated while a domain is defined.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 62 ++++++++++++++++++++++++++++++-- src/qemu/qemu_command.c | 96 ------------------------------------------------- 2 files changed, 59 insertions(+), 99 deletions(-)
ACK
If virDomainDelIOThread API was called with VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CONFIG and both XML were already a different it could result in removing iothread from config XML even if there was a disk using that iothread. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 189c10ead5..1a7cc12874 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5740,6 +5740,25 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, } static int +qemuDomainDelIOThreadCheck(virDomainDefPtr def, + unsigned int iothread_id) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i]->iothread == iothread_id) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot remove IOThread %u since it " + "is being used by disk '%s'"), + iothread_id, def->disks[i]->dst); + return -1; + } + } + + return 0; +} + +static int qemuDomainChgIOThread(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int iothread_id, @@ -5773,6 +5792,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0) goto endjob; } else { + if (qemuDomainDelIOThreadCheck(def, iothread_id) < 0) + goto endjob; + if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0) goto endjob; } @@ -5797,6 +5819,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, goto endjob; } + if (qemuDomainDelIOThreadCheck(persistentDef, iothread_id) < 0) + goto endjob; + virDomainIOThreadIDDel(persistentDef, iothread_id); } @@ -5855,7 +5880,6 @@ qemuDomainDelIOThread(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; int ret = -1; - size_t i; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -5872,17 +5896,6 @@ qemuDomainDelIOThread(virDomainPtr dom, if (virDomainDelIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - /* If there is a disk using the IOThread to be removed, then fail. */ - for (i = 0; i < vm->def->ndisks; i++) { - if (vm->def->disks[i]->iothread == iothread_id) { - virReportError(VIR_ERR_INVALID_ARG, - _("cannot remove IOThread %u since it " - "is being used by disk '%s'"), - iothread_id, vm->def->disks[i]->dst); - goto cleanup; - } - } - ret = qemuDomainChgIOThread(driver, vm, iothread_id, false, flags); cleanup: -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:14 +0100, Pavel Hrdina wrote:
If virDomainDelIOThread API was called with VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CONFIG and both XML were already a different it could result in removing iothread from config XML even if there was a disk using that iothread.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 189c10ead5..1a7cc12874 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5740,6 +5740,25 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, }
static int +qemuDomainDelIOThreadCheck(virDomainDefPtr def, + unsigned int iothread_id) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i]->iothread == iothread_id) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot remove IOThread %u since it " + "is being used by disk '%s'"), + iothread_id, def->disks[i]->dst); + return -1; + } + } + + return 0; +} + +static int qemuDomainChgIOThread(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int iothread_id, @@ -5773,6 +5792,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0) goto endjob; } else { + if (qemuDomainDelIOThreadCheck(def, iothread_id) < 0) + goto endjob; + if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0) goto endjob; } @@ -5797,6 +5819,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, goto endjob; }
+ if (qemuDomainDelIOThreadCheck(persistentDef, iothread_id) < 0) + goto endjob;
This check should be done prior to modifying the live state so that you don't modify it and the API then returns failure. On the other hand ... It's not worse than it was. Fixing the corner case would require also moving the call to virDomainIOThreadIDFind. Consider this as an ACK Peter
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1a7cc12874..03fea2713d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5675,21 +5675,6 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, int new_niothreads = 0; qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; - /* Normally would use virDomainIOThreadIDFind, but we need the index - * from whence to delete for later... - */ - for (idx = 0; idx < vm->def->niothreadids; idx++) { - if (iothread_id == vm->def->iothreadids[idx]->iothread_id) - break; - } - - if (idx == vm->def->niothreadids) { - virReportError(VIR_ERR_INVALID_ARG, - _("cannot find IOThread '%u' in iothreadids list"), - iothread_id); - return -1; - } - if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) return -1; @@ -5745,6 +5730,13 @@ qemuDomainDelIOThreadCheck(virDomainDefPtr def, { size_t i; + if (!virDomainIOThreadIDFind(def, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in iothreadids list"), + iothread_id); + return -1; + } + for (i = 0; i < def->ndisks; i++) { if (def->disks[i]->iothread == iothread_id) { virReportError(VIR_ERR_INVALID_ARG, @@ -5809,16 +5801,6 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, goto endjob; } else { - virDomainIOThreadIDDefPtr iothrid; - if (!(iothrid = virDomainIOThreadIDFind(persistentDef, - iothread_id))) { - virReportError(VIR_ERR_INVALID_ARG, - _("cannot find IOThread '%u' in persistent " - "iothreadids"), - iothread_id); - goto endjob; - } - if (qemuDomainDelIOThreadCheck(persistentDef, iothread_id) < 0) goto endjob; -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:15 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-)
ACK
This follows the same check for disk, because we cannot remove iothread if it's used by disk or by controller. It could lead to crashing QEMU. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 03fea2713d..1c3db4e13f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5747,6 +5747,17 @@ qemuDomainDelIOThreadCheck(virDomainDefPtr def, } } + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->iothread == iothread_id) { + const char *model = virDomainControllerModelSCSITypeToString(def->controllers[i]->model); + virReportError(VIR_ERR_INVALID_ARG, + _("cannot remove IOThread '%u' since it " + "is being used by controller '%s'"), + iothread_id, model); + return -1; + } + } + return 0; } -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:16 +0100, Pavel Hrdina wrote:
This follows the same check for disk, because we cannot remove iothread if it's used by disk or by controller. It could lead to crashing QEMU.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 03fea2713d..1c3db4e13f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5747,6 +5747,17 @@ qemuDomainDelIOThreadCheck(virDomainDefPtr def, } }
+ for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->iothread == iothread_id) { + const char *model = virDomainControllerModelSCSITypeToString(def->controllers[i]->model); + virReportError(VIR_ERR_INVALID_ARG, + _("cannot remove IOThread '%u' since it " + "is being used by controller '%s'"), + iothread_id, model);
I'd drop the 'model' variable and appropriate part from the commit message. Otherwise it may become obsolete once non-SCSI controllers start to support iothreads. ACK
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 7 ------- src/qemu/qemu_driver.c | 29 ++++++++++++++++++++++------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a70084658a..8b4e54d574 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20241,13 +20241,6 @@ virDomainIOThreadIDAdd(virDomainDefPtr def, virDomainIOThreadIDDefPtr iothrid = NULL; int pos; - if (virDomainIOThreadIDFind(def, iothread_id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot duplicate iothread_id '%u' in iothreadids"), - iothread_id); - return NULL; - } - if (VIR_ALLOC(iothrid) < 0) goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1c3db4e13f..ce2f8ff8bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5582,13 +5582,6 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; virDomainIOThreadIDDefPtr iothrid; - if (virDomainIOThreadIDFind(vm->def, iothread_id)) { - virReportError(VIR_ERR_INVALID_ARG, - _("an IOThread is already using iothread_id '%u'"), - iothread_id); - goto cleanup; - } - if (virAsprintf(&alias, "iothread%u", iothread_id) < 0) return -1; @@ -5724,6 +5717,22 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, goto cleanup; } + +static int +qemuDomainAddIOThreadCheck(virDomainDefPtr def, + unsigned int iothread_id) +{ + if (virDomainIOThreadIDFind(def, iothread_id)) { + virReportError(VIR_ERR_INVALID_ARG, + _("an IOThread is already using iothread_id '%u'"), + iothread_id); + return -1; + } + + return 0; +} + + static int qemuDomainDelIOThreadCheck(virDomainDefPtr def, unsigned int iothread_id) @@ -5792,6 +5801,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, } if (add) { + if (qemuDomainAddIOThreadCheck(def, iothread_id) < 0) + goto endjob; + if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0) goto endjob; } else { @@ -5808,6 +5820,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, if (persistentDef) { if (add) { + if (qemuDomainAddIOThreadCheck(persistentDef, iothread_id) < 0) + goto endjob; + if (!virDomainIOThreadIDAdd(persistentDef, iothread_id)) goto endjob; -- 2.11.1
On Fri, Feb 17, 2017 at 15:49:17 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 7 ------- src/qemu/qemu_driver.c | 29 ++++++++++++++++++++++------- 2 files changed, 22 insertions(+), 14 deletions(-)
ACK
participants (2)
-
Pavel Hrdina -
Peter Krempa