Re: [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
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)