On Wed, Nov 19, 2014 at 12:51:22PM +0000, Daniel P. Berrange wrote:
On Wed, Nov 19, 2014 at 09:45:39AM +0000, Daniel P. Berrange wrote:
On Wed, Nov 19, 2014 at 08:23:22AM +0100, Martin Kletzander wrote:
On Tue, Nov 18, 2014 at 05:59:47PM +0000, Daniel P. Berrange wrote:
This proof of concept patch extends the virDomainDefineXML and virDomainCreateXML APIs so that they can validate the user supplied XML document against the RNG schemas.
The virsh command will enable validation by default, it must be turned off with --skip-validation if desired.
This series is not complete
- The network, interface, storage pool, etc APIs are not wired up to support validation. - Only the QEMU virt driver is wired up to validate - The virsh edit command is not wired up to validate
It is enough to demonstrate it working with 'virsh define' and the QEMU driver though.
The biggest problem I see is the really awful error messages we get back from libxml2 when validation fails :-( They are essentially useless :-(
This is one of the things why I'm not convinced this work is worth it. It may be nice if we tell the user their XML is invalid instead of silently losing information. But error message similar to "invalid element in interleave" doesn't help much when you are adding 100-line XML. There are some better validators, but requiring those would be too cumbersome.
At least when using 'virsh edit' you would know what element you just changed / added. So if you got get a generic 'validation failed' error you have a pretty good idea of where in teh document you made the mistake. So I think it'd be useful in that scenario. The error reporting is more of a problem for the apps where they're passing in a big XML document to define the guest and basically anything could be wrong.
So, it seems not all of the error messages are so awful. It does a bad job of reporting unknown elements, but if you have an unknown attribute it does better:
"Invalid attribute foo for element name"
If you give an invalid value for an attribute which is an enum it is semi-usefull
"Element domain failed to validate attributes"
So this does seem somewhat more useful to have in libvirt
As I said, I'm not against this, I agree that it will still be useful. If you meant this as an RFC, then I misunderstood that and I should've just wrote that as an initial PoC it's fine with me :) Do you want me to finish the review? Martin