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

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.