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