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

[Michal Prívozník]

On 3/24/22 10:00, Peter Krempa wrote:
> On Wed, Mar 23, 2022 at 19:04:20 +0100, Andrea Bolognani wrote:
>> The _ABSENT value of each enumeration has 'default' as string
>> representation, and when that's been formatted to XML we should
>> parse it back successfully, so we can't just treat encountering
>> it as an error.
>>
>> Callers of virXMLPropTristate*() can of course still pass
>> VIR_XML_PROP_NONZERO explicitly to the helpers if the current
>> behavior is the one they want.
>>
>> After this change, libvirtd no longer logs
>>
>>   error : virXMLPropEnumInternal:516 : XML error: Invalid value
>>   for attribute 'value' in element 'allowReboot': 'default'.
>>
>> when it gets restarted while there are running guests.
>>
>> Fixes: 8861d96c880d25c940456c5997a2ac93fc073c78
>> Fixes: c8726ede83ac117cb18c0b0a1fbfeeac8b80384b

I wouldn't say Fixes, because this is basically an RFE, not a bugfix.

>> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
>> ---
>>  src/util/virxml.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/src/util/virxml.c b/src/util/virxml.c
>> index 8ff59e7cda..db5212de20 100644
>> --- a/src/util/virxml.c
>> +++ b/src/util/virxml.c
>> @@ -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.
> 

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

Michal