src/qemu/qemu_domain.c | 52 +++++++++++-------- .../qemustatusxml2xmldata/vcpus-multi-in.xml | 2 +- 2 files changed, 31 insertions(+), 23 deletions(-)
The use of the parseOpaque parameter was mistakenly removed in
commit 4a4132b4625778cf80acb9c92d06351b44468ac3
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Tue Dec 3 10:49:49 2019 +0000
conf: don't use passed in caps in post parse method
causing the method to re-fetch qemuCaps that were already just
fetched and put into parseOpaque.
This is inefficient when parsing incoming XML, but for live
XML this is more serious as it means we use the capabilities
for the current QEMU binary on disk, rather than the running
QEMU.
That commit, however, did have a useful side effect of fixing
a crasher bug in the qemu post parse callback introduced by
commit 5e939cea896fb3373a6f68f86e325c657429ed3d
Author: Jiri Denemark <jdenemar@redhat.com>
Date: Thu Sep 26 18:42:02 2019 +0200
qemu: Store default CPU in domain XML
The qemuDomainDefSetDefaultCPU() method in that patch did not
allow for the possibility that qemuCaps would be NULL and thus
resulted in a SEGV.
This shows a risk in letting each check in the post parse
callback look for qemuCaps == NULL. The safer option is to
check once upfront and immediately stop (postpone) further
validation.
Honouring the cached caps for the live status XML, highlights
a second flaw, which is that it shouldn't check the virt
type or arch for running guests. The info needed to check this
is not in the cache caps, only the list of flags are populated.
Thus some of the post parse logic is made to only run for
inactive guests. This showed a bug in one test data file which
lacked an ID attribute for the live guest.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
src/qemu/qemu_domain.c | 52 +++++++++++--------
.../qemustatusxml2xmldata/vcpus-multi-in.xml | 2 +-
2 files changed, 31 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6f53e17b6a..2abe93c829 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4703,16 +4703,17 @@ static int
qemuDomainDefPostParse(virDomainDefPtr def,
unsigned int parseFlags,
void *opaque,
- void *parseOpaque G_GNUC_UNUSED)
+ void *parseOpaque)
{
virQEMUDriverPtr driver = opaque;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
- g_autoptr(virQEMUCaps) qemuCaps = NULL;
+ /* Note that qemuCaps may be NULL when this function is called. This
+ * function shall not fail in that case. It will be re-run on VM startup
+ * with the capabilities populated. */
+ virQEMUCapsPtr qemuCaps = parseOpaque;
- if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
- def->emulator))) {
+ if (!qemuCaps)
return 1;
- }
if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -4721,18 +4722,31 @@ qemuDomainDefPostParse(virDomainDefPtr def,
return -1;
}
- if (!virQEMUCapsIsArchSupported(qemuCaps, def->os.arch)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Emulator '%s' does not support arch '%s'"),
- def->emulator, virArchToString(def->os.arch));
- return -1;
- }
+ if (def->id == -1) {
+ if (!virQEMUCapsIsArchSupported(qemuCaps, def->os.arch)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Emulator '%s' does not support arch '%s'"),
+ def->emulator, virArchToString(def->os.arch));
+ return -1;
+ }
- if (!virQEMUCapsIsVirtTypeSupported(qemuCaps, def->virtType)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Emulator '%s' does not support virt type '%s'"),
- def->emulator, virDomainVirtTypeToString(def->virtType));
- return -1;
+ if (!virQEMUCapsIsVirtTypeSupported(qemuCaps, def->virtType)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Emulator '%s' does not support virt type '%s'"),
+ def->emulator, virDomainVirtTypeToString(def->virtType));
+ return -1;
+ }
+ if (!def->os.machine) {
+ const char *machine = virQEMUCapsGetPreferredMachine(qemuCaps,
+ def->virtType);
+ def->os.machine = g_strdup(machine);
+ }
+ } else {
+ if (!def->os.machine) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing machine type"));
+ return -1;
+ }
}
if (def->os.bootloader || def->os.bootloaderArgs) {
@@ -4741,12 +4755,6 @@ qemuDomainDefPostParse(virDomainDefPtr def,
return -1;
}
- if (!def->os.machine) {
- const char *machine = virQEMUCapsGetPreferredMachine(qemuCaps,
- def->virtType);
- def->os.machine = g_strdup(machine);
- }
-
qemuDomainNVRAMPathGenerate(cfg, def);
if (qemuDomainDefAddDefaultDevices(def, qemuCaps) < 0)
diff --git a/tests/qemustatusxml2xmldata/vcpus-multi-in.xml b/tests/qemustatusxml2xmldata/vcpus-multi-in.xml
index 11ec74ecf8..be48c55fe0 100644
--- a/tests/qemustatusxml2xmldata/vcpus-multi-in.xml
+++ b/tests/qemustatusxml2xmldata/vcpus-multi-in.xml
@@ -310,7 +310,7 @@
<allowReboot value='yes'/>
<blockjobs active='no'/>
<agentTimeout>-2</agentTimeout>
- <domain type='kvm'>
+ <domain type='kvm' id='1729'>
<name>QEMUGuest1</name>
<uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
<memory unit='KiB'>219100</memory>
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 12/10/19 7:43 AM, Daniel P. Berrangé wrote:
> The use of the parseOpaque parameter was mistakenly removed in
>
> commit 4a4132b4625778cf80acb9c92d06351b44468ac3
> Author: Daniel P. Berrangé <berrange@redhat.com>
> Date: Tue Dec 3 10:49:49 2019 +0000
>
> conf: don't use passed in caps in post parse method
>
> causing the method to re-fetch qemuCaps that were already just
> fetched and put into parseOpaque.
>
> This is inefficient when parsing incoming XML, but for live
> XML this is more serious as it means we use the capabilities
> for the current QEMU binary on disk, rather than the running
> QEMU.
>
> That commit, however, did have a useful side effect of fixing
> a crasher bug in the qemu post parse callback introduced by
>
> commit 5e939cea896fb3373a6f68f86e325c657429ed3d
> Author: Jiri Denemark <jdenemar@redhat.com>
> Date: Thu Sep 26 18:42:02 2019 +0200
>
> qemu: Store default CPU in domain XML
>
> The qemuDomainDefSetDefaultCPU() method in that patch did not
> allow for the possibility that qemuCaps would be NULL and thus
> resulted in a SEGV.
>
> This shows a risk in letting each check in the post parse
> callback look for qemuCaps == NULL. The safer option is to
> check once upfront and immediately stop (postpone) further
> validation.
>
> Honouring the cached caps for the live status XML, highlights
> a second flaw, which is that it shouldn't check the virt
> type or arch for running guests. The info needed to check this
> is not in the cache caps, only the list of flags are populated.
>
> Thus some of the post parse logic is made to only run for
> inactive guests. This showed a bug in one test data file which
> lacked an ID attribute for the live guest.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> src/qemu/qemu_domain.c | 52 +++++++++++--------
> .../qemustatusxml2xmldata/vcpus-multi-in.xml | 2 +-
> 2 files changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6f53e17b6a..2abe93c829 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4703,16 +4703,17 @@ static int
> qemuDomainDefPostParse(virDomainDefPtr def,
> unsigned int parseFlags,
> void *opaque,
> - void *parseOpaque G_GNUC_UNUSED)
> + void *parseOpaque)
> {
> virQEMUDriverPtr driver = opaque;
> g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> - g_autoptr(virQEMUCaps) qemuCaps = NULL;
> + /* Note that qemuCaps may be NULL when this function is called. This
> + * function shall not fail in that case. It will be re-run on VM startup
> + * with the capabilities populated. */
> + virQEMUCapsPtr qemuCaps = parseOpaque;
>
> - if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
> - def->emulator))) {
> + if (!qemuCaps)
> return 1;
> - }
>
ACK to this bit, but move the comment to be above the if (!qemuCaps) IMO
> if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -4721,18 +4722,31 @@ qemuDomainDefPostParse(virDomainDefPtr def,
> return -1;
> }
>
> - if (!virQEMUCapsIsArchSupported(qemuCaps, def->os.arch)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Emulator '%s' does not support arch '%s'"),
> - def->emulator, virArchToString(def->os.arch));
> - return -1;
> - }
> + if (def->id == -1) {
> + if (!virQEMUCapsIsArchSupported(qemuCaps, def->os.arch)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Emulator '%s' does not support arch '%s'"),
> + def->emulator, virArchToString(def->os.arch));
> + return -1;
> + }
>
> - if (!virQEMUCapsIsVirtTypeSupported(qemuCaps, def->virtType)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Emulator '%s' does not support virt type '%s'"),
> - def->emulator, virDomainVirtTypeToString(def->virtType));
> - return -1;
> + if (!virQEMUCapsIsVirtTypeSupported(qemuCaps, def->virtType)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Emulator '%s' does not support virt type '%s'"),
> + def->emulator, virDomainVirtTypeToString(def->virtType));
> + return -1;
> + }
> + if (!def->os.machine) {
> + const char *machine = virQEMUCapsGetPreferredMachine(qemuCaps,
> + def->virtType);
> + def->os.machine = g_strdup(machine);
> + }
> + } else {
> + if (!def->os.machine) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing machine type"));
> + return -1;
> + }
> }
>
Most of this seems like it should go into the Validate path, so it will
be skipped for cases we need to be protective, per
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE usage.
The def->os.machine check needs to be kept here in some capacity, also
to protect later code. So maybe move that condition to the top and have
it return 1. Or maybe it is moved into qemuDomainDefPostParseBasic which
is where the default emulator is set?
Adding active VM handling into PostParse seems tricky, but if it's
needed maybe it should be based on PARSE_INACTIVE rather than the
def->id check
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.