[RFC 0/1] virxml: Accept 'default' for virTristate* properties
Sending this as an RFC because it's incomplete. After virXMLPropTristate*() had been introduced, existing code was gradually converted to use the new helpers; however, if you look for example at 593140dabd66 you'll see that the original implementation considered encountering 'default' to be an error, and the change I'm proposing would relax such a check. So we need to go through the callers one by one, adapting them as necessary (in this case, we would pass VIR_XML_PROP_NONZERO in addition to existing flags) to ensure that the original semantic is preserved. Any better ideas? Andrea Bolognani (1): virxml: Accept 'default' for virTristate* properties src/util/virxml.c | 4 ---- 1 file changed, 4 deletions(-) -- 2.35.1
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 Signed-off-by: Andrea Bolognani <abologna@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); } -- 2.35.1
On Wed, Mar 23, 2022 at 07:04:20PM +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.
I had the same issue, but did not get around sending a patch for it. What I assume is that allowReboot is one of the few, if not the only exception where we format the default zero value.
Fixes: 8861d96c880d25c940456c5997a2ac93fc073c78 Fixes: c8726ede83ac117cb18c0b0a1fbfeeac8b80384b Signed-off-by: Andrea Bolognani <abologna@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; -
I would rather change this flag to something like VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this flag making the callers be able to opt in for this behaviour rather then all the others having to opt out. Honestly I do not understand the flag as it is currently because it does completely nothing when these functions just add the flag in unconditionally.
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); } -- 2.35.1
On Thu, Mar 24, 2022 at 09:58:28AM +0100, Martin Kletzander wrote:
What I assume is that allowReboot is one of the few, if not the only exception where we format the default zero value.
My guess is that you're right, but I haven't actually verified this yet :)
@@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node, virXMLPropFlags flags, virTristateBool *result) { - flags |= VIR_XML_PROP_NONZERO; -
I would rather change this flag to something like VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this flag making the callers be able to opt in for this behaviour rather then all the others having to opt out.
Yeah, this sounds better from the caller's point of view, but it would require adding a check to make sure that only one of VIR_XML_PROP_NONZERO and VIR_XML_PROP_ALLOW_ZERO has been passed. I'll see how clunky that looks.
Honestly I do not understand the flag as it is currently because it does completely nothing when these functions just add the flag in unconditionally.
There are other helpers, such as virXMLPropUInt(), that don't unconditionally set the flag. For all those, the behavior is up to the caller, and having the flag makes sense. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Mar 24, 2022 at 10:32:51AM +0000, Andrea Bolognani wrote:
On Thu, Mar 24, 2022 at 09:58:28AM +0100, Martin Kletzander wrote:
What I assume is that allowReboot is one of the few, if not the only exception where we format the default zero value.
My guess is that you're right, but I haven't actually verified this yet :)
@@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node, virXMLPropFlags flags, virTristateBool *result) { - flags |= VIR_XML_PROP_NONZERO; -
I would rather change this flag to something like VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this flag making the callers be able to opt in for this behaviour rather then all the others having to opt out.
Yeah, this sounds better from the caller's point of view, but it would require adding a check to make sure that only one of VIR_XML_PROP_NONZERO and VIR_XML_PROP_ALLOW_ZERO has been passed. I'll see how clunky that looks.
I meant only keeping the new one, although I must admit I completely missed the fact that it is used properly somewhere else. If we went that way then the flag might itself be a tristate enum "default, allow_zero, non_zero" where default is different for numbers and enums, but that seems even clunkier and I don't like that myself.
Honestly I do not understand the flag as it is currently because it does completely nothing when these functions just add the flag in unconditionally.
There are other helpers, such as virXMLPropUInt(), that don't unconditionally set the flag. For all those, the behavior is up to the caller, and having the flag makes sense.
It looks like the issue stems from the fact that we are trying to have a very "universal" approach to parsing the properties while at the same time having very different usage of enum and number values. virXMLPropUInt() and virXMLPropEnum() don't even evaluate the flags in the same function they would both bubble to. Maybe we could use the fact that some integer properties default to -1 and some to 0 and make the default explicit... I don't know, it all seems like overcomplicating to me. In any case I feel like we want not to parse default enum values (not only booleans and switches) and going through all the callers seems very tedious. And the opposite seems to be the case for numbers. But I just did a quick check in the code, nothing thorough.
-- Andrea Bolognani / Red Hat / Virtualization
On Thu, Mar 24, 2022 at 12:02:05 +0100, Martin Kletzander wrote:
On Thu, Mar 24, 2022 at 10:32:51AM +0000, Andrea Bolognani wrote:
On Thu, Mar 24, 2022 at 09:58:28AM +0100, Martin Kletzander wrote:
What I assume is that allowReboot is one of the few, if not the only exception where we format the default zero value.
My guess is that you're right, but I haven't actually verified this yet :)
@@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node, virXMLPropFlags flags, virTristateBool *result) { - flags |= VIR_XML_PROP_NONZERO; -
I would rather change this flag to something like VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this flag making the callers be able to opt in for this behaviour rather then all the others having to opt out.
Yeah, this sounds better from the caller's point of view, but it would require adding a check to make sure that only one of VIR_XML_PROP_NONZERO and VIR_XML_PROP_ALLOW_ZERO has been passed. I'll see how clunky that looks.
I meant only keeping the new one, although I must admit I completely missed the fact that it is used properly somewhere else. If we went that way then the flag might itself be a tristate enum "default, allow_zero, non_zero" where default is different for numbers and enums, but that seems even clunkier and I don't like that myself.
Alternatively introduce a new helper e.g. virXMLPropTristateBoolDefault which will allow also the 'default' value to be specified explicitly. In such case we can document the difference in the comment and don't have to touch every other usage.
On Thu, Mar 24, 2022 at 12:42:12PM +0100, Peter Krempa wrote:
On Thu, Mar 24, 2022 at 12:02:05 +0100, Martin Kletzander wrote:
On Thu, Mar 24, 2022 at 10:32:51AM +0000, Andrea Bolognani wrote:
On Thu, Mar 24, 2022 at 09:58:28AM +0100, Martin Kletzander wrote:
I would rather change this flag to something like VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this flag making the callers be able to opt in for this behaviour rather then all the others having to opt out.
Yeah, this sounds better from the caller's point of view, but it would require adding a check to make sure that only one of VIR_XML_PROP_NONZERO and VIR_XML_PROP_ALLOW_ZERO has been passed. I'll see how clunky that looks.
I meant only keeping the new one, although I must admit I completely missed the fact that it is used properly somewhere else. If we went that way then the flag might itself be a tristate enum "default, allow_zero, non_zero" where default is different for numbers and enums, but that seems even clunkier and I don't like that myself.
Alternatively introduce a new helper e.g. virXMLPropTristateBoolDefault which will allow also the 'default' value to be specified explicitly.
In such case we can document the difference in the comment and don't have to touch every other usage.
Yeah, I thought of that approach too and it sounds like it would be the least terrible one. I'm going to give it a try and report back :) Thanks for all your input so far! -- Andrea Bolognani / Red Hat / Virtualization
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 Signed-off-by: Andrea Bolognani <abologna@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.
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@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
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
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.
participants (4)
-
Andrea Bolognani -
Martin Kletzander -
Michal Prívozník -
Peter Krempa