Re: [libvirt] [PATCH v4 2/3] numatune: add check for numatune nodeset range

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 >