[PATCH 0/3] numa_conf: Deny other memory modes than 'restrictive' if a memnode is 'restrictive'
We already check that there's no <memnode mode='restrictive'/> when <memory mode='restrictive'/> is set. But we are missing the opposite check: there's <memory mode='restrictive'/> when there is <memnode mode='restrictive'/>. Michal Prívozník (3): virDomainNumatuneNodeSpecified: Fix const correctness numa_conf: Move memnode mode validation into virDomainNumaDefValidate() numa_conf: Deny other memory modes than 'restrictive' if a memnode is 'restrictive' src/conf/numa_conf.c | 26 ++++++++---- src/conf/numa_conf.h | 2 +- ...strictive-mode-err-mixed.x86_64-latest.err | 1 + ...une-memnode-restrictive-mode-err-mixed.xml | 41 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 5 files changed, 61 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.xml -- 2.39.3
The virDomainNumatuneNodeSpecified() function does not write into passed @numatune pointer, it just reads from it. Therefore, the argument should be const, which allows this function to be called from places where virDomainNuma is already const (e.g. domain validation code). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 2 +- src/conf/numa_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index be0c4572c5..dd4997c759 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -116,7 +116,7 @@ struct _virDomainNuma { bool -virDomainNumatuneNodeSpecified(virDomainNuma *numatune, +virDomainNumatuneNodeSpecified(const virDomainNuma *numatune, int cellid) { if (numatune && diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index bbb928abb2..f5a20315b6 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -210,7 +210,7 @@ int virDomainNumatuneSpecifiedMaxNode(virDomainNuma *numatune); bool virDomainNumatuneNodesetIsAvailable(virDomainNuma *numatune, virBitmap *auto_nodeset); -bool virDomainNumatuneNodeSpecified(virDomainNuma *numatune, +bool virDomainNumatuneNodeSpecified(const virDomainNuma *numatune, int cellid); int virDomainNumaDefParseXML(virDomainNuma *def, xmlXPathContextPtr ctxt); -- 2.39.3
When parsing a <memnode/> we also check whether the @mode argument fulfills some requirements wrt 'restrictive' mode. This is not the right place though. There's virDomainNumaDefValidate() which contains other checks. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index dd4997c759..6095139385 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -191,14 +191,6 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa, VIR_DOMAIN_NUMATUNE_MEM_STRICT) < 0) return -1; - if (numa->memory.mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE && - mem_node->mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'restrictive' mode is required in memnode element " - "when mode is 'restrictive' in memory element")); - return -1; - } - tmp = virXMLPropString(cur_node, "nodeset"); if (!tmp) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -1083,6 +1075,14 @@ virDomainNumaDefValidate(const virDomainNuma *def) const virDomainNumaNode *node = &def->mem_nodes[i]; g_autoptr(virBitmap) levelsSeen = virBitmapNew(0); + if (virDomainNumatuneNodeSpecified(def, i) && + def->memory.mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE && + node->mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'restrictive' mode is required in memnode element when mode is 'restrictive' in memory element")); + return -1; + } + for (j = 0; j < node->ncaches; j++) { const virNumaCache *cache = &node->caches[j]; -- 2.39.3
We already do check that if there's <memory mode='restrictive'/> then all <memnode/> have to be of 'restrictive' mode too. But what we are missing the reverse: if there is <memnode/> with 'restrictive' mode, then the <memory/> has to be of the same mode too. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2208946 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 20 ++++++--- ...strictive-mode-err-mixed.x86_64-latest.err | 1 + ...une-memnode-restrictive-mode-err-mixed.xml | 41 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.xml diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 6095139385..a616521763 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -1075,12 +1075,20 @@ virDomainNumaDefValidate(const virDomainNuma *def) const virDomainNumaNode *node = &def->mem_nodes[i]; g_autoptr(virBitmap) levelsSeen = virBitmapNew(0); - if (virDomainNumatuneNodeSpecified(def, i) && - def->memory.mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE && - node->mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'restrictive' mode is required in memnode element when mode is 'restrictive' in memory element")); - return -1; + if (virDomainNumatuneNodeSpecified(def, i)) { + if (def->memory.mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE && + node->mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'restrictive' mode is required in memnode element when mode is 'restrictive' in memory element")); + return -1; + } + + if (node->mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE && + def->memory.mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'restrictive' mode is required in memory element when mode is 'restrictive' in memnode element")); + return -1; + } } for (j = 0; j < node->ncaches; j++) { diff --git a/tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.x86_64-latest.err b/tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.x86_64-latest.err new file mode 100644 index 0000000000..88f02ee70e --- /dev/null +++ b/tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.x86_64-latest.err @@ -0,0 +1 @@ +XML error: 'restrictive' mode is required in memory element when mode is 'restrictive' in memnode element diff --git a/tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.xml b/tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.xml new file mode 100644 index 0000000000..0304dec8ef --- /dev/null +++ b/tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.xml @@ -0,0 +1,41 @@ +<domain type='qemu'> + <name>QEMUGuest</name> + <uuid>9f4b6512-e73a-4a25-93e8-5307802821ce</uuid> + <memory unit='KiB'>24682468</memory> + <currentMemory unit='KiB'>24682468</currentMemory> + <vcpu placement='static'>32</vcpu> + <numatune> + <memory mode='interleave' nodeset='0-3'/> + <memnode cellid='0' mode='strict' nodeset='3'/> + <memnode cellid='2' mode='restrictive' nodeset='1-2'/> + </numatune> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + <numa> + <cell id='0' cpus='0' memory='20002' unit='KiB'/> + <cell id='1' cpus='1-27,29' memory='660066' unit='KiB'/> + <cell id='2' cpus='28,30-31' memory='24002400' unit='KiB'/> + </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> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 93d5ae018f..2cc9bd074a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1868,6 +1868,7 @@ mymain(void) DO_TEST_CAPS_LATEST("numatune-memnode"); DO_TEST_PARSE_ERROR_NOCAPS("numatune-memnode-invalid-mode"); DO_TEST_CAPS_LATEST("numatune-memnode-restrictive-mode"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("numatune-memnode-restrictive-mode-err-mixed"); DO_TEST_CAPS_LATEST("numatune-system-memory"); DO_TEST_NOCAPS("numatune-memnode-no-memory"); -- 2.39.3
On Mon, May 22, 2023 at 11:53:45AM +0200, Michal Privoznik wrote:
We already check that there's no <memnode mode='restrictive'/> when <memory mode='restrictive'/> is set. But we are missing the opposite check: there's <memory mode='restrictive'/> when there is <memnode mode='restrictive'/>.
Even though the logic is inverted in here it is correct both in the patch and in the commit messages, so for the series: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Michal Prívozník (3): virDomainNumatuneNodeSpecified: Fix const correctness numa_conf: Move memnode mode validation into virDomainNumaDefValidate() numa_conf: Deny other memory modes than 'restrictive' if a memnode is 'restrictive'
src/conf/numa_conf.c | 26 ++++++++---- src/conf/numa_conf.h | 2 +- ...strictive-mode-err-mixed.x86_64-latest.err | 1 + ...une-memnode-restrictive-mode-err-mixed.xml | 41 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 5 files changed, 61 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/numatune-memnode-restrictive-mode-err-mixed.xml
-- 2.39.3
participants (2)
-
Martin Kletzander -
Michal Privoznik