[libvirt] [PATCH v3 1/3] numatune: add check for numatune nodeset range
Martin Kletzander
mkletzan at redhat.com
Wed Oct 29 13:20:33 UTC 2014
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 at cn.fujitsu.com>
>---
> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141029/7d274027/attachment-0001.sig>
More information about the libvir-list
mailing list