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

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(a)cn.fujitsu.com&gt; >--- > 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 >