1:04 p.m.
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
1:04 p.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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(a)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
3:58 a.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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(a)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
5:32 a.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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
6:02 a.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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
6:42 a.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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.
7:22 a.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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
4 a.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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(a)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.
4:14 a.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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(a)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
5:03 a.m.
New subject: [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 :)
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
6:37 a.m.
New subject: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
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.
974
days inactive
975
days old
10 comments
4 participants
participants (4)
-
Andrea Bolognani -
Martin Kletzander -
Michal Prívozník -
Peter Krempa