[libvirt] [PATCH v10 08/19] backup: Parse and output checkpoint XML

[Daniel P. Berrangé]

On Wed, Jul 24, 2019 at 06:17:24PM +0200, Peter Krempa wrote:
> On Wed, Jul 24, 2019 at 17:11:19 +0100, Daniel Berrange wrote:
> > On Wed, Jul 24, 2019 at 12:55:58AM -0500, Eric Blake wrote:
> > > Add a new file checkpoint_conf.c that performs the translation to and
> > > from new XML describing a checkpoint. The code shares a common base
> > > class with snapshots, since a checkpoint similarly represents the
> > > domain state at a moment in time. Add some basic testing of round trip
> > > XML handling through the new code.
> > > 
> > > Of note - this code intentionally differs from snapshots in that XML
> > > schema validation is unconditional, rather than based on a public API
> > > flag.
> > 
> > I'm in two minds as to whether this is really a good idea.
> > 
> > I always have a strong tendancy to prefer consistency across the APIs
> > we expose, which would point to an opt-in flag for validation. If we
> > did later find we need a way to skip validation, then we end up having
> > to add a NO_VALIDATION API flag, which makes inconsitency even more
> > glaring to app devs
> > 
> > I understand that this can help catch mistakes in apps, but I'm not
> > sold on it as a default, when it is easy for apps to opt-in if they
> > want to.
> 
> I've specifically requested this behaviour as I don't think that there's
> value in not validating the XML. The XML parser is tied to what we parse
> anyways and the checks done when parsing are _WAY_ simpler if we know
> that all the values conform to the schema.

If there is no downside to validation though, why only do it for one
schema. We should do it for all of the XML parsers we have. We didn't
do this originally because we didn't have confidence in accuracy of
the schemas, which was shown a sensible decision many times over as
we found a great many schema bugs.

None the less, it would still be possible to turn on validation for
all our schemas, as simply declare the current _VALIDATE flags are
now a no-op.

There is the possibility that apps are lazy in creating XML
documents adding elements that may not be supported with the version
of libvirt they are talking to. That might be caught at parsing time
or might be silently ignored with little ill effect. So enforcing
validation would mean apps have to be more careful with the XML they
feed libvirt. Such enforcement could be considered a good thing even
if it risks highlighting existing broken behaviour in apps.

> Also if we'd ever want to do a generated parser, we will need to
> strictly adhere to the schema first. Consistency is great as long as it
> does not hinder progress. And in this case I think that validating
> everything right away is progress.

If & when we attempt generated parsers, we still have the option
to enforce schema validation at that time, so I don't see this as
blocking.

So overall, either we should turn on validation for all our schemas,
or we should require  a VALIDATE flag for this new API. Both avoid
special case behaviour with the checkpoint APIs.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|