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