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

[John Ferlan]

On 02/13/2018 04:40 AM, Andrea Bolognani wrote:
> 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?
> 

That's fine... I guess since you started listing them I figured adding
another in the next patch was "natural".

How about this (or something close to it):

"Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to
handle support. A few assign specific data values to the option. See
virDomainDefFeaturesCheckABIStability for details."

John