[libvirt] [PATCH 0/8] Add XML validation to the APIs

[Daniel P. Berrange]

On Thu, Nov 20, 2014 at 12:02:12PM +0100, Martin Kletzander wrote:
> 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?

Actually if you want to review patches 4, 5, 6, 7 that would be useful.
Those are general refactoring of the way we handle flags with the XML
parsers/formatters. The 7th patch was awful to create and will be a
rebase nightmare if we leave it too long.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|