On Tue, Dec 10, 2019 at 10:57:44AM +0000, Daniel P. Berrangé wrote:
On Mon, Dec 09, 2019 at 01:21:05PM -0500, Cole Robinson wrote:
On 12/4/19 9:20 AM, Daniel P. Berrangé wrote:
The XML parser currently calls virCapabilitiesDomainDataLookup during parsing to find the domain capabilities matching the triple
(virt type, os type, arch)
This is, however, bogus with the QEMU driver as it assumes that there is an emulator known to the default driver capabilities that matches this triple. It is entirely possible for the driver to be parsing an XML file with a custom emulator path specified pointing to a binary that doesn't exist in the default driver capabilities. This will, for example be the case on a RHEL host which only installs the host native emulator to /usr/bin. The user can have built a custom QEMU for non-native arches into $HOME and wish to use that.
Aside from validation, this call is also used to fill in a machine type for the guest if not otherwise specified. Again, this data may be incorrect for the QEMU driver because it is not taking account of the emulator binary that is referenced.
To start fixing this, move the validation to the post-parse callbacks where more intelligent driver specific logic can be applied.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/bhyve/bhyve_domain.c | 7 ++++++- src/conf/capabilities.c | 17 +++++++++++++++++ src/conf/capabilities.h | 7 +++++++ src/conf/domain_conf.c | 19 ++----------------- src/libvirt_private.syms | 1 + src/libxl/libxl_domain.c | 7 ++++++- src/lxc/lxc_domain.c | 5 +++++ src/openvz/openvz_conf.c | 7 ++++++- src/phyp/phyp_driver.c | 9 +++++++-- src/qemu/qemu_domain.c | 19 +++++++++++++++---- src/vmware/vmware_driver.c | 9 +++++++-- src/vmx/vmx.c | 9 +++++++-- src/vz/vz_driver.c | 7 ++++++- 13 files changed, 92 insertions(+), 31 deletions(-)
diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 7d24bb602f..575f141b53 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -74,11 +74,16 @@ bhyveDomainDefNeedsISAController(virDomainDefPtr def)
static int bhyveDomainDefPostParse(virDomainDefPtr def, - virCapsPtr caps G_GNUC_UNUSED, + virCapsPtr caps, unsigned int parseFlags G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (!virCapabilitiesDomainSupported(caps, def->os.type, + def->os.arch, + def->virtType)) + return -1; + /* Add an implicit PCI root controller */ if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index ff7d265621..748dd64273 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -816,6 +816,23 @@ virCapabilitiesDomainDataLookup(virCapsPtr caps, }
+bool +virCapabilitiesDomainSupported(virCapsPtr caps, + int ostype, + virArch arch, + int virttype) +{ + g_autofree virCapsDomainDataPtr capsdata = NULL; + + capsdata = virCapabilitiesDomainDataLookup(caps, ostype, + arch, + virttype, + NULL, NULL); + + return capsdata != NULL; +} + + int virCapabilitiesAddStoragePool(virCapsPtr caps, int poolType) diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 8a7137d7eb..c39fe0de08 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -309,6 +309,13 @@ virCapabilitiesDomainDataLookup(virCapsPtr caps, const char *emulator, const char *machinetype);
+bool +virCapabilitiesDomainSupported(virCapsPtr caps, + int ostype, + virArch arch, + int domaintype); + + void virCapabilitiesClearHostNUMACellCPUTopology(virCapsHostNUMACellCPUPtr cpu, size_t ncpus); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1c2b8f26ed..6abe15f721 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19565,14 +19565,11 @@ virDomainCachetuneDefParse(virDomainDefPtr def, static int virDomainDefParseCaps(virDomainDefPtr def, xmlXPathContextPtr ctxt, - virDomainXMLOptionPtr xmlopt, - virCapsPtr caps, - unsigned int flags) + virDomainXMLOptionPtr xmlopt) { g_autofree char *virttype = NULL; g_autofree char *arch = NULL; g_autofree char *ostype = NULL; - g_autofree virCapsDomainDataPtr capsdata = NULL;
virttype = virXPathString("string(./@type)", ctxt); ostype = virXPathString("string(./os/type[1])", ctxt); @@ -19633,18 +19630,6 @@ virDomainDefParseCaps(virDomainDefPtr def, def->os.arch = virArchFromHost(); }
- if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, - def->os.arch, - def->virtType, - NULL, NULL))) { - if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)) - return -1; - virResetLastError(); - } else { - if (!def->os.machine) - def->os.machine = g_strdup(capsdata->machinetype); - } - return 0; }
This series is a move in the right direction IMO, there's a couple issues though.
We drop the VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE protection here, by moving the equivalent into the PostParse callback and not the Validate callback. This reintroduces the recurring problem of VMs disappearing on libvirt restart, if host capabilities change.
There is VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL, which has different behavior, all these error checks would need to 'return 1', but I think drivers need specially handling for this so I'm not sure if it's a simple change like that.
Oh, I was mislead in my reading of qemuDomainDefPostParse, as I saw that it already returned error when os.machine is NULL, but I didn't realize we'd never even be calling qemuDomainDefPostParse in that case, as the virDomainDefParseCaps() willl have caused it to be skipped.
On looking again I realized that I did in fact know about this problem already, but I mistakenly fixed it in a later patch in this series: 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 that patch was a bit stupid though as it refetched the qemu caps. On the plus side it accidentally fixed a segv with a patch from a few weeks ago that review missed. I've sent a small fix for the immediate stupid part of my commit above. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|