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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]