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

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 :)