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

Reply via email to