On Mon, 2018-02-12 at 16:59 -0500, John Ferlan wrote:
> > @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
> > tmp = virXMLPropString(nodes[i], "driver");
> > if (tmp) {
> > int value = virDomainIOAPICTypeFromString(tmp);
> > - if (value < 0) {
> > + if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
>
> "none" wouldn't be legal XML according to "iopic" in domaincommon.rng. I
> think this is where things get tricky - I'm fine with the changes as is,
> but that interaction between domain_conf xml parsing and the rng schema
> gets me wondering about whether that "none" value should be in the rng.
> Perhaps there's another opinion on this that may want to pipe in now...
"none" should definitely *not* be in the schema, since it's not
a valid value. The problem here is that schema compliance is not
enforced by libvirt: when using virsh, you are given the option
to simply ignore schema validation failures and go ahead with
defining/modifying the guest, so the only way to actually make
sure invalid values don't make it into the virDomainDef is to do
something like the above.
I mean, it's not like accepting "none" in the parser would be a
big issue - it would just be ignored anyway. But by adding an
explicit check we can report a better error in the very unlikely
scenario where
<ioapic driver="none"/>
has been specified in the guest configuration, so I think it's
a good idea to have it there given how little effort it requires.
> > @@ -2352,9 +2353,10 @@ struct _virDomainDef {
> >
> > virDomainOSDef os;
> > char *emulator;
> > - /* These three options are of type virTristateSwitch,
> > - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
> > - * virDomainCapabilitiesPolicy */
> > + /* Most of the values in {kvm_,hyperv_,}features are of type
>
> {kvm_|hyperv_|caps_}features
>
> > + * virTristateSwitch, but there are exceptions: for example,
> > + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
>
> s/,//
>
> > + * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
>
> s/and so on/./
I see you ask for the comment to be updated in the next commit when
_HPT is changed as well, but it wasn't really my intention to list
all types there - we both know how that kind of comment can get out
of sync with reality very quickly :)
Another approach, which didn't make it to the list for some reason,
was to end the comment with
[...] and so on. See virDomainDefFeaturesCheckABIStability() for
more details.
That seems like a better way to handle the ever-changing nature of
libvirt than a comment, don't you think?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
