Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virbitmap.h | 3 +++ tests/virbitmaptest.c | 13 ++++++++++++- 4 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d63a8f0..1e2bc0a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1011,6 +1011,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy; virBitmapNewData; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b6bd074..3e7269e 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -651,6 +651,51 @@ virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) } /** + * virBitmapLastSetBit: + * @bitmap: the bitmap + * + * Search for the last set bit in bitmap @bitmap. + * + * Returns the position of the found bit, or -1 if no bit found.

+ */ +ssize_t +virBitmapLastSetBit(virBitmapPtr bitmap) +{ + ssize_t i; + int unusedBits; + ssize_t sz; + unsigned long bits; + + unusedBits = bitmap->map_len * VIR_BITMAP_BITS_PER_UNIT - bitmap->max_bit; + + sz = bitmap->map_len - 1; + if (unusedBits > 0) { + bits = bitmap->map[sz] & (VIR_BITMAP_BIT(VIR_BITMAP_BITS_PER_UNIT - unusedBits) - 1); + if (bits != 0) + goto found; + + sz--; + } + + for (; sz >= 0; sz--) { + bits = bitmap->map[sz]; + if (bits != 0) + goto found; + } + + if (bits == 0) + return -1; + +found:

On Thu, Oct 30, 2014 at 01:44:18PM +0800, Chen Fan wrote: >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 | 1 + > src/qemu/qemu_command.c | 4 +++ > src/util/virnuma.c | 38 ++++++++++++++++++++++ > src/util/virnuma.h | 1 + > ...rgv-numatune-static-nodeset-exceed-hostnode.xml | 36 ++++++++++++++++++++ > tests/qemuxml2argvmock.c | 9 +++++ > tests/qemuxml2argvtest.c | 1 + > 9 files changed, 119 insertions(+) > 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..6986739 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; >+ int bit; >+ >+ if (!numatune) >+ return ret; >+ >+ nodemask = virDomainNumatuneGetNodeset(numatune, NULL, -1); >+ if (nodemask) >+ ret = virBitmapLastSetBit(nodemask); >+ >+ for (i = 0; i < numatune->nmem_nodes; i++) { >+ nodemask = numatune->mem_nodes[i].nodeset; >+ if (!nodemask) >+ continue; >+ >+ bit = virBitmapLastSetBit(nodemask); >+ 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 1e2bc0a..4a30ad7 100644 >--- a/src/libvirt_private.syms >+++ b/src/libvirt_private.syms >@@ -1729,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..d2c5f0c 100644 >--- a/src/qemu/qemu_command.c >+++ b/src/qemu/qemu_command.c >@@ -53,6 +53,7 @@ > #include "virstoragefile.h" > #include "virtpm.h" > #include "virscsi.h" >+#include "virnuma.h" > #if defined(__linux__) > # include <linux/capability.h> > #endif >@@ -6663,6 +6664,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/util/virnuma.c b/src/util/virnuma.c >index 690615f..4188ef5 100644 >--- a/src/util/virnuma.c >+++ b/src/util/virnuma.c >@@ -165,6 +165,33 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, > return ret; > } > >+bool >+virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) >+{ >+ int maxnode; >+ int bit; >+ >+ if (!numatune) >+ return true; >+ >+ bit = virDomainNumatuneSpecifiedMaxNode(numatune); >+ if (bit == -1) (bit < 0) would go with the rest of the code, the functions can be then modified to report various kinds of errors more easily. >+ return true; >+ >+ if ((maxnode = virNumaGetMaxNode()) < 0) >+ return false; >+ >+ maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; >+ if (bit > maxnode) >+ goto error; >+ >+ return true; >+ >+ error: >+ virReportError(VIR_ERR_INTERNAL_ERROR, >+ _("NUMA node %d is out of range"), bit); >+ return false; >+} > > bool > virNumaIsAvailable(void) >@@ -330,6 +357,17 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, > return 0; > } > >+bool >+virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) >+{ >+ if (virDomainNumatuneSpecifiedMaxNode(numatune) != -1) { similarly " >= 0" here. >+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >+ _("libvirt is compiled without NUMA tuning support")); >+ return false; >+ } >+ >+ return true; >+} > > bool > virNumaIsAvailable(void) >diff --git a/src/util/virnuma.h b/src/util/virnuma.h >index 04b6b8c..5bb37b8 100644 >--- a/src/util/virnuma.h >+++ b/src/util/virnuma.h >@@ -34,6 +34,7 @@ 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..b7564fe >--- /dev/null >+++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml >@@ -0,0 +1,36 @@ >+<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> >+ Empty line at EOF. "make syntax-check" would find that for you ;) >diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c >index bff3b0f..7218747 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 maxnodesNum = 7; >+ >+ return maxnodesNum; >+} Why not just "return 7;" ???

>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); Very long line. > DO_TEST_PARSE_ERROR("numatune-memnode-nocpu", NONE); > DO_TEST_PARSE_ERROR("numatune-memnodes-problematic", NONE); > DO_TEST("numad", NONE); >-- >1.9.3 >

On Thu, Oct 30, 2014 at 01:44:19PM +0800, Chen Fan wrote: >Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; >--- > src/util/virnuma.c | 23 ++++------------------- > 1 file changed, 4 insertions(+), 19 deletions(-) > >diff --git a/src/util/virnuma.c b/src/util/virnuma.c >index 4188ef5..613a43c 100644 >--- a/src/util/virnuma.c >+++ b/src/util/virnuma.c >@@ -95,31 +95,19 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, > int ret = -1; > int bit = 0; > size_t i; >- int maxnode = 0; > virBitmapPtr tmp_nodemask = NULL; > >+ if (!virNumaNodesetIsAvailable(numatune)) >+ return -1; >+ > tmp_nodemask = virDomainNumatuneGetNodeset(numatune, nodemask, -1); > if (!tmp_nodemask) > return 0; > >- if (numa_available() < 0) { >- virReportError(VIR_ERR_INTERNAL_ERROR, >- "%s", _("Host kernel is not aware of NUMA.")); >- return -1; >- } >- >- maxnode = numa_max_node(); >- maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; >- > /* Convert nodemask to NUMA bitmask. */ > nodemask_zero(&mask); > bit = -1; > while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) >= 0) { >- if (bit > maxnode) { >- virReportError(VIR_ERR_INTERNAL_ERROR, >- _("NUMA node %d is out of range"), bit); >- return -1; >- } > nodemask_set(&mask, bit); > } > I think this is safe, "numad" returning nodeset that's not on the host would be weird error and it is easy to find in the logs.

>@@ -347,10 +335,7 @@ int > virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, > virBitmapPtr nodemask ATTRIBUTE_UNUSED) > { >- if (virDomainNumatuneGetNodeset(numatune, NULL, -1)) { >- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >- _("libvirt is compiled without NUMA tuning support")); >- >+ if (!virNumaNodesetIsAvailable(numatune)) { > return -1; > } This makes the square brackets obsolete.