[libvirt] storage: inconsistent in 'format' scheme between document and implementation
Hi, I've found a problem that a 'format' element in storage-{pool,vol} XML doesn't behave as described in the document. The document says that a format of {pool,vol} is specified as the value of a format element, like: <target> <path>/var/lib/virt/images/sparse.img</path> <format>qcow2</format> <permissions> However, the implementation doesn't follow this: if (options->formatFromString) { char *format = virXPathString(conn, "string(/volume/target/format/@type)", ctxt); if (format == NULL) ret->target.format = options->defaultFormat; else ret->target.format = (options->formatFromString)(format); if (ret->target.format < 0) { virStorageReportError(conn, VIR_ERR_XML_ERROR, _("unknown volume format type %s"), format); VIR_FREE(format); goto cleanup; } VIR_FREE(format); } The implementation assumes that a format of {pool,vol} is specified as the attribute of a format element, like: <target> <path>/var/lib/virt/images/sparse.img</path> <format type='qcow2' /> <permissions> Thus, we need to fix either the document or the implementation. (I guess the implementation is correct, right?) Thanks, ozaki-r
On Thu, May 21, 2009 at 03:57:28PM +0900, Ryota Ozaki wrote:
Hi,
I've found a problem that a 'format' element in storage-{pool,vol} XML doesn't behave as described in the document.
The document says that a format of {pool,vol} is specified as the value of a format element, like:
<target> <path>/var/lib/virt/images/sparse.img</path> <format>qcow2</format> <permissions>
However, the implementation doesn't follow this:
if (options->formatFromString) { char *format = virXPathString(conn, "string(/volume/target/format/@type)", ctxt); [...] The implementation assumes that a format of {pool,vol} is specified as the attribute of a format element, like:
<target> <path>/var/lib/virt/images/sparse.img</path> <format type='qcow2' /> <permissions>
Thus, we need to fix either the document or the implementation. (I guess the implementation is correct, right?)
Right, moreover the schemas docs/schemas/storagepool.rng makes it clear too it's a type attribute: ... <define name='sourcefmtfs'> <optional> <element name='format'> <attribute name='type'> <choice> <value>auto</value> <value>ext2</value> ... I have updated the doc to clear this, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, May 21, 2009 at 5:01 PM, Daniel Veillard <veillard@redhat.com> wrote:
On Thu, May 21, 2009 at 03:57:28PM +0900, Ryota Ozaki wrote:
Hi,
I've found a problem that a 'format' element in storage-{pool,vol} XML doesn't behave as described in the document.
The document says that a format of {pool,vol} is specified as the value of a format element, like:
<target> <path>/var/lib/virt/images/sparse.img</path> <format>qcow2</format> <permissions>
However, the implementation doesn't follow this:
if (options->formatFromString) { char *format = virXPathString(conn, "string(/volume/target/format/@type)", ctxt); [...] The implementation assumes that a format of {pool,vol} is specified as the attribute of a format element, like:
<target> <path>/var/lib/virt/images/sparse.img</path> <format type='qcow2' /> <permissions>
Thus, we need to fix either the document or the implementation. (I guess the implementation is correct, right?)
Right, moreover the schemas docs/schemas/storagepool.rng makes it clear too it's a type attribute:
Yes, I also checked that. BTW, is the schema file used by libvirtd or drivers? This is just interest. Thanks, ozaki-r
... <define name='sourcefmtfs'> <optional> <element name='format'> <attribute name='type'> <choice> <value>auto</value> <value>ext2</value> ...
I have updated the doc to clear this,
thanks !
Daniel
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, May 21, 2009 at 05:25:35PM +0900, Ryota Ozaki wrote:
Yes, I also checked that.
BTW, is the schema file used by libvirtd or drivers? This is just interest.
We don't use them in the libvirt code themselves (though it would be relatively trivial to ask libxml2 to validate instances with them), but some are used byt the test suite. I'm not sure the storage schemas are used though, but we try to keep them up to date when the XML input and output code is updated. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, May 21, 2009 at 10:46:11AM +0200, Daniel Veillard wrote:
On Thu, May 21, 2009 at 05:25:35PM +0900, Ryota Ozaki wrote:
Yes, I also checked that.
BTW, is the schema file used by libvirtd or drivers? This is just interest.
We don't use them in the libvirt code themselves (though it would be relatively trivial to ask libxml2 to validate instances with them), but some are used byt the test suite. I'm not sure the storage schemas are used though, but we try to keep them up to date when the XML input and output code is updated.
I think it could be nice to add a flag to the various DefineXML/CreateXML methods VIR_DOMAIN_XML_VALIDATE, to tell the parser that the app wants strict validation. We shouldn't turn on validation by default though, because that makes back compatability harder for applications, because they have to make sure never to use new XML attributes/elements with old libvirt installs. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, May 21, 2009 at 7:05 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, May 21, 2009 at 10:46:11AM +0200, Daniel Veillard wrote:
On Thu, May 21, 2009 at 05:25:35PM +0900, Ryota Ozaki wrote:
Yes, I also checked that.
BTW, is the schema file used by libvirtd or drivers? This is just interest.
We don't use them in the libvirt code themselves (though it would be relatively trivial to ask libxml2 to validate instances with them), but some are used byt the test suite. I'm not sure the storage schemas are used though, but we try to keep them up to date when the XML input and output code is updated.
I think it could be nice to add a flag to the various DefineXML/CreateXML methods VIR_DOMAIN_XML_VALIDATE, to tell the parser that the app wants strict validation.
Sounds good for me. Actually I took a lot of time to notice the problem. Another possible way to turn strict validation on is to use a global option in libvirtd.conf. It would be easy to turn it on all related methods (and may be easy to implement). Anyway, we need to keep RNG schemas valid though, I've found errors in storagevol.rng through trying xmllint... (e.g., the 'volume' element does not allow 'type' attribute.)
strict validation. We shouldn't turn on validation by default though, because that makes back compatability harder for applications, because they have to make sure never to use new XML attributes/elements with old libvirt installs.
I agree.
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, May 21, 2009 at 11:29:33PM +0900, Ryota Ozaki wrote:
Anyway, we need to keep RNG schemas valid though, I've found errors in storagevol.rng through trying xmllint... (e.g., the 'volume' element does not allow 'type' attribute.)
We have a test case which validates a number of example XML files using the RNG schemea. See tests/storagevolschematest and the data files in tests/storagevolschemadata/*.xml If there are some attributes not covered by these example data files, then do send patches to add more data files, or extennd the existing ones. The test case is there to try and ensure the RNG schema is as accurate as possible. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, May 21, 2009 at 11:35 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, May 21, 2009 at 11:29:33PM +0900, Ryota Ozaki wrote:
Anyway, we need to keep RNG schemas valid though, I've found errors in storagevol.rng through trying xmllint... (e.g., the 'volume' element does not allow 'type' attribute.)
We have a test case which validates a number of example XML files using the RNG schemea. See tests/storagevolschematest and the data files in tests/storagevolschemadata/*.xml
If there are some attributes not covered by these example data files, then do send patches to add more data files, or extennd the existing ones. The test case is there to try and ensure the RNG schema is as accurate as possible.
OK, I'll confirm the errors and write patches if needed. Thanks, ozaki-r
Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, May 21, 2009 at 11:05:15AM +0100, Daniel P. Berrange wrote:
On Thu, May 21, 2009 at 10:46:11AM +0200, Daniel Veillard wrote:
On Thu, May 21, 2009 at 05:25:35PM +0900, Ryota Ozaki wrote:
Yes, I also checked that.
BTW, is the schema file used by libvirtd or drivers? This is just interest.
We don't use them in the libvirt code themselves (though it would be relatively trivial to ask libxml2 to validate instances with them), but some are used byt the test suite. I'm not sure the storage schemas are used though, but we try to keep them up to date when the XML input and output code is updated.
I think it could be nice to add a flag to the various DefineXML/CreateXML methods VIR_DOMAIN_XML_VALIDATE, to tell the parser that the app wants strict validation. We shouldn't turn on validation by default though, because that makes back compatability harder for applications, because they have to make sure never to use new XML attributes/elements with old libvirt installs.
Unfortunately virDomainDefineXML virNetworkCreateXML virNetworkDefineXML don't have flags for validation, so the approach will be limited to a subset. One thing we could do is a generic virValidateXML(const char *xmlDesc, unsigned int flags); possibly with a conn parameter too for error reporting, as long as we keep all top level nodes for each xml descriptions different then, it's trivial to know what schemas to use for validation (or we could build a generic schemas importing all the others). That will be just a wrapper around xmlRelaxNGValidateDoc() and related calls, with a compile time detection, in case libxml2 was built without schemas validation. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, May 21, 2009 at 5:46 PM, Daniel Veillard <veillard@redhat.com> wrote:
On Thu, May 21, 2009 at 05:25:35PM +0900, Ryota Ozaki wrote:
Yes, I also checked that.
BTW, is the schema file used by libvirtd or drivers? This is just interest.
We don't use them in the libvirt code themselves (though it would be relatively trivial to ask libxml2 to validate instances with them), but some are used byt the test suite. I'm not sure the storage schemas are used though, but we try to keep them up to date when the XML input and output code is updated.
Thank you for letting me know libxml2 (xmllint)! It's very helpful for me to validate my XML files. ozaki-r
Daniel
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, May 21, 2009 at 03:57:28PM +0900, Ryota Ozaki wrote:
Hi,
I've found a problem that a 'format' element in storage-{pool,vol} XML doesn't behave as described in the document.
The document says that a format of {pool,vol} is specified as the value of a format element, like:
<target> <path>/var/lib/virt/images/sparse.img</path> <format>qcow2</format> <permissions>
However, the implementation doesn't follow this:
if (options->formatFromString) { char *format = virXPathString(conn, "string(/volume/target/format/@type)", ctxt); if (format == NULL) ret->target.format = options->defaultFormat; else ret->target.format = (options->formatFromString)(format);
if (ret->target.format < 0) { virStorageReportError(conn, VIR_ERR_XML_ERROR, _("unknown volume format type %s"), format); VIR_FREE(format); goto cleanup; } VIR_FREE(format); }
The implementation assumes that a format of {pool,vol} is specified as the attribute of a format element, like:
<target> <path>/var/lib/virt/images/sparse.img</path> <format type='qcow2' /> <permissions>
Thus, we need to fix either the document or the implementation. (I guess the implementation is correct, right?)
Yes, the code is always correct :-P If only because people will be using it in the way it works, rather than the way its documented. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Ryota Ozaki