[libvirt] [PATCH 2/2] qemu: Figure out nodeset bitmap size correctly

[Peter Krempa]

On Thu, Apr 12, 2018 at 08:47:58 +0200, Andrea Bolognani wrote:
> The current private XML parsing code relies on the assumption
> that NUMA node IDs start from 0 and are densely allocated,
> neither of which is necessarily the case.
> 
> Change it so that the bitmap size is dynamically calculated by
> looking at NUMA node IDs instead, which ensures all nodes will
> be able to fit and thus the bitmap will be parsed successfully.
> 
> Update one of the test cases so that it would fail with the
> previous approach, but passes with the new one.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1490158

While the patch below will fix this case, I'd also like to see that the
parsing of the bitmaps is made non-fatal. If the nodesets are missing
some of the reported data will be wrong, but not having this is
certainly not a deal-breaker so that we should not reconnect to qemu.

> 
> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> ---
>  src/qemu/qemu_domain.c                    | 11 ++++++++++-
>  tests/qemustatusxml2xmldata/modern-in.xml |  2 +-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 100304fd05..b126c69490 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2248,6 +2248,8 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
>      virCapsPtr caps = NULL;
>      char *nodeset;
>      char *cpuset;
> +    int nodesetSize = 0;
> +    int i;
>      int ret = -1;
>  
>      nodeset = virXPathString("string(./numad/@nodeset)", ctxt);
> @@ -2259,8 +2261,15 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt,
>      if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>          goto cleanup;
>  
> +    /* Figure out how big the nodeset bitmap needs to be.
> +     * This is necessary because NUMA node IDs are not guaranteed to
> +     * start from 0 or be densely allocated */
> +    for (i = 0; i < caps->host.nnumaCell; i++) {
> +        nodesetSize = MAX(nodesetSize, caps->host.numaCell[i]->num + 1);
> +    }

Syntax-check is moaning about this.

> +
>      if (nodeset &&
> -        virBitmapParse(nodeset, &priv->autoNodeset, caps->host.nnumaCell_max) < 0)
> +        virBitmapParse(nodeset, &priv->autoNodeset, nodesetSize) < 0)
>          goto cleanup;
>  
>      if (cpuset) {

ACK if you actually run syntax-check.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180419/cd42c77a/attachment-0001.sig>