Re: [libvirt] [PATCH 06/30] conf: move virt type / os type / arch validation to post-parse

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(a)redhat.com&gt; > > --- > > 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.