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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
