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.
> Another issue: other drivers use the machine= setting too, libxl at
> least. This appears to drop filling in a default machine type for them,
> at least at XML parse time.
I thought that was the case too, but I couln't find any functional use
of def->os.machine in any of the other drivers:
$ git grep os.machine | grep -v -E '(qemu|conf)/'
libvirt-domain.c: * "os.machine" - the machine hardware name as a string
libxl/libxl_conf.c: def->os.machine);
libxl/xen_common.c: def->os.machine = g_strdup(capsdata->machinetype);
vbox/vbox_common.c: VIR_DEBUG("def->os.machine %s", def->os.machine);
vz/vz_sdk.c: if (def->os.machine != NULL || def->os.bootmenu != 0 ||
that libxl_conf.c usage is just an error message
The xen_common.c usage is just when parsing XM/XL config files,
so output only.
The vz_sdk.c usage reports error for any non-NULL machine
I guess there is a difference in that if the user does dumpxml for a
libxl guest we've no longer filled in the machine. I can fix that,
but it doesn't look like it'll have a functional effect.
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
