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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
