[libvirt] [PATCH 2/2] virschematest: Make sure that validator is initialized

[Ján Tomko]

On Fri, Aug 12, 2016 at 02:59:09PM +0200, Michal Privoznik wrote:
>It may happen that a developer wants to run just a specific
>subset of tests:
>
>tests $ VIR_TEST_RANGE=22 ../run ./virschematest
>
>This now fails miserably:
>
>    ==6840== Invalid read of size 8
>    ==6840==    at 0x4F397C0: virXMLValidatorValidate (virxml.c:1216)
>    ==6840==    by 0x402B72: testSchemaFile (virschematest.c:53)
>    ==6840==    by 0x403737: virTestRun (testutils.c:180)
>    ==6840==    by 0x402CF5: testSchemaDir (virschematest.c:98)
>    ==6840==    by 0x402EB1: testSchemaDirs (virschematest.c:131)
>    ==6840==    by 0x40314D: mymain (virschematest.c:194)
>    ==6840==    by 0x4051AF: virTestMain (testutils.c:982)
>    ==6840==    by 0x4035A9: main (virschematest.c:217)
>    ==6840==  Address 0x10 is not stack'd, malloc'd or (recently) free'd
>
>Problem is, we are trying to do two types of tests here: validate
>RNG schema itself, and validate XML files against RNG schemas.
>And the latter tries to re-use a resource allocated in the
>former. Therefore if the former is skipped (due to
>VIR_TEST_RANGE) we have to allocate the resource manually.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> tests/virschematest.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>

Introduced by commit 27bdc0af, which was fixing two bugs:
* a failure of virXMLValidatorInit did not set ret to -1
* the errors reported by virXMLValidatorInit were not displayed
  even with DEBUG on, because nothing called virDispatchError

>diff --git a/tests/virschematest.c b/tests/virschematest.c
>index c9cc314..1b55dad 100644
>--- a/tests/virschematest.c
>+++ b/tests/virschematest.c
>@@ -38,6 +38,14 @@ struct testSchemaData {
>     const char *xml_path;
> };
>
>+static char *
>+testGetSchemaPath(const char *schema)
>+{
>+    char *schema_path;
>+    ignore_value(virAsprintf(&schema_path, "%s/docs/schemas/%s",
>+                             abs_topsrcdir, schema));
>+    return schema_path;
>+}
>
> static int
> testSchemaFile(const void *args)
>@@ -120,6 +128,20 @@ testSchemaDirs(const char *schema, virXMLValidatorPtr validator, ...)
>     int ret = 0;
>     char *dir_path = NULL;
>     const char *dir;
>+    bool freeValidator = false;
>+
>+    if (!validator) {
>+        char *schema_path = testGetSchemaPath(schema);
>+
>+        if (!schema_path ||
>+            !(validator = virXMLValidatorInit(schema_path))) {
>+            VIR_FREE(schema_path);

This leaves ret at 0 and skips va_start (but valgrind does not
complain).

>+            goto cleanup;
>+        }
>+
>+        VIR_FREE(schema_path);
>+        freeValidator = true;
>+    }
>
>     va_start(args, validator);
>
>@@ -134,6 +156,8 @@ testSchemaDirs(const char *schema, virXMLValidatorPtr validator, ...)
>     }
>
>  cleanup:
>+    if (freeValidator)
>+        virXMLValidatorFree(validator);

Rather than cleaning it up conditionally, I would let the caller handle
it:

@@ -180,6 +180,12 @@ mymain(void)
         data.schema = sch;                                                     \
         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 (testSchemaDirs(sch, data.validator, __VA_ARGS__, NULL) < 0)    \
                 ret = -1;                                                      \

This still won't dispatch the errors from virXMLValidatorInit in case
the schema test is skipped by VIR_TEST_RANGE.

But it should not matter since the order of the tests is still altered
based on whether the schema test is skipped or not.


So ACK if you agree with my suggested diff.


If we want the test ordering independent on the schema tests, we should
not be skipping any virTestRun, initializing the schema all the way
inside testSchemaFile. I will put that on my TODO list.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160812/73ff2417/attachment-0001.sig>