Split up parts of virDomainVcpuPinDefParseXML into virDomainEmulatorPinDefParseXML. --- src/conf/domain_conf.c | 50 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1763305..ec7f9c9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13171,7 +13171,6 @@ static virDomainPinDefPtr virDomainVcpuPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, int maxvcpus, - bool emulator, bool iothreads) { virDomainPinDefPtr def; @@ -13186,7 +13185,7 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, ctxt->node = node; - if (!emulator && !iothreads) { + if (!iothreads) { ret = virXPathInt("string(./@vcpu)", ctxt, &vcpuid); if ((ret == -2) || (vcpuid < -1)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -13235,10 +13234,7 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, } if (!(tmp = virXMLPropString(node, "cpuset"))) { - if (emulator) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing cpuset for emulatorpin")); - else if (iothreads) + if (iothreads) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for iothreadpin")); else @@ -13262,6 +13258,38 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, } +/* Parse the XML definition for emulatorpin. + * emulatorpin has the form of + * <emulatorpin cpuset='0'/> + */ +static virDomainPinDefPtr +virDomainEmulatorPinDefParseXML(xmlNodePtr node) +{ + virDomainPinDefPtr def; + char *tmp = NULL; + + if (VIR_ALLOC(def) < 0) + return NULL; + + if (!(tmp = virXMLPropString(node, "cpuset"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing cpuset for emulatorpin")); + goto error; + } + + if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto error; + + VIR_FREE(tmp); + return def; + + error: + VIR_FREE(tmp); + VIR_FREE(def); + return NULL; +} + + int virDomainDefMaybeAddController(virDomainDefPtr def, int type, @@ -13942,7 +13970,7 @@ virDomainDefParseXML(xmlDocPtr xml, for (i = 0; i < n; i++) { virDomainPinDefPtr vcpupin = NULL; vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt, - def->maxvcpus, false, false); + def->maxvcpus, false); if (!vcpupin) goto error; @@ -14012,11 +14040,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } - def->cputune.emulatorpin = virDomainVcpuPinDefParseXML(nodes[0], - ctxt, 0, - true, false); - - if (!def->cputune.emulatorpin) + if (!(def->cputune.emulatorpin = virDomainEmulatorPinDefParseXML(nodes[0]))) goto error; } VIR_FREE(nodes); @@ -14035,7 +14059,7 @@ virDomainDefParseXML(xmlDocPtr xml, virDomainPinDefPtr iothreadpin = NULL; iothreadpin = virDomainVcpuPinDefParseXML(nodes[i], ctxt, def->iothreads, - false, true); + true); if (!iothreadpin) goto error; -- 2.2.2

Extract part that parses iothreads into virDomainIothreadPinDefParseXML --- src/conf/domain_conf.c | 112 +++++++++++++++++++++++++++++-------------------- 1 file changed, 66 insertions(+), 46 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ec7f9c9..10ec17a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13153,30 +13153,19 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, return idmap; } -/* Parse the XML definition for a vcpupin or emulatorpin. +/* Parse the XML definition for a vcpupin * * vcpupin has the form of * <vcpupin vcpu='0' cpuset='0'/> - * - * and emulatorpin has the form of - * <emulatorpin cpuset='0'/> - * - * and an iothreadspin has the form - * <iothreadpin iothread='1' cpuset='2'/> - * - * A vcpuid of -1 is valid and only valid for emulatorpin. So callers - * have to check the returned cpuid for validity. */ static virDomainPinDefPtr virDomainVcpuPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - int maxvcpus, - bool iothreads) + int maxvcpus) { virDomainPinDefPtr def; xmlNodePtr oldnode = ctxt->node; int vcpuid = -1; - unsigned int iothreadid; char *tmp = NULL; int ret; @@ -13185,28 +13174,66 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, ctxt->node = node; - if (!iothreads) { - ret = virXPathInt("string(./@vcpu)", ctxt, &vcpuid); - if ((ret == -2) || (vcpuid < -1)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("vcpu id must be an unsigned integer or -1")); - goto error; - } else if (vcpuid == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("vcpu id value -1 is not allowed for vcpupin")); - goto error; - } + ret = virXPathInt("string(./@vcpu)", ctxt, &vcpuid); + if ((ret == -2) || (vcpuid < -1)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("vcpu id must be an unsigned integer or -1")); + goto error; + } else if (vcpuid == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("vcpu id value -1 is not allowed for vcpupin")); + goto error; + } - if (vcpuid >= maxvcpus) { + if (vcpuid >= maxvcpus) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("vcpu id must be less than maxvcpus")); + goto error; + } + + def->id = vcpuid; + + if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("vcpu id must be less than maxvcpus")); - goto error; - } + _("missing cpuset for vcpupin")); - def->id = vcpuid; + goto error; } - if (iothreads && (tmp = virXPathString("string(./@iothread)", ctxt))) { + if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto error; + + cleanup: + VIR_FREE(tmp); + ctxt->node = oldnode; + return def; + + error: + VIR_FREE(def); + goto cleanup; +} + + +/* Parse the XML definition for a iothreadpin + * and an iothreadspin has the form + * <iothreadpin iothread='1' cpuset='2'/> + */ +static virDomainPinDefPtr +virDomainIothreadPinDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + int iothreads) +{ + virDomainPinDefPtr def; + xmlNodePtr oldnode = ctxt->node; + unsigned int iothreadid; + char *tmp = NULL; + + if (VIR_ALLOC(def) < 0) + return NULL; + + ctxt->node = node; + + if ((tmp = virXPathString("string(./@iothread)", ctxt))) { if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid setting for iothread '%s'"), tmp); @@ -13220,11 +13247,9 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, goto error; } - /* NB: maxvcpus is actually def->iothreads - * IOThreads are numbered "iothread1...iothread<n>", where - * "n" is the iothreads value - */ - if (iothreadid > maxvcpus) { + /* IOThreads are numbered "iothread1...iothread<n>", where + * "n" is the iothreads value */ + if (iothreadid > iothreads) { virReportError(VIR_ERR_XML_ERROR, "%s", _("iothread id must not exceed iothreads")); goto error; @@ -13234,13 +13259,8 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, } if (!(tmp = virXMLPropString(node, "cpuset"))) { - if (iothreads) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing cpuset for iothreadpin")); - else - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing cpuset for vcpupin")); - + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing cpuset for iothreadpin")); goto error; } @@ -13258,6 +13278,7 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, } + /* Parse the XML definition for emulatorpin. * emulatorpin has the form of * <emulatorpin cpuset='0'/> @@ -13970,7 +13991,7 @@ virDomainDefParseXML(xmlDocPtr xml, for (i = 0; i < n; i++) { virDomainPinDefPtr vcpupin = NULL; vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt, - def->maxvcpus, false); + def->maxvcpus); if (!vcpupin) goto error; @@ -14057,9 +14078,8 @@ virDomainDefParseXML(xmlDocPtr xml, for (i = 0; i < n; i++) { virDomainPinDefPtr iothreadpin = NULL; - iothreadpin = virDomainVcpuPinDefParseXML(nodes[i], ctxt, - def->iothreads, - true); + iothreadpin = virDomainIothreadPinDefParseXML(nodes[i], ctxt, + def->iothreads); if (!iothreadpin) goto error; -- 2.2.2

Defining a domain with the following config: <domain ...> ... <iothreads>1</iothreads> <cputune> <iothreadpin cpuset='1'/> will result in the following config formatted back: <domain type='kvm'> ... <iothreads>1</iothreads> <cputune> <iothreadpin iothread='0' cpuset='1'/> After restart the VM would vanish. Since our schema requires the @iothread field to be present in <iothreadpin> make it required by the code too. --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10ec17a..2ebd714 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13233,31 +13233,35 @@ virDomainIothreadPinDefParseXML(xmlNodePtr node, ctxt->node = node; - if ((tmp = virXPathString("string(./@iothread)", ctxt))) { - if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid setting for iothread '%s'"), tmp); - goto error; - } - VIR_FREE(tmp); + if (!(tmp = virXPathString("string(./@iothread)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing iothread id in iothreadpin")); + goto error; + } - if (iothreadid == 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("zero is an invalid iothread id value")); - goto error; - } + if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid setting for iothread '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); - /* IOThreads are numbered "iothread1...iothread<n>", where - * "n" is the iothreads value */ - if (iothreadid > iothreads) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("iothread id must not exceed iothreads")); - goto error; - } + if (iothreadid == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("zero is an invalid iothread id value")); + goto error; + } - def->id = iothreadid; + /* IOThreads are numbered "iothread1...iothread<n>", where + * "n" is the iothreads value */ + if (iothreadid > iothreads) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iothread id must not exceed iothreads")); + goto error; } + def->id = iothreadid; + if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for iothreadpin")); -- 2.2.2

Refactor the code to parse the vcpupin in a similar way the iothreadpin code is now structured. This allows to get rid of some very strange conditions and error messages. Additionally since a existing bug ( https://bugzilla.redhat.com/show_bug.cgi?id=1208434 ) allows to add vcpupin definitions for vcpus that don't exist, this patch makes the parser to ignore all vcpupins that don't have a matching vCPU in the definition rather than just offlined ones. --- src/conf/domain_conf.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2ebd714..ddc0bf8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13160,36 +13160,30 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, */ static virDomainPinDefPtr virDomainVcpuPinDefParseXML(xmlNodePtr node, - xmlXPathContextPtr ctxt, - int maxvcpus) + xmlXPathContextPtr ctxt) { virDomainPinDefPtr def; xmlNodePtr oldnode = ctxt->node; - int vcpuid = -1; + unsigned int vcpuid; char *tmp = NULL; - int ret; if (VIR_ALLOC(def) < 0) return NULL; ctxt->node = node; - ret = virXPathInt("string(./@vcpu)", ctxt, &vcpuid); - if ((ret == -2) || (vcpuid < -1)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("vcpu id must be an unsigned integer or -1")); - goto error; - } else if (vcpuid == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("vcpu id value -1 is not allowed for vcpupin")); + if (!(tmp = virXPathString("string(./@vcpu)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing vcpu id in vcpupin")); goto error; } - if (vcpuid >= maxvcpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("vcpu id must be less than maxvcpus")); + if (virStrToLong_uip(tmp, NULL, 10, &vcpuid) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid setting for vcpu '%s'"), tmp); goto error; } + VIR_FREE(tmp); def->id = vcpuid; @@ -13993,11 +13987,8 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; for (i = 0; i < n; i++) { - virDomainPinDefPtr vcpupin = NULL; - vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt, - def->maxvcpus); - - if (!vcpupin) + virDomainPinDefPtr vcpupin; + if (!(vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt))) goto error; if (virDomainPinIsDuplicate(def->cputune.vcpupin, @@ -14015,7 +14006,7 @@ virDomainDefParseXML(xmlDocPtr xml, * <vcpupin> nodes greater than current vcpus, * ignoring them instead. */ - VIR_WARN("Ignore vcpupin for not onlined vcpus"); + VIR_WARN("Ignore vcpupin for missing vcpus"); virDomainPinDefFree(vcpupin); } else { def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; -- 2.2.2

Previously we checked that the vcpu we are trying to set is in range of the number of threads presented by qemu. The problem is that if the VM is offline the count is 0. Since the condition subtracted 1 from the count the number would overflow and the check would never trigger. Change the condition for more sensible ones with specific error messages. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1208434 --- src/qemu/qemu_driver.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6132674..9c6b905 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5084,10 +5084,17 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, priv = vm->privateData; - if (vcpu > (priv->nvcpupids-1)) { + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && vcpu >= vm->def->vcpus) { virReportError(VIR_ERR_INVALID_ARG, - _("vcpu number out of range %d > %d"), - vcpu, priv->nvcpupids - 1); + _("vcpu %d is out of range of live cpu count %d"), + vcpu, vm->def->vcpus); + goto endjob; + } + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && vcpu >= persistentDef->vcpus) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu %d is out of range of persistent cpu count %d"), + vcpu, persistentDef->vcpus); goto endjob; } -- 2.2.2