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?
--
Andrea Bolognani / Red Hat / Virtualization