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

[Eric Blake]

On 7/24/19 5:42 PM, Eric Blake wrote:

> 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?

Thinking just a bit more: for the case of defining a new checkpoint,
mandatory schema validation is still appealing (the new RNG file is
pretty small, and easy enough to compare to the code in
checkpoint_conf.c).  But for the case of the
VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE flag, which includes a full
<domain> sub-element, there _is_ a risk that we still have
inconsistencies between the domain XML we produce and what the schema
parser will require during the REDEFINE (because we don't have schema
validation always enabled for domains in general, and because <domain>
is so much of a bigger beast to begin with).

The last thing we need is for restarting libvirtd to fail to reload all
checkpoints merely because of a schema failure on the <domain>
subelement.  I may still end up implementing a VALIDATE flag (maybe just
at the checkpoint_conf.c level for internal use, rather than as a public
API flag) that is a no-op for checkpoint creation but allows bypass of
the validation of a <domain> sub-element during REDEFINE (and I would
actually code it up that way too: try to validate the XML as-is; if it
fails, then edit a copy of the XML to remove the <domain> element and
try to revalidate the rest, rather than skipping validation altogether -
so that the rest of the checkpoint code can still rely on validation
rather than reinstating redundant checking code).  But it is definitely
material for a separate patch:

> 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190726/06187051/attachment-0001.sig>