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
