Re: [PATCH v3 1/3] numatune: add check for numatune nodeset range

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 29, 2014 at 08:33:32PM +0800, Chen Fan wrote:
For memnode in numatune element, the range of attribute 'nodeset'
was not validated. on my host maxnodes was 1, but when setting nodeset
to '0-2' or more, guest also started succuss. there probably was qemu's
bug too.


How about rewording this as:

There was no check for 'nodeset' attribute in numatune-related
elements.  This patch adds validation that any nodeset specified does
not exceed maximum host node.

Signed-off-by: Chen Fan <chen.fan.fnst@xxxxxxxxxxxxxx>
---
src/conf/numatune_conf.c                           | 28 +++++++++++++
src/conf/numatune_conf.h                           |  1 +
src/libvirt_private.syms                           |  2 +
src/qemu/qemu_command.c                            |  3 ++
src/qemu/qemu_command.h                            |  1 +
src/util/virbitmap.c                               | 49 ++++++++++++++++++++++
src/util/virbitmap.h                               |  3 ++
src/util/virnuma.c                                 | 33 +++++++++++++++
src/util/virnuma.h                                 |  2 +
...rgv-numatune-static-nodeset-exceed-hostnode.xml | 35 ++++++++++++++++
tests/qemuxml2argvmock.c                           |  9 ++++
tests/qemuxml2argvtest.c                           |  1 +
tests/virbitmaptest.c                              | 26 +++++++++++-
13 files changed, 192 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml

diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c
index 21d9a64..54f309a 100644
--- a/src/conf/numatune_conf.c
+++ b/src/conf/numatune_conf.c
@@ -612,3 +612,31 @@ virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune)

    return false;
}
+
+int
+virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune)
+{
+    int ret = -1;
+    virBitmapPtr nodemask = NULL;
+    size_t i;
+
+    if (!numatune)
+        return ret;
+
+    nodemask = virDomainNumatuneGetNodeset(numatune, NULL, -1);
+    if (nodemask) {
+        ret = virBitmapLastSetBit(nodemask, -1);
+    }
+    for (i = 0; i < numatune->nmem_nodes; i++) {

Well, you're using the advantage of accessible structure members here
(numatune->nmem_nodes), but using accessors around.  These particular
ones are useless here when you don't need any of the logic they
provide.

+        int bit = -1;
+        nodemask = virDomainNumatuneGetNodeset(numatune, NULL, i);
+        if (!nodemask)
+            continue;
+
+        bit = virBitmapLastSetBit(nodemask, -1);
+        if (bit > ret)
+            ret = bit;
+    }
+
+    return ret;
+}
diff --git a/src/conf/numatune_conf.h b/src/conf/numatune_conf.h
index 5254629..15dc0d6 100644
--- a/src/conf/numatune_conf.h
+++ b/src/conf/numatune_conf.h
@@ -102,4 +102,5 @@ bool virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune);

bool virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune);

+int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune);
#endif /* __NUMATUNE_CONF_H__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d63a8f0..4a30ad7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1011,6 +1011,7 @@ virBitmapFree;
virBitmapGetBit;
virBitmapIsAllClear;
virBitmapIsAllSet;
+virBitmapLastSetBit;
virBitmapNew;
virBitmapNewCopy;
virBitmapNewData;
@@ -1728,6 +1729,7 @@ virNumaGetPageInfo;
virNumaGetPages;
virNumaIsAvailable;
virNumaNodeIsAvailable;
+virNumaNodesetIsAvailable;
virNumaSetPagePoolSize;
virNumaSetupMemoryPolicy;

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2e5af4f..9757d3e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6663,6 +6663,9 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
        goto cleanup;
    }

+    if (!virNumaNodesetIsAvailable(def->numatune))
+        goto cleanup;
+
    for (i = 0; i < def->mem.nhugepages; i++) {
        ssize_t next_bit, pos = 0;

diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index aa40c9e..f263665 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -27,6 +27,7 @@
# include "domain_addr.h"
# include "domain_conf.h"
# include "vircommand.h"
+# include "virnuma.h"
# include "capabilities.h"
# include "qemu_conf.h"
# include "qemu_domain.h"
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index b6bd074..aed525a 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -651,6 +651,55 @@ virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos)
}

/**
+ * virBitmapLastSetBit:
+ * @bitmap: the bitmap
+ * @pos: the position after which to search for a set bit
+ *
+ * Search for the last set bit before position @pos in bitmap @bitmap.
+ * @pos can be -1 to search for the last set bit. Position starts
+ * at max_bit.
+ *
+ * Returns the position of the found bit, or -1 if no bit found.
+ */
+ssize_t
+virBitmapLastSetBit(virBitmapPtr bitmap, ssize_t pos)
+{
+    size_t nl;
+    size_t nb;
+    unsigned long bits;
+    size_t i;
+
+    if (pos < 0)
+        pos = bitmap->max_bit;
+
+    pos--;
+
+    if (pos >= bitmap->max_bit)
+        return -1;
+
+    nl = pos / VIR_BITMAP_BITS_PER_UNIT;
+    nb = pos % VIR_BITMAP_BITS_PER_UNIT;
+

You can use VIR_BITMAP_UNIT_OFFSET and VIR_BITMAP_BIT_OFFSET macros here.

+    bits = bitmap->map[nl] & ((1UL << (nb + 1)) - 1);
+

VIR_BITMAP_BIT(nb + 1) - 1

+    while (bits == 0 && --nl < bitmap->map_len) {
+        bits = bitmap->map[nl];
+    }
+

Reading this is weird, especially when you're using underflowing of
size_t, eww.  I think you made this unnecessarily complicated by
keeping "pos" attribute there.  If (and only if) you really need to
have @pos there, I rather suggest making this a wrapper using
virBitmapNextSetBit(), but I hope you don't.

+    if (bits == 0)
+        return -1;
+
+    i =  VIR_BITMAP_BITS_PER_UNIT - 1;
+    for (; i < VIR_BITMAP_BITS_PER_UNIT; i--) {
+        if (bits & 1UL << i) {
+            return i + nl * VIR_BITMAP_BITS_PER_UNIT;
+        }
+    }
+
+    return -1;
+}
+
+/**
 * virBitmapNextClearBit:
 * @bitmap: the bitmap
 * @pos: the position after which to search for a clear bit
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index 4493cc9..c0fad55 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -105,6 +105,9 @@ bool virBitmapIsAllClear(virBitmapPtr bitmap)
ssize_t virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos)
    ATTRIBUTE_NONNULL(1);

+ssize_t virBitmapLastSetBit(virBitmapPtr bitmap, ssize_t pos)
+    ATTRIBUTE_NONNULL(1);
+
ssize_t virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t pos)
    ATTRIBUTE_NONNULL(1);


Could you separate these virbitmap.* hunks and the tests for them into
their own patch, please?

diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 690615f..fbe8fd1 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -312,6 +312,33 @@ virNumaGetNodeCPUs(int node,

    return ret;
}
+
+bool
+virNumaNodesetIsAvailable(virDomainNumatunePtr numatune)
+{
+    int maxnode;
+    int bit;
+
+    if (!numatune)
+        return true;
+
+    if ((maxnode = virNumaGetMaxNode()) < 0)
+        return false;
+
+    maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES;
+
+    bit = virDomainNumatuneSpecifiedMaxNode(numatune);
+    if (bit > maxnode)
+        goto error;
+
+    return true;
+
+ error:
+    virReportError(VIR_ERR_INTERNAL_ERROR,
+                   _("NUMA node %d is out of range"), bit);
+    return false;
+}
+
# undef MASK_CPU_ISSET
# undef n_bits

@@ -373,6 +400,12 @@ virNumaGetNodeCPUs(int node ATTRIBUTE_UNUSED,
                   _("NUMA isn't available on this host"));
    return -1;
}
+
+bool
+virNumaNodesetIsAvailable(virDomainNumatunePtr numatune)
+{
+    return true;
+}
#endif


In what case would you like this to return true?


diff --git a/src/util/virnuma.h b/src/util/virnuma.h
index 04b6b8c..e129bb2 100644
--- a/src/util/virnuma.h
+++ b/src/util/virnuma.h
@@ -34,6 +34,8 @@ char *virNumaGetAutoPlacementAdvice(unsigned short vcups,
int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune,
                             virBitmapPtr nodemask);

+bool virNumaNodesetIsAvailable(virDomainNumatunePtr numatune);
+
bool virNumaIsAvailable(void);
int virNumaGetMaxNode(void);
bool virNumaNodeIsAvailable(int node);
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml
new file mode 100644
index 0000000..84a560a
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml
@@ -0,0 +1,35 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>1048576</memory>
+  <currentMemory unit='KiB'>1048576</currentMemory>
+  <vcpu placement='static'>4</vcpu>
+  <numatune>
+    <memnode cellid='0' mode='strict' nodeset='0-8'/>
+    <memnode cellid='1' mode='strict' nodeset='0'/>
+  </numatune>
+  <os>
+    <type arch='i686' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <cpu>
+    <numa>
+      <cell id='0' cpus='0-1' memory='524288'/>
+      <cell id='1' cpus='2-3' memory='524288'/>
+    </numa>
+  </cpu>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <disk type='block' device='disk'>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <controller type='ide' index='0'/>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index bff3b0f..316bf0b 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -21,6 +21,7 @@
#include <config.h>

#include "internal.h"
+#include "virnuma.h"
#include <time.h>

time_t time(time_t *t)
@@ -30,3 +31,11 @@ time_t time(time_t *t)
        *t = ret;
    return ret;
}
+
+int
+virNumaGetMaxNode(void)
+{
+   const int maxnodes = 7;
+
+   return maxnodes;
+}
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 0e9fab9..bab6d0d 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1250,6 +1250,7 @@ mymain(void)
    DO_TEST_FAILURE("numatune-memnode-no-memory", NONE);

    DO_TEST("numatune-auto-nodeset-invalid", NONE);
+    DO_TEST_FAILURE("numatune-static-nodeset-exceed-hostnode", QEMU_CAPS_OBJECT_MEMORY_RAM);
    DO_TEST_PARSE_ERROR("numatune-memnode-nocpu", NONE);
    DO_TEST_PARSE_ERROR("numatune-memnodes-problematic", NONE);
    DO_TEST("numad", NONE);
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index ea832ad..21e8611 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -171,7 +171,7 @@ test3(const void *data ATTRIBUTE_UNUSED)
    return ret;
}

-/* test for virBitmapNextSetBit, virBitmapNextClearBit */
+/* test for virBitmapNextSetBit, virBitmapLastSetBit, virBitmapNextClearBit */
static int
test4(const void *data ATTRIBUTE_UNUSED)
{
@@ -200,6 +200,9 @@ test4(const void *data ATTRIBUTE_UNUSED)
    if (virBitmapNextSetBit(bitmap, -1) != -1)
        goto error;

+    if (virBitmapLastSetBit(bitmap, -1) != -1)
+        goto error;
+
    for (i = 0; i < size; i++) {
        if (virBitmapNextClearBit(bitmap, i - 1) != i)
            goto error;
@@ -232,6 +235,18 @@ test4(const void *data ATTRIBUTE_UNUSED)
    if (virBitmapNextSetBit(bitmap, i) != -1)
        goto error;

+    j = sizeof(bitsPos)/sizeof(int) - 1;
+    i = -1;
+
+    while (j < ARRAY_CARDINALITY(bitsPos)) {
+        i = virBitmapLastSetBit(bitmap, i);
+        if (i != bitsPos[j--])
+            goto error;
+    }
+
+    if (virBitmapLastSetBit(bitmap, i) != -1)
+        goto error;
+
    j = 0;
    i = -1;

@@ -255,6 +270,15 @@ test4(const void *data ATTRIBUTE_UNUSED)
    if (virBitmapNextSetBit(bitmap, i) != -1)
        goto error;

+    for (i = size; i > 0; i--) {
+        if (virBitmapLastSetBit(bitmap, i) != i - 1)
+            goto error;
+    }
+
+    if (virBitmapLastSetBit(bitmap, i) != -1)
+        goto error;
+
+
    if (virBitmapNextClearBit(bitmap, -1) != -1)
        goto error;

--
1.9.3

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]