[RFC 1/1] virxml: Accept 'default' for virTristate* properties

[Andrea Bolognani]

On Thu, Mar 24, 2022 at 10:14:18AM +0100, Michal Prívozník wrote:
> On 3/24/22 10:00, Peter Krempa wrote:
> > On Wed, Mar 23, 2022 at 19:04:20 +0100, Andrea Bolognani wrote:
> >> Fixes: 8861d96c880d25c940456c5997a2ac93fc073c78
> >> Fixes: c8726ede83ac117cb18c0b0a1fbfeeac8b80384b
>
> I wouldn't say Fixes, because this is basically an RFE, not a bugfix.

But it *is* a bugfix: we have stopped accepting values that we
accepted in the past, and libvirtd has started logging spurious error
messages as a consequence of that.

I agree that the Fixes tags shown above are not accurate: they should
instead point to commits like 593140dabd66, which are the ones that
replaced an open-coded implementation that would accept 'default' as
a valid value with a call to an helper that doesn't. I need to spend
some time digging through the git history to come up with the actual
list though :)

> >> @@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
> >>                         virXMLPropFlags flags,
> >>                         virTristateBool *result)
> >>  {
> >> -    flags |= VIR_XML_PROP_NONZERO;
> >> -
> >>      return virXMLPropEnumInternal(node, name, virTristateBoolTypeFromString,
> >>                                    flags, result, VIR_TRISTATE_BOOL_ABSENT);
> >>  }
> >> @@ -573,8 +571,6 @@ virXMLPropTristateSwitch(xmlNodePtr node,
> >>                           virXMLPropFlags flags,
> >>                           virTristateSwitch *result)
> >>  {
> >> -    flags |= VIR_XML_PROP_NONZERO;
> >> -
> >>      return virXMLPropEnumInternal(node, name, virTristateSwitchTypeFromString,
> >>                                    flags, result, VIR_TRISTATE_SWITCH_ABSENT);
> >>  }
> >
> > You can't do this without further consideration:
> >
> > At least the usage in 'virDomainDiskSourceNetworkParse' where 'tls'
> > attribute is parsed declared in the schema as 'virYesNo' which allows
> > only 'yes' and 'no'.
> >
> > If you want to do this you must fix all callers to pass
> > VIR_XML_PROP_NONZERO explicitly and then remove it from those you care
> > about.

Have you perhaps missed the cover letter? I posted this as RFC
specifically because I'm aware of the fact that I need to go through
all the commits that introduced a call to virXMLPropTristate*() and
check whether that changed the behavior compared to the existing
code. I just didn't feel like doing all that archeology work without
validating the approach first was a good idea :)

> Don't forget about RNG change. I don't see much value in doing this
> change without corresponding RNG update.

I don't think RNG changes are needed. I'm just trying to restore the
original behavior, which the RNG should already account for. I am
specifically not interested in adding more scenarios where 'default'
is formatted to XML: not formatting the attribute at all is clearly
the better choice, but there are some cases (such as allowReboot)
where we have historically formatted that value and so we also need
to be able to parse it back.

-- 
Andrea Bolognani / Red Hat / Virtualization