[libvirt] [PATCH 08/10] conf: Prepare making memory device target node optional

[John Ferlan]

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,
>