On Wed, 2014-10-29 at 14:20 +0100, Martin Kletzander wrote:
> 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.
Look good to me.
>
> >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.
right, I should use numatune->mem_nodes[i].nodeset directly.
>
> >+ 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.
I will delete the "pos" attribute.
>
> >+ 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?
Sure.
>
> >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?
when libvirt does not support numa, we can not check it.
maybe we should return false if setting nodeset.
Thanks,
Chen
>
> >
> >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
> >
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list