[libvirt] [PATCH 0/4] news: Add schema validation and tweak few details
The file caused quite some trouble. Add schema validation so that breakage is kept to a minimum. Peter Krempa (4): tests: schema: Add possibility to validate individual files news: Introduce rules for the schema file and fix offending lines schema: Introduce schema for the news.xml file news: Add template for a <release> section docs/news.xml | 64 +++++++++++++++++++++++++++++++++++--------- docs/schemas/news.rng | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/virschematest.c | 36 ++++++++++++++++++++----- 3 files changed, 153 insertions(+), 20 deletions(-) create mode 100644 docs/schemas/news.rng -- 2.12.1
Sometimes it may be desired to validate individual files against a schema. Refactor the data structures to unify them and introduce a new macro DO_TEST_FILE(schema, xmlfile) which will test the XML file against the given schema file. --- tests/virschematest.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/tests/virschematest.c b/tests/virschematest.c index faf66d642..beefabc96 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -35,6 +35,7 @@ VIR_LOG_INIT("tests.schematest"); struct testSchemaData { virXMLValidatorPtr validator; + const char *schema; const char *xml_path; }; @@ -140,15 +141,10 @@ testSchemaDirs(const char *schema, virXMLValidatorPtr validator, ...) } -struct testSchemaFileData { - virXMLValidatorPtr validator; - const char *schema; -}; - static int testSchemaGrammar(const void *opaque) { - struct testSchemaFileData *data = (struct testSchemaFileData *) opaque; + struct testSchemaData *data = (struct testSchemaData *) opaque; char *schema_path; int ret = -1; @@ -171,7 +167,7 @@ static int mymain(void) { int ret = 0; - struct testSchemaFileData data; + struct testSchemaData data; memset(&data, 0, sizeof(data)); @@ -196,6 +192,30 @@ mymain(void) } \ } while (0) +#define DO_TEST_FILE(sch, xmlfile) \ + do { \ + data.schema = sch; \ + data.xml_path = xmlfile; \ + if (virTestRun("test schema grammar file: " sch, \ + testSchemaGrammar, &data) == 0) { \ + /* initialize the validator even if the schema test \ + * was skipped because of VIR_TEST_RANGE */ \ + if (!data.validator && testSchemaGrammar(&data) < 0) { \ + ret = -1; \ + break; \ + } \ + if (virTestRun("Checking " xmlfile " against " sch, \ + testSchemaFile, &data) < 0) \ + ret = -1; \ + \ + virXMLValidatorFree(data.validator); \ + data.validator = NULL; \ + } else { \ + ret = -1; \ + } \ + } while (0) + + DO_TEST("capability.rng", "capabilityschemadata", "xencapsdata"); DO_TEST("domain.rng", "domainschemadata", "qemuargv2xmldata", "qemuxml2argvdata", "sexpr2xmldata", "xmconfigdata", -- 2.12.1
On Tue, 2017-03-28 at 13:58 +0200, Peter Krempa wrote: [...]
@@ -196,6 +192,30 @@ mymain(void) } \ } while (0) +#define DO_TEST_FILE(sch, xmlfile) \ + do { \ + data.schema = sch; \ + data.xml_path = xmlfile; \ + if (virTestRun("test schema grammar file: " sch, \ + testSchemaGrammar, &data) == 0) { \ + /* initialize the validator even if the schema test \ + * was skipped because of VIR_TEST_RANGE */ \ + if (!data.validator && testSchemaGrammar(&data) < 0) { \ + ret = -1; \ + break; \ + } \ + if (virTestRun("Checking " xmlfile " against " sch, \ + testSchemaFile, &data) < 0) \ + ret = -1; \ + \ + virXMLValidatorFree(data.validator); \ + data.validator = NULL; \ + } else { \ + ret = -1; \ + } \ + } while (0) + +
Only one empty line here, please. Now that you've introduced DO_TEST_FILE(), I think it would make sense to rename DO_TEST() to DO_TEST_DIRS() for clarity. ACK whether you feel the same or not. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Mar 28, 2017 at 01:58:56PM +0200, Peter Krempa wrote:
Sometimes it may be desired to validate individual files against a schema. Refactor the data structures to unify them and introduce a new macro DO_TEST_FILE(schema, xmlfile) which will test the XML file against the given schema file. --- tests/virschematest.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/tests/virschematest.c b/tests/virschematest.c index faf66d642..beefabc96 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -35,6 +35,7 @@ VIR_LOG_INIT("tests.schematest");
struct testSchemaData { virXMLValidatorPtr validator; + const char *schema; const char *xml_path; };
@@ -140,15 +141,10 @@ testSchemaDirs(const char *schema, virXMLValidatorPtr validator, ...) }
-struct testSchemaFileData { - virXMLValidatorPtr validator; - const char *schema; -}; - static int testSchemaGrammar(const void *opaque) { - struct testSchemaFileData *data = (struct testSchemaFileData *) opaque; + struct testSchemaData *data = (struct testSchemaData *) opaque; char *schema_path; int ret = -1;
@@ -171,7 +167,7 @@ static int mymain(void) { int ret = 0; - struct testSchemaFileData data; + struct testSchemaData data;
memset(&data, 0, sizeof(data));
@@ -196,6 +192,30 @@ mymain(void) } \ } while (0)
+#define DO_TEST_FILE(sch, xmlfile) \ + do { \ + data.schema = sch; \ + data.xml_path = xmlfile; \ + if (virTestRun("test schema grammar file: " sch, \ + testSchemaGrammar, &data) == 0) { \ + /* initialize the validator even if the schema test \ + * was skipped because of VIR_TEST_RANGE */ \ + if (!data.validator && testSchemaGrammar(&data) < 0) { \ + ret = -1; \ + break; \ + } \ + if (virTestRun("Checking " xmlfile " against " sch, \ + testSchemaFile, &data) < 0) \
testSchemaFile does not prepend abs_srcdir to the path (like testSchemaDirs does), so after you add the test case, the check fails in vpath builds (I just checked it). I would suggest just a wrapper around testSchemaFile that prepends that path or just use it when calling the macro.
+ ret = -1; \ + \ + virXMLValidatorFree(data.validator); \ + data.validator = NULL; \ + } else { \ + ret = -1; \ + } \ + } while (0) + + DO_TEST("capability.rng", "capabilityschemadata", "xencapsdata"); DO_TEST("domain.rng", "domainschemadata", "qemuargv2xmldata", "qemuxml2argvdata", "sexpr2xmldata", "xmconfigdata", -- 2.12.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Add more strict rules for the news file and fix offending entries. --- docs/news.xml | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/docs/news.xml b/docs/news.xml index 17186b954..024b659e8 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -5,9 +5,20 @@ This file will be processed to produce both HTML and plain text versions of the release notes. - Keep the style consistent with existing entries as much as possible: - each change should be documented by a short, one-sentence summary - and optionally a description where it's explained in more detail --> + Keep the style consistent with existing entries as much as possible. + + Each change should be documented by a short, one-sentence one-line summary. + The summary should not contain any formatting tags. + + Optionally use a description where the change can be explained in more + detail. The description may contain a <code> tag for switching to + non-proportional font. No other tags are allowed. + + Each <release> tag is required to contain at least one <section> tag and + each <section> tag is required to contain at least one <change> tag. + + Lines should be kept under 80 columns, and should not exceed 100 columns. + --> <libvirt> <release version="v3.2.0" date="unreleased"> @@ -355,7 +366,7 @@ </change> <change> <summary> - nss: Introduce <code>libvirt_guest</code> + nss: Introduce libvirt_guest </summary> <description> New <code>libvirt_guest</code> nss module that translates libvirt @@ -615,7 +626,7 @@ <description> The new libssh transport allows one to connect to a running libvirtd via SSH, using the libssh library; for example: - <tt>qemu+libssh://<i>server</i>/system</tt>. + <code>qemu+libssh://server/system</code>. </description> </change> <change> @@ -629,17 +640,22 @@ </change> <change> <summary> - qemu: Users can now enable debug logging for native gluster - volumes in qemu using the "gluster_debug_level" option in qemu.conf + Allow debugging of gluster volumes in qemu </summary> + <description> + Users can now enable debug logging for native gluster + volumes in qemu using the "gluster_debug_level" option in qemu.conf + </description> </change> <change> <summary> - memory hotplug: Slot numbers for memory devices are now - automatically allocated and thus persistent. In addition slot numbers - can be specified without providing a base address, which simplifies - user configuration + Pre-allocate memory slots for memory hotplug </summary> + <description> + Slot numbers for memory devices are now automatically allocated and + thus persistent. In addition slot numbers can be specified without + providing a base address, which simplifies user configuration + </description> </change> <change> <summary> @@ -669,8 +685,7 @@ </change> <change> <summary> - virsh: Add support for passing an alternative persistent XML - to migrate command + virsh: Add support for passing an alternative persistent XML to migrate command </summary> </change> <change> -- 2.12.1
On Tue, 2017-03-28 at 13:58 +0200, Peter Krempa wrote:
Add more strict rules for the news file and fix offending entries.
s/more strict/stricter/
--- docs/news.xml | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/docs/news.xml b/docs/news.xml index 17186b954..024b659e8 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -5,9 +5,20 @@ This file will be processed to produce both HTML and plain text versions of the release notes. - Keep the style consistent with existing entries as much as possible: - each change should be documented by a short, one-sentence summary - and optionally a description where it's explained in more detail --> + Keep the style consistent with existing entries as much as possible. + + Each change should be documented by a short, one-sentence one-line summary. + The summary should not contain any formatting tags.
"one-sentence one-line" is rather clunky, I would suggest rewording it along the lines of Each change should be documented by a short, one-sentence summary, which should fit in a single line and should not contain any formatting tags.
+ Optionally use a description where the change can be explained in more + detail.
I think something along the lines of You can optionally add a description if you feel like the summary alone is not enough to document the change accurately. would work better here.
The description may contain a <code> tag for switching to + non-proportional font. No other tags are allowed. + + Each <release> tag is required to contain at least one <section> tag and + each <section> tag is required to contain at least one <change> tag.
I'm not sure the above two line are really needed. I would omit them. ACK if you take care of at least the commit message and the first suggestion, I leave the remaining two up to you. -- Andrea Bolognani / Red Hat / Virtualization
Since this file gets changed (and broken) rather often, introduce a schema file so that the test suite can validate it. --- docs/news.xml | 2 ++ docs/schemas/news.rng | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/virschematest.c | 2 ++ 3 files changed, 77 insertions(+) create mode 100644 docs/schemas/news.rng diff --git a/docs/news.xml b/docs/news.xml index 024b659e8..c2a2917c1 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -18,6 +18,8 @@ each <section> tag is required to contain at least one <change> tag. Lines should be kept under 80 columns, and should not exceed 100 columns. + + This file is validated against docs/news.rng schema. --> <libvirt> diff --git a/docs/schemas/news.rng b/docs/schemas/news.rng new file mode 100644 index 000000000..94a6870c1 --- /dev/null +++ b/docs/schemas/news.rng @@ -0,0 +1,73 @@ +<?xml version="1.0"?> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + <start> + <ref name="libvirt"/> + </start> + + <define name="libvirt"> + <element name="libvirt"> + <oneOrMore> + <ref name="release"/> + </oneOrMore> + </element> + </define> + + <define name="release"> + <element name="release"> + <attribute name="version"> + <data type="string"> + <param name="pattern">v[0-9]+\.[0-9]+\.[0-9]+</param> + </data> + </attribute> + <attribute name="date"> + <data type="string"> + <param name="pattern">[0-9]{4}-[0-9]{2}-[0-9]{2}|unreleased</param> + </data> + </attribute> + <oneOrMore> + <ref name="section"/> + </oneOrMore> + </element> + </define> + + <define name="section"> + <element name="section"> + <attribute name="title"> + <data type="string"/> + </attribute> + <oneOrMore> + <ref name="change"/> + </oneOrMore> + </element> + </define> + + <define name="change"> + <element name="change"> + <element name="summary"> + <choice> + <data type="string"> + <param name="pattern">\n[^\n]+\n +</param> + </data> + <empty/> + </choice> + </element> + <optional> + <element name="description"> + <ref name="description"/> + </element> + </optional> + </element> + </define> + + <define name="description"> + <oneOrMore> + <choice> + <text/> + <element name="code"> + <text/> + </element> + </choice> + </oneOrMore> + </define> + +</grammar> diff --git a/tests/virschematest.c b/tests/virschematest.c index beefabc96..e0e5872db 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -237,6 +237,8 @@ mymain(void) DO_TEST("storagevol.rng", "storagevolxml2xmlin", "storagevolxml2xmlout", "storagevolschemadata"); + DO_TEST_FILE("news.rng", "../docs/news.xml"); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.12.1
On Tue, 2017-03-28 at 13:58 +0200, Peter Krempa wrote: [...]
@@ -18,6 +18,8 @@ each <section> tag is required to contain at least one <change> tag. Lines should be kept under 80 columns, and should not exceed 100 columns. + + This file is validated against docs/news.rng schema.
It's actually docs/schemas/news.rng ;) [...]
@@ -0,0 +1,73 @@ +<?xml version="1.0"?> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes">
I would put datatypeLibrary= right under xmlns=, but it's okay either way. I'm no RNG expert, but the schema looks fine to me.
@@ -237,6 +237,8 @@ mymain(void) DO_TEST("storagevol.rng", "storagevolxml2xmlin", "storagevolxml2xmlout", "storagevolschemadata"); + DO_TEST_FILE("news.rng", "../docs/news.xml");
You'll want to deal with how this interacts with VPATH builds, as mentioned by Martin. -- Andrea Bolognani / Red Hat / Virtualization
After the release it's necessary to add a new <release> section for the upcomming release. Add a template so that it does not have to be compiled over and over again. --- docs/news.xml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index c2a2917c1..1ebe0eec3 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -20,6 +20,27 @@ Lines should be kept under 80 columns, and should not exceed 100 columns. This file is validated against docs/news.rng schema. + + Use the following template to add a new release section after the release: + + <release version="FIXME" date="unreleased"> + <section title="New features"> + <change> + <summary/> + </change> + </section> + <section title="Improvements"> + <change> + <summary/> + </change> + </section> + <section title="Bug fixes"> + <change> + <summary/> + </change> + </section> + </release> + --> <libvirt> -- 2.12.1
On Tue, 2017-03-28 at 13:58 +0200, Peter Krempa wrote:
After the release it's necessary to add a new <release> section for the upcomming release. Add a template so that it does not have to be
s/upcomming/upcoming/ [...]
@@ -20,6 +20,27 @@ Lines should be kept under 80 columns, and should not exceed 100 columns. This file is validated against docs/news.rng schema. + + Use the following template to add a new release section after the release:
s/ after the release// ACK with these two nits fixed. -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Martin Kletzander -
Peter Krempa