4:31 p.m.
On 10/16/2015 08:11 AM, Peter Krempa wrote:
Adjust the config code so that it does not enforce that target
memory
node is specified. To avoid breakage, adjust the qemu memory hotplug
config checker to disallow such config for now.
---
docs/formatdomain.html.in | 5 +++--
docs/schemas/domaincommon.rng | 8 +++++---
src/conf/domain_conf.c | 10 +++++++---
src/qemu/qemu_domain.c | 7 +++++++
4 files changed, 22 insertions(+), 8 deletions(-)
It almost feels like patches 9 & 10 should precede this one. Perhaps
easier with the html.in change. Consider this as a review for 8-10 as
well as some other random thoughts.
So if I'm reading things correctly, a PPC64 guest "can" supply NUMA
guest nodes, but it doesn't have to. Is that a fair assumption? It
matters mostly to help describe things. If it can have it, then perhaps
another test (copy qemuxml2argv-memory-hotplug-ppc64-nonuma.xml to
qemuxml2argv-memory-hotplug-ppc64.xml and add the numa node defs).
If PPC64 doesn't support NUMA guest nodes, then does it make sense in
post processing code to set targetNode = -1 if ARCH_IS_PPC64? It
wouldn't be the first PPC64 specific code. Along the same lines, would
be ignoring targetNode in patch 9 if PPC64. Furthermore, if PPC64
doesn't support NUMA guest nodes, I would have expected a check during
parse - I didn't look that hard for this case. I was merely thinking
what happens if PPC64 does define NUMA guest nodes and those can be used
by the memory device.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c88b032..279424d 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6300,8 +6300,9 @@ qemu-kvm -net nic,model=? /dev/null
added memory as a scaled integer.
</p>
<p>
- The mandatory <code>node</code> subelement configures the guest
NUMA
- node to attach the memory to.
+ The <code>node</code> subelement configures the guest NUMA node
to
+ attach the memory to. Note: Some hypervisors or specific
+ configurations may require that <code>node</code> is specified.
How about:
The optional <code>node</code> subelement provides the guest <a
href="#elementsCPU">cpu NUMA node or cell id</a> to attach the memory.
<p> Note: The hypervisor may require the <code>node</code> to be
specified for some architectures.</p>
FWIW: It almost feels like we need an example of what is meant - that is
"For QEMU/KVM, the PowerPC64 pseries guests do not require that the
<code>node</code> subelement is defined. If not defined, then the memory
device will be attached to [???]."
BTW: The additional text only makes sense as of patch 10 and it wasn't
clear if PPC64 "could" support numa guest nodes.
</p>
</dd>
</dl>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f196177..994face 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4532,9 +4532,11 @@
<element name="size">
<ref name="scaledInteger"/>
</element>
- <element name="node">
- <ref name="unsignedInt"/>
- </element>
+ <optional>
+ <element name="node">
+ <ref name="unsignedInt"/>
+ </element>
+ </optional>
</interleave>
</element>
</define>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 514bd8a..a5ab29c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12520,11 +12520,15 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
int ret = -1;
xmlNodePtr save = ctxt->node;
ctxt->node = node;
+ int rv;
- if (virXPathInt("string(./node)", ctxt, &def->targetNode) < 0
||
- def->targetNode < 0) {
+ /* initialize to value which marks that the user didn't speify it */
specify
This feels like a need for something like the Tristate{Bool|State}
structs... Or way to force optional fields to something specific
+ def->targetNode = -1;
+
+ if ((rv = virXPathInt("string(./node)", ctxt, &def->targetNode)) ==
-2 ||
+ (rv == 0 && def->targetNode < 0)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("invalid or missing value of memory device node"));
+ _("invalid value of memory device node"));
...value for memory...
goto cleanup;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a22e5ad..29fed1d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3591,6 +3591,13 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef
*mem,
return -1;
}
+ if (mem->targetNode == -1) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("target NUMA node needs to be specifed for memory
"
specified
Before an explicit ACK - a few adjustments I think would be beneficial.
John
+ "device"));
+ return -1;
+ }
+
if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
if (mem->info.addr.dimm.slot >= def->mem.memory_slots) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,