5:42 p.m.
On 7/24/19 11:40 AM, Daniel P. Berrangé wrote:
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.
The opt-in argument allowing us to skip something if we run into an RNG
bug is slightly tempting, but
>
> 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.
the amount of simplifications when you can rely on the data already
being validated was rather substantial.
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.
Yes, I'd love to add more validation to more of our existing legacy
APIs, but one project at a time. Maybe a libvirt.conf or qemu.conf
switch to opt in to validation (and ignore the flags, where flags exist)
is worthwhile (then, if you do run into a schema bug, you can
temporarily weaken qemu.conf and rely on the flags on a per-API basis).
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.
It would also be possible to add a _VALIDATE flag for checkpoints that
is initially treated as a no-op (ie. I perform the validation using the
RNG schema no matter what, regardless of the flag's presence, because it
makes the rest of my code shorter), but if we run into an RNG schema bug
down the road, we could then still have the position of omitting the
flag to bypass validation at that time. Then again, if we run into an
RNG bug where we reject something that looks like it should be valid, an
older libvirt will fail to parse the XML no matter what, then by the
time you upgrade to a version of libvirt that honors the flag to allow a
bypass, you've also upgraded to a version of libvirt that fixes the bug,
so why do you need the bypass?
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.
Existing APIs are produced by existing apps (and yes, they have been
lazy, where strict validation would probably break some apps). But
checkpoints are a brand-new API, and as there are no existing apps
producing checkpoint XML (only new stuff), now is a great time to
enforce that the new stuff is sane from the get-go.
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.
There's still time to add it in prior to the 5.6 release if we decide we
need the opt-out capability. I'm probably going to commit this patch
as-is while we continue the discussion, where we can decide if a
follow-up patch needs to make it opt-in rather than mandatory.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org