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

[Peter Krempa]

On Thu, Mar 24, 2022 at 10:03:36 +0000, Andrea Bolognani wrote:
> 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 :)

I did skip the cover letter at first. Your example though doesn't show
that you wanted to fix all the callers first, because patch like this
just breaks stuff.

Ideally with the flag removal you update _all_ callers to pass the flag
explicitly and remove the flag in a subsequent path which will be the
real bugfix.

Here the commit + cover letter looks like a "I want to fix my thing and
don't care about the rest" type of commit.