Re: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

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?