Re: [libvirt] [PATCH 06/30] conf: move virt type / os type / arch validation to post-parse
On Tue, Dec 10, 2019 at 11:32:18AM -0500, Cole Robinson wrote:
> On 12/10/19 5:57 AM, Daniel P. Berrangé wrote:
> >
> >> 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.
>
> Check grep for 'xenfv' and 'xenpv', domaincapabilities does some machine
> value handling, and virt-manager will use the reported VM machine value
> and feed it back to domaincapabilities, so it could be used in a round
> about way.
Yes, I can see the dom capabilities reports them, and apps can of
coure put them in the XML they feed in, but AFAICT the libxl driver
will not do anything with the values provided when defining/starting
a guest
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/30] conf: move virt type / os type / arch validation to post-parse
On 12/10/19 5:57 AM, Daniel P. Berrangé wrote:
>
>> 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.
Check grep for 'xenfv' and 'xenpv', domaincapabilities does some machine
value handling, and virt-manager will use the reported VM machine value
and feed it back to domaincapabilities, so it could be used in a round
about way.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/30] conf: move virt type / os type / arch validation to post-parse
On Tue, Dec 10, 2019 at 10:57:44AM +, 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é
> > > ---
> > > 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
> > >
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é
> > ---
> > 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
Re: [libvirt] [PATCH 06/30] conf: move virt type / os type / arch validation to post-parse
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é
> ---
> 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
[libvirt] [PATCH 06/30] conf: move virt type / os type / arch validation to post-parse
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é
---
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)",
