[libvirt] [PATCH v2 0/5] qemu_domain, qemu_process: maxCpus check to non-x86 guests
v2: - original patch was split in 3 patches as John Ferlan requested; - patches 4 and 5 are cleanups; - first patch link: https://www.redhat.com/archives/libvir-list/2018-October/msg00251.html Daniel Henrique Barboza (5): qemu_process.c: adding maxCpus value to error message qemu_process.c: make qemuValidateCpuCount public qemu_domain.c: moving maxCpu validation to qemuDomainDefValidate qemu_process.c: moving qemuValidateCpuCount to qemu_domain.c qemu_process.c: removing qemuProcessStartValidateXML src/qemu/qemu_domain.c | 27 +++++++++++++++++++ src/qemu/qemu_process.c | 57 ++++------------------------------------- 2 files changed, 32 insertions(+), 52 deletions(-) -- 2.19.1
Adding the maxCpus value in the error message of qemuValidateCpuCount allows the user to set an acceptable maxCpus count without knowing QEMU internals. x86 guests, that might have been created prior to the x86 qemuDomainDefValidate maxCpus check code (that validates the maxCpus value in editing time), will also benefit from this change. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1850923914..84a2163743 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3900,8 +3900,9 @@ qemuValidateCpuCount(virDomainDefPtr def, } if (maxCpus > 0 && virDomainDefGetVcpusMax(def) > maxCpus) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Maximum CPUs greater than specified machine type limit")); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Maximum CPUs greater than specified machine " + "type limit %u"), maxCpus); return -1; } -- 2.19.1
qemuValidateCpuCount validates the maxCpus value of a domain at startup time, preventing it to start if the value exceeds a maximum. This checking is also done at qemu_domain.c, qemuDomainDefValidate. However, it is done only for x86 (and even then, in a specific scenario). We want this check to be done for all archs. To accomplish this, let's first make qemuValidateCpuCount public so it can be used inside qemuDomainDefValidate. The function was renamed to qemuProcessValidateCpuCount to be compliant with the other public methods at qemu_process.h. The method signature was slightly adapted to fit the const 'def' variable used in qemuDomainDefValidate. This change has no downside in in its original usage at qemuProcessStartValidateXML. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 8 ++++---- src/qemu/qemu_process.h | 3 +++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 84a2163743..4d134bd7be 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3887,9 +3887,9 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, } -static int -qemuValidateCpuCount(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps) +int +qemuProcessValidateCpuCount(const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { unsigned int maxCpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, def->os.machine); @@ -5152,7 +5152,7 @@ qemuProcessStartValidateXML(virQEMUDriverPtr driver, * If back compat isn't a concern, XML validation should probably * be done at parse time. */ - if (qemuValidateCpuCount(vm->def, qemuCaps) < 0) + if (qemuProcessValidateCpuCount(vm->def, qemuCaps) < 0) return -1; /* checks below should not be executed when starting a qemu process for a diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2037467c94..d3b2baac51 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -47,6 +47,9 @@ int qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem); +int qemuProcessValidateCpuCount(const virDomainDef *def, + virQEMUCapsPtr qemuCaps); + void qemuProcessReconnectAll(virQEMUDriverPtr driver); typedef struct _qemuProcessIncomingDef qemuProcessIncomingDef; -- 2.19.1
Adding maxCpu validation in qemuDomainDefValidate allows the user to spot over the board maxCpus counts at editing time, instead of facing a runtime error when starting the domain. This check is also arch independent. This leaves us with 2 calls to qemuProcessValidateCpuCount: one in qemuProcessStartValidateXML and the new one at qemuDomainDefValidate. The call in qemuProcessStartValidateXML is redundant. Following up in that code, there is a call to virDomainDefValidate, which in turn will call config.domainValidateCallback. In this case, the callback function is qemuDomainDefValidate. This means that, on startup time, qemuProcessValidateCpuCount will be called twice. To avoid that, let's also remove the qemuProcessValidateCpuCount call from qemuProcessStartValidateXML. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 4 ++++ src/qemu/qemu_process.c | 14 +------------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 37926850b2..3b86d28216 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4084,6 +4084,10 @@ qemuDomainDefValidate(const virDomainDef *def, } } + if (qemuProcessValidateCpuCount(def, qemuCaps) < 0) { + goto cleanup; + } + if (ARCH_IS_X86(def->os.arch) && virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM) { if (!qemuDomainIsQ35(def)) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4d134bd7be..2adbf375ee 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5140,21 +5140,9 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm, static int qemuProcessStartValidateXML(virQEMUDriverPtr driver, virDomainObjPtr vm, - virQEMUCapsPtr qemuCaps, virCapsPtr caps, unsigned int flags) { - /* The bits we validate here are XML configs that we previously - * accepted. We reject them at VM startup time rather than parse - * time so that pre-existing VMs aren't rejected and dropped from - * the VM list when libvirt is updated. - * - * If back compat isn't a concern, XML validation should probably - * be done at parse time. - */ - if (qemuProcessValidateCpuCount(vm->def, qemuCaps) < 0) - return -1; - /* checks below should not be executed when starting a qemu process for a * VM that was running before (migration, snapshots, save). It's more * important to start such VM than keep the configuration clean */ @@ -5204,7 +5192,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, } - if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0) + if (qemuProcessStartValidateXML(driver, vm, caps, flags) < 0) return -1; if (qemuProcessStartValidateGraphics(vm) < 0) -- 2.19.1
On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
Adding maxCpu validation in qemuDomainDefValidate allows the user to spot over the board maxCpus counts at editing time, instead of facing a runtime error when starting the domain. This check is also arch independent.
This leaves us with 2 calls to qemuProcessValidateCpuCount: one in qemuProcessStartValidateXML and the new one at qemuDomainDefValidate.
The call in qemuProcessStartValidateXML is redundant. Following up in that code, there is a call to virDomainDefValidate, which in turn will call config.domainValidateCallback. In this case, the callback function is qemuDomainDefValidate. This means that, on startup time, qemuProcessValidateCpuCount will be called twice.
To avoid that, let's also remove the qemuProcessValidateCpuCount call from qemuProcessStartValidateXML.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 4 ++++ src/qemu/qemu_process.c | 14 +------------- 2 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 37926850b2..3b86d28216 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4084,6 +4084,10 @@ qemuDomainDefValidate(const virDomainDef *def, } }
+ if (qemuProcessValidateCpuCount(def, qemuCaps) < 0) { + goto cleanup; + } +
make check would have told you to remove the { ... } for the one line goto cleanup; I'll take care of it for you (as well as the merge conflict it creates in the subsequent patch). John
if (ARCH_IS_X86(def->os.arch) && virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM) { if (!qemuDomainIsQ35(def)) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4d134bd7be..2adbf375ee 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5140,21 +5140,9 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm, static int qemuProcessStartValidateXML(virQEMUDriverPtr driver, virDomainObjPtr vm, - virQEMUCapsPtr qemuCaps, virCapsPtr caps, unsigned int flags) { - /* The bits we validate here are XML configs that we previously - * accepted. We reject them at VM startup time rather than parse - * time so that pre-existing VMs aren't rejected and dropped from - * the VM list when libvirt is updated. - * - * If back compat isn't a concern, XML validation should probably - * be done at parse time. - */ - if (qemuProcessValidateCpuCount(vm->def, qemuCaps) < 0) - return -1; - /* checks below should not be executed when starting a qemu process for a * VM that was running before (migration, snapshots, save). It's more * important to start such VM than keep the configuration clean */ @@ -5204,7 +5192,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
}
- if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0) + if (qemuProcessStartValidateXML(driver, vm, caps, flags) < 0) return -1;
if (qemuProcessStartValidateGraphics(vm) < 0)
On 11/15/18 7:38 PM, John Ferlan wrote:
On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
Adding maxCpu validation in qemuDomainDefValidate allows the user to spot over the board maxCpus counts at editing time, instead of facing a runtime error when starting the domain. This check is also arch independent.
This leaves us with 2 calls to qemuProcessValidateCpuCount: one in qemuProcessStartValidateXML and the new one at qemuDomainDefValidate.
The call in qemuProcessStartValidateXML is redundant. Following up in that code, there is a call to virDomainDefValidate, which in turn will call config.domainValidateCallback. In this case, the callback function is qemuDomainDefValidate. This means that, on startup time, qemuProcessValidateCpuCount will be called twice.
To avoid that, let's also remove the qemuProcessValidateCpuCount call from qemuProcessStartValidateXML.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 4 ++++ src/qemu/qemu_process.c | 14 +------------- 2 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 37926850b2..3b86d28216 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4084,6 +4084,10 @@ qemuDomainDefValidate(const virDomainDef *def, } }
+ if (qemuProcessValidateCpuCount(def, qemuCaps) < 0) { + goto cleanup; + } + make check would have told you to remove the { ... } for the one line goto cleanup;
I'll take care of it for you (as well as the merge conflict it creates in the subsequent patch).
I appreciate it. I'll make sure to execute 'make check' before sending the patch next time. Thanks, Daniel
John
if (ARCH_IS_X86(def->os.arch) && virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM) { if (!qemuDomainIsQ35(def)) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4d134bd7be..2adbf375ee 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5140,21 +5140,9 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm, static int qemuProcessStartValidateXML(virQEMUDriverPtr driver, virDomainObjPtr vm, - virQEMUCapsPtr qemuCaps, virCapsPtr caps, unsigned int flags) { - /* The bits we validate here are XML configs that we previously - * accepted. We reject them at VM startup time rather than parse - * time so that pre-existing VMs aren't rejected and dropped from - * the VM list when libvirt is updated. - * - * If back compat isn't a concern, XML validation should probably - * be done at parse time. - */ - if (qemuProcessValidateCpuCount(vm->def, qemuCaps) < 0) - return -1; - /* checks below should not be executed when starting a qemu process for a * VM that was running before (migration, snapshots, save). It's more * important to start such VM than keep the configuration clean */ @@ -5204,7 +5192,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
}
- if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0) + if (qemuProcessStartValidateXML(driver, vm, caps, flags) < 0) return -1;
if (qemuProcessStartValidateGraphics(vm) < 0)
Previous patch removed the call to qemuProcessValidateCpuCount from qemuProcessStartValidateXML, in qemu_process.c. The only caller left is qemuDomainDefValidate, in qemu_domain.c. Instead of having a public function declared inside qemu_process.c that isn't used in that file, this patch moves the function to qemu_domain.c, making in static and renaming it to qemuDomainValidateCpuCount to be compliant with other static functions names in the file. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 25 ++++++++++++++++++++++++- src/qemu/qemu_process.c | 23 ----------------------- src/qemu/qemu_process.h | 3 --- 3 files changed, 24 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3b86d28216..a0d69da646 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3990,6 +3990,29 @@ qemuDomainDefValidateMemory(const virDomainDef *def) } +static int +qemuDomainValidateCpuCount(const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + unsigned int maxCpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, def->os.machine); + + if (virDomainDefGetVcpus(def) == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Domain requires at least 1 vCPU")); + return -1; + } + + if (maxCpus > 0 && virDomainDefGetVcpusMax(def) > maxCpus) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Maximum CPUs greater than specified machine " + "type limit %u"), maxCpus); + return -1; + } + + return 0; +} + + static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, @@ -4084,7 +4107,7 @@ qemuDomainDefValidate(const virDomainDef *def, } } - if (qemuProcessValidateCpuCount(def, qemuCaps) < 0) { + if (qemuDomainValidateCpuCount(def, qemuCaps) < 0) { goto cleanup; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2adbf375ee..0d1dc7ed35 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3887,29 +3887,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, } -int -qemuProcessValidateCpuCount(const virDomainDef *def, - virQEMUCapsPtr qemuCaps) -{ - unsigned int maxCpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, def->os.machine); - - if (virDomainDefGetVcpus(def) == 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Domain requires at least 1 vCPU")); - return -1; - } - - if (maxCpus > 0 && virDomainDefGetVcpusMax(def) > maxCpus) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Maximum CPUs greater than specified machine " - "type limit %u"), maxCpus); - return -1; - } - - return 0; -} - - static int qemuProcessVerifyHypervFeatures(virDomainDefPtr def, virCPUDataPtr cpu) diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index d3b2baac51..2037467c94 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -47,9 +47,6 @@ int qemuProcessDestroyMemoryBackingPath(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem); -int qemuProcessValidateCpuCount(const virDomainDef *def, - virQEMUCapsPtr qemuCaps); - void qemuProcessReconnectAll(virQEMUDriverPtr driver); typedef struct _qemuProcessIncomingDef qemuProcessIncomingDef; -- 2.19.1
Commit ("qemu_domain.c: moving maxCpu validation to qemuDomainDefValidate") shortened the code of qemuProcessStartValidateXML. The function is called only by qemuProcessStartValidate, in the same file, and its code is now a single check that calls virDomainDefValidate. Instead of leaving a function call just to execute a single check, this patch puts the check in the body of qemuProcessStartValidate in the place where qemuProcessStartValidateXML was being called. The function can now be removed. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0d1dc7ed35..c9ef2050a1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5114,23 +5114,6 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm, } -static int -qemuProcessStartValidateXML(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virCapsPtr caps, - unsigned int flags) -{ - /* checks below should not be executed when starting a qemu process for a - * VM that was running before (migration, snapshots, save). It's more - * important to start such VM than keep the configuration clean */ - if ((flags & VIR_QEMU_PROCESS_START_NEW) && - virDomainDefValidate(vm->def, caps, 0, driver->xmlopt) < 0) - return -1; - - return 0; -} - - /** * qemuProcessStartValidate: * @vm: domain object @@ -5169,7 +5152,11 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, } - if (qemuProcessStartValidateXML(driver, vm, caps, flags) < 0) + /* Checks below should not be executed when starting a qemu process for a + * VM that was running before (migration, snapshots, save). It's more + * important to start such VM than keep the configuration clean */ + if ((flags & VIR_QEMU_PROCESS_START_NEW) && + virDomainDefValidate(vm->def, caps, 0, driver->xmlopt) < 0) return -1; if (qemuProcessStartValidateGraphics(vm) < 0) -- 2.19.1
On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
v2: - original patch was split in 3 patches as John Ferlan requested; - patches 4 and 5 are cleanups; - first patch link: https://www.redhat.com/archives/libvir-list/2018-October/msg00251.html
Daniel Henrique Barboza (5): qemu_process.c: adding maxCpus value to error message qemu_process.c: make qemuValidateCpuCount public qemu_domain.c: moving maxCpu validation to qemuDomainDefValidate qemu_process.c: moving qemuValidateCpuCount to qemu_domain.c qemu_process.c: removing qemuProcessStartValidateXML
src/qemu/qemu_domain.c | 27 +++++++++++++++++++ src/qemu/qemu_process.c | 57 ++++------------------------------------- 2 files changed, 32 insertions(+), 52 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> (series) and pushed. Congrats on your first libvirt patches. John
On 11/15/18 7:40 PM, John Ferlan wrote:
On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
v2: - original patch was split in 3 patches as John Ferlan requested; - patches 4 and 5 are cleanups; - first patch link: https://www.redhat.com/archives/libvir-list/2018-October/msg00251.html
Daniel Henrique Barboza (5): qemu_process.c: adding maxCpus value to error message qemu_process.c: make qemuValidateCpuCount public qemu_domain.c: moving maxCpu validation to qemuDomainDefValidate qemu_process.c: moving qemuValidateCpuCount to qemu_domain.c qemu_process.c: removing qemuProcessStartValidateXML
src/qemu/qemu_domain.c | 27 +++++++++++++++++++ src/qemu/qemu_process.c | 57 ++++------------------------------------- 2 files changed, 32 insertions(+), 52 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> (series) and pushed.
Congrats on your first libvirt patches.
Thanks! Looking forward for more! Daniel
John
participants (2)
-
Daniel Henrique Barboza -
John Ferlan