[libvirt] [PATCH 0/9] tests: Couple of small improvements

*** BLURB HERE *** Michal Prívozník (9): tests: Always put a '\n' after each debug print tests: Always put '\n' at the end of VIR_TEST_VERBOSE virhashtest: Drop useless new line virhostdevtest: Check for integer retval in more verbose way virhostdevtest: Drop most of 'cleanup' and 'out' labels virhostdevtest: Drop useless VIR_TEST_DEBUG virhostdevtest: s/VIR_DEBUG/VIR_TEST_DEBUG/ virhostdevtest: Reset libvirt error on expected failure virhostdevtest: s/virReportError/fprintf/ tests/bhyveargv2xmltest.c | 6 +- tests/bhyvexml2argvtest.c | 4 +- tests/bhyvexml2xmltest.c | 2 +- tests/cputest.c | 16 +-- tests/qemublocktest.c | 10 +- tests/qemuhotplugtest.c | 10 +- tests/qemumonitorjsontest.c | 8 +- tests/qemuxml2argvtest.c | 2 +- tests/qemuxml2xmltest.c | 4 +- tests/securityselinuxlabeltest.c | 4 +- tests/storagepoolxml2argvtest.c | 8 +- tests/testutils.c | 2 +- tests/testutils.h | 8 +- tests/testutilsqemu.c | 11 +- tests/testutilsqemuschema.c | 10 +- tests/utiltest.c | 32 ++--- tests/virbuftest.c | 8 +- tests/virerrortest.c | 12 +- tests/virhashtest.c | 21 ++- tests/virhostcputest.c | 2 +- tests/virhostdevtest.c | 217 +++++++++++-------------------- tests/virjsontest.c | 80 ++++++------ tests/virlogtest.c | 14 +- tests/virnetdaemontest.c | 4 +- tests/virpcitest.c | 2 +- tests/virresctrltest.c | 2 +- 26 files changed, 221 insertions(+), 278 deletions(-) -- 2.21.0

There is an inconsistency with VIR_TEST_DEBUG() calls. One half (roughly) of calls does have the newline character the other one doesn't. Well, it doesn't have it because it assumed blindly that new line will be printed, which is not the case. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/bhyveargv2xmltest.c | 6 +++--- tests/bhyvexml2argvtest.c | 4 ++-- tests/bhyvexml2xmltest.c | 2 +- tests/cputest.c | 2 +- tests/qemuxml2argvtest.c | 2 +- tests/qemuxml2xmltest.c | 4 ++-- tests/storagepoolxml2argvtest.c | 8 ++++---- tests/testutils.c | 2 +- tests/testutils.h | 4 +++- tests/testutilsqemu.c | 2 +- tests/testutilsqemuschema.c | 2 +- tests/utiltest.c | 32 ++++++++++++++++---------------- tests/virbuftest.c | 8 ++++---- tests/virhostcputest.c | 2 +- tests/virjsontest.c | 6 +++--- tests/virlogtest.c | 14 +++++++------- tests/virnetdaemontest.c | 4 ++-- tests/virpcitest.c | 2 +- tests/virresctrltest.c | 2 +- 19 files changed, 55 insertions(+), 53 deletions(-) diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c index d552364846..49acb74759 100644 --- a/tests/bhyveargv2xmltest.c +++ b/tests/bhyveargv2xmltest.c @@ -41,13 +41,13 @@ testCompareXMLToArgvFiles(const char *xmlfile, driver.xmlopt))) { if ((flags & FLAG_EXPECT_FAILURE) && !virTestOOMActive()) { VIR_TEST_DEBUG("Got expected failure from " - "bhyveParseCommandLineString.\n"); + "bhyveParseCommandLineString."); } else { goto fail; } } else if ((flags & FLAG_EXPECT_FAILURE) && !virTestOOMActive()) { VIR_TEST_DEBUG("Did not get expected failure from " - "bhyveParseCommandLineString.\n"); + "bhyveParseCommandLineString."); goto fail; } @@ -61,7 +61,7 @@ testCompareXMLToArgvFiles(const char *xmlfile, log); } else { VIR_TEST_DEBUG("bhyveParseCommandLineString " - "should have logged a warning\n"); + "should have logged a warning"); goto fail; } } else { /* didn't expect a warning */ diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c index c84925c57b..72374d74ba 100644 --- a/tests/bhyvexml2argvtest.c +++ b/tests/bhyvexml2argvtest.c @@ -41,7 +41,7 @@ static int testCompareXMLToArgvFiles(const char *xml, ret = 0; } else if (flags & FLAG_EXPECT_FAILURE) { ret = 0; - VIR_TEST_DEBUG("Got expected error: %s\n", + VIR_TEST_DEBUG("Got expected error: %s", virGetLastErrorMessage()); virResetLastError(); } @@ -61,7 +61,7 @@ static int testCompareXMLToArgvFiles(const char *xml, if ((cmd == NULL) || (ldcmd == NULL)) { if (flags & FLAG_EXPECT_FAILURE) { ret = 0; - VIR_TEST_DEBUG("Got expected error: %s\n", + VIR_TEST_DEBUG("Got expected error: %s", virGetLastErrorMessage()); virResetLastError(); } diff --git a/tests/bhyvexml2xmltest.c b/tests/bhyvexml2xmltest.c index e8a6867fcd..a02d16cb9e 100644 --- a/tests/bhyvexml2xmltest.c +++ b/tests/bhyvexml2xmltest.c @@ -44,7 +44,7 @@ testCompareXMLToXMLHelper(const void *data) if ((ret != 0) && (info->flags & FLAG_EXPECT_FAILURE)) { ret = 0; - VIR_TEST_DEBUG("Got expected error: %s\n", + VIR_TEST_DEBUG("Got expected error: %s", virGetLastErrorMessage()); virResetLastError(); } diff --git a/tests/cputest.c b/tests/cputest.c index 6e28e05756..dfc01f7e75 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -1028,7 +1028,7 @@ mymain(void) char *log; \ if ((log = virTestLogContentAndReset()) && \ strlen(log) > 0) \ - VIR_TEST_DEBUG("\n%s\n", log); \ + VIR_TEST_DEBUG("\n%s", log); \ VIR_FREE(log); \ } \ ret = -1; \ diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c166fd18d6..1f4d4ed09a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -549,7 +549,7 @@ testCompareXMLToArgv(const void *data) ok: if (ret == 0 && flags & FLAG_EXPECT_FAILURE) { ret = -1; - VIR_TEST_DEBUG("Error expected but there wasn't any.\n"); + VIR_TEST_DEBUG("Error expected but there wasn't any."); goto cleanup; } if (!virTestOOMActive()) { diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 525eb9a740..07bcc29c1e 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -62,7 +62,7 @@ testCompareStatusXMLToXMLFiles(const void *opaque) VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) { - VIR_TEST_DEBUG("\nfailed to parse '%s'\n", data->infile); + VIR_TEST_DEBUG("\nfailed to parse '%s'", data->infile); goto cleanup; } @@ -72,7 +72,7 @@ testCompareStatusXMLToXMLFiles(const void *opaque) VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST))) { - VIR_TEST_DEBUG("\nfailed to format back '%s'\n", data->infile); + VIR_TEST_DEBUG("\nfailed to format back '%s'", data->infile); goto cleanup; } diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index 0c01931946..8e09149906 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -38,13 +38,13 @@ testCompareXMLToArgvFiles(bool shouldFail, case VIR_STORAGE_POOL_FS: case VIR_STORAGE_POOL_NETFS: if (!(pool = virStoragePoolObjNew())) { - VIR_TEST_DEBUG("pool type '%s' alloc pool obj fails\n", defTypeStr); + VIR_TEST_DEBUG("pool type '%s' alloc pool obj fails", defTypeStr); goto cleanup; } virStoragePoolObjSetDef(pool, def); if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) { - VIR_TEST_DEBUG("pool type '%s' has no pool source\n", defTypeStr); + VIR_TEST_DEBUG("pool type '%s' has no pool source", defTypeStr); def = NULL; goto cleanup; } @@ -70,12 +70,12 @@ testCompareXMLToArgvFiles(bool shouldFail, case VIR_STORAGE_POOL_VSTORAGE: case VIR_STORAGE_POOL_LAST: default: - VIR_TEST_DEBUG("pool type '%s' has no xml2argv test\n", defTypeStr); + VIR_TEST_DEBUG("pool type '%s' has no xml2argv test", defTypeStr); goto cleanup; }; if (!(actualCmdline = virCommandToString(cmd, false))) { - VIR_TEST_DEBUG("pool type '%s' failed to get commandline\n", defTypeStr); + VIR_TEST_DEBUG("pool type '%s' failed to get commandline", defTypeStr); goto cleanup; } diff --git a/tests/testutils.c b/tests/testutils.c index 245b1832f6..d6a6742e10 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -1006,7 +1006,7 @@ int virTestMain(int argc, fprintf(stderr, "Usage: %s\n", argv[0]); fputs("effective environment variables:\n" "VIR_TEST_VERBOSE set to show names of individual tests\n" - "VIR_TEST_DEBUG set to show information for debugging failures\n", + "VIR_TEST_DEBUG set to show information for debugging failures", stderr); return EXIT_FAILURE; } diff --git a/tests/testutils.h b/tests/testutils.h index 8c12fd7c12..cfc60084b2 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -89,8 +89,10 @@ unsigned int virTestGetRegenerate(void); #define VIR_TEST_DEBUG(...) \ do { \ - if (virTestGetDebug()) \ + if (virTestGetDebug()) { \ fprintf(stderr, __VA_ARGS__); \ + fprintf(stderr, "\n"); \ + } \ } while (0) #define VIR_TEST_VERBOSE(...) \ diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 9ac9f9bd39..cd04606cf0 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -837,7 +837,7 @@ testQemuGetLatestCapsForArch(const char *arch, continue; if (virParseVersionString(tmp, &ver, false) < 0) { - VIR_TEST_DEBUG("skipping caps file '%s'\n", ent->d_name); + VIR_TEST_DEBUG("skipping caps file '%s'", ent->d_name); continue; } diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index f1365e8846..0728771bfe 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -538,7 +538,7 @@ testQEMUSchemaGetLatest(void) return NULL; } - VIR_TEST_DEBUG("replies file: '%s'\n", capsLatestFile); + VIR_TEST_DEBUG("replies file: '%s'", capsLatestFile); if (virTestLoadFile(capsLatestFile, &capsLatest) < 0) goto cleanup; diff --git a/tests/utiltest.c b/tests/utiltest.c index 5e4920e3ca..71b8d869bd 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -78,8 +78,8 @@ testDiskNameToIndex(const void *data ATTRIBUTE_UNUSED) idx = virDiskNameToIndex(diskName); if (idx < 0 || idx != i) { - VIR_TEST_DEBUG("\nExpect [%zu]\n", i); - VIR_TEST_DEBUG("Actual [%d]\n", idx); + VIR_TEST_DEBUG("\nExpect [%zu]", i); + VIR_TEST_DEBUG("Actual [%d]", idx); VIR_FREE(diskName); @@ -108,21 +108,21 @@ testDiskNameParse(const void *data ATTRIBUTE_UNUSED) return -1; if (disk->idx != idx) { - VIR_TEST_DEBUG("\nExpect [%d]\n", disk->idx); - VIR_TEST_DEBUG("Actual [%d]\n", idx); + VIR_TEST_DEBUG("\nExpect [%d]", disk->idx); + VIR_TEST_DEBUG("Actual [%d]", idx); return -1; } if (disk->partition != partition) { - VIR_TEST_DEBUG("\nExpect [%d]\n", disk->partition); - VIR_TEST_DEBUG("Actual [%d]\n", partition); + VIR_TEST_DEBUG("\nExpect [%d]", disk->partition); + VIR_TEST_DEBUG("Actual [%d]", partition); return -1; } } for (i = 0; i < ARRAY_CARDINALITY(diskNamesInvalid); ++i) { if (!virDiskNameParse(diskNamesInvalid[i], &idx, &partition)) { - VIR_TEST_DEBUG("Should Fail [%s]\n", diskNamesInvalid[i]); + VIR_TEST_DEBUG("Should Fail [%s]", diskNamesInvalid[i]); return -1; } } @@ -166,9 +166,9 @@ testParseVersionString(const void *data ATTRIBUTE_UNUSED) versions[i].allowMissing); if (result != versions[i].result) { - VIR_TEST_DEBUG("\nVersion string [%s]\n", versions[i].string); - VIR_TEST_DEBUG("Expect result [%d]\n", versions[i].result); - VIR_TEST_DEBUG("Actual result [%d]\n", result); + VIR_TEST_DEBUG("\nVersion string [%s]", versions[i].string); + VIR_TEST_DEBUG("Expect result [%d]", versions[i].result); + VIR_TEST_DEBUG("Actual result [%d]", result); return -1; } @@ -177,9 +177,9 @@ testParseVersionString(const void *data ATTRIBUTE_UNUSED) continue; if (version != versions[i].version) { - VIR_TEST_DEBUG("\nVersion string [%s]\n", versions[i].string); - VIR_TEST_DEBUG("Expect version [%lu]\n", versions[i].version); - VIR_TEST_DEBUG("Actual version [%lu]\n", version); + VIR_TEST_DEBUG("\nVersion string [%s]", versions[i].string); + VIR_TEST_DEBUG("Expect version [%lu]", versions[i].version); + VIR_TEST_DEBUG("Actual version [%lu]", version); return -1; } @@ -213,9 +213,9 @@ testRoundValueToPowerOfTwo(const void *data ATTRIBUTE_UNUSED) for (i = 0; i < ARRAY_CARDINALITY(roundData); i++) { result = VIR_ROUND_UP_POWER_OF_TWO(roundData[i].input); if (roundData[i].output != result) { - VIR_TEST_DEBUG("\nInput number [%u]\n", roundData[i].input); - VIR_TEST_DEBUG("Expected number [%u]\n", roundData[i].output); - VIR_TEST_DEBUG("Actual number [%u]\n", result); + VIR_TEST_DEBUG("\nInput number [%u]", roundData[i].input); + VIR_TEST_DEBUG("Expected number [%u]", roundData[i].output); + VIR_TEST_DEBUG("Actual number [%u]", result); return -1; } diff --git a/tests/virbuftest.c b/tests/virbuftest.c index caad6f3ecb..a7eaef636d 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -354,7 +354,7 @@ testBufAddStr(const void *opaque ATTRIBUTE_UNUSED) } if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufAddStr(): Strings don't match:\n"); + VIR_TEST_DEBUG("testBufAddStr(): Strings don't match:"); virTestDifference(stderr, data->expect, actual); goto cleanup; } @@ -387,7 +387,7 @@ testBufEscapeStr(const void *opaque ATTRIBUTE_UNUSED) } if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufEscapeStr(): Strings don't match:\n"); + VIR_TEST_DEBUG("testBufEscapeStr(): Strings don't match:"); virTestDifference(stderr, data->expect, actual); goto cleanup; } @@ -416,7 +416,7 @@ testBufEscapeRegex(const void *opaque) } if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufEscapeRegex: Strings don't match:\n"); + VIR_TEST_DEBUG("testBufEscapeRegex: Strings don't match:"); virTestDifference(stderr, data->expect, actual); goto cleanup; } @@ -445,7 +445,7 @@ testBufSetIndent(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; if (STRNEQ(actual, " test\n test2\n")) { - VIR_TEST_DEBUG("testBufSetIndent: expected indent not set\n"); + VIR_TEST_DEBUG("testBufSetIndent: expected indent not set"); goto cleanup; } diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c index bb60dd3ffc..88e6e9dc16 100644 --- a/tests/virhostcputest.c +++ b/tests/virhostcputest.c @@ -48,7 +48,7 @@ linuxTestCompareFiles(const char *cpuinfofile, &nodeinfo.cores, &nodeinfo.threads) < 0) { if (virTestGetDebug()) { if (virGetLastErrorCode()) - VIR_TEST_DEBUG("\n%s\n", virGetLastErrorMessage()); + VIR_TEST_DEBUG("\n%s", virGetLastErrorMessage()); } VIR_FORCE_FCLOSE(cpuinfo); goto fail; diff --git a/tests/virjsontest.c b/tests/virjsontest.c index 4241acd911..dbc0b41cc8 100644 --- a/tests/virjsontest.c +++ b/tests/virjsontest.c @@ -42,7 +42,7 @@ testJSONFromFile(const void *data) VIR_TEST_VERBOSE("Failed to parse %s\n", info->doc); return -1; } else { - VIR_TEST_DEBUG("As expected, failed to parse %s\n", info->doc); + VIR_TEST_DEBUG("As expected, failed to parse %s", info->doc); return 0; } } else { @@ -77,7 +77,7 @@ testJSONFromString(const void *data) VIR_TEST_VERBOSE("Failed to parse %s\n", info->doc); return -1; } else { - VIR_TEST_DEBUG("As expected, failed to parse %s\n", info->doc); + VIR_TEST_DEBUG("As expected, failed to parse %s", info->doc); return 0; } } else { @@ -87,7 +87,7 @@ testJSONFromString(const void *data) } } - VIR_TEST_DEBUG("Parsed %s\n", info->doc); + VIR_TEST_DEBUG("Parsed %s", info->doc); if (!(formatted = virJSONValueToString(json, false))) { VIR_TEST_VERBOSE("Failed to format json data\n"); diff --git a/tests/virlogtest.c b/tests/virlogtest.c index 62b4b75300..ea7fd5e2a4 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -36,7 +36,7 @@ testLogMatch(const void *opaque) bool got = virLogProbablyLogMessage(data->str); if (got != data->pass) { - VIR_TEST_DEBUG("Expected '%d' but got '%d' for '%s'\n", + VIR_TEST_DEBUG("Expected '%d' but got '%d' for '%s'", data->pass, got, data->str); return -1; } @@ -54,7 +54,7 @@ testLogParseOutputs(const void *opaque) noutputs = virLogParseOutputs(data->str, &outputs); if (noutputs < 0) { if (!data->pass) { - VIR_TEST_DEBUG("Got expected error: %s\n", + VIR_TEST_DEBUG("Got expected error: %s", virGetLastErrorMessage()); virResetLastError(); ret = 0; @@ -62,10 +62,10 @@ testLogParseOutputs(const void *opaque) } } else if (noutputs != data->count) { VIR_TEST_DEBUG("Expected number of parsed outputs is %d, " - "but got %d\n", data->count, noutputs); + "but got %d", data->count, noutputs); goto cleanup; } else if (!data->pass) { - VIR_TEST_DEBUG("Test should have failed\n"); + VIR_TEST_DEBUG("Test should have failed"); goto cleanup; } @@ -86,7 +86,7 @@ testLogParseFilters(const void *opaque) nfilters = virLogParseFilters(data->str, &filters); if (nfilters < 0) { if (!data->pass) { - VIR_TEST_DEBUG("Got expected error: %s\n", + VIR_TEST_DEBUG("Got expected error: %s", virGetLastErrorMessage()); virResetLastError(); ret = 0; @@ -94,10 +94,10 @@ testLogParseFilters(const void *opaque) } } else if (nfilters != data->count) { VIR_TEST_DEBUG("Expected number of parsed outputs is %d, " - "but got %d\n", data->count, nfilters); + "but got %d", data->count, nfilters); goto cleanup; } else if (!data->pass) { - VIR_TEST_DEBUG("Test should have failed\n"); + VIR_TEST_DEBUG("Test should have failed"); goto cleanup; } diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c index 816bfe05d4..9089428047 100644 --- a/tests/virnetdaemontest.c +++ b/tests/virnetdaemontest.c @@ -338,13 +338,13 @@ static int testExecRestart(const void *opaque) cleanup: if (ret < 0) { if (!data->pass) { - VIR_TEST_DEBUG("Got expected error: %s\n", + VIR_TEST_DEBUG("Got expected error: %s", virGetLastErrorMessage()); virResetLastError(); ret = 0; } } else if (!data->pass) { - VIR_TEST_DEBUG("Test should have failed\n"); + VIR_TEST_DEBUG("Test should have failed"); ret = -1; } VIR_FREE(infile); diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 961a7eff1a..b4c51851d7 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -359,7 +359,7 @@ mymain(void) char *fakerootdir; if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { - VIR_TEST_DEBUG("Out of memory\n"); + VIR_TEST_DEBUG("Out of memory"); abort(); } diff --git a/tests/virresctrltest.c b/tests/virresctrltest.c index 5abbb11e9a..34882ac127 100644 --- a/tests/virresctrltest.c +++ b/tests/virresctrltest.c @@ -56,7 +56,7 @@ test_virResctrlGetUnused(const void *opaque) ret = 0; goto cleanup; } else if (data->fail) { - VIR_TEST_DEBUG("Error expected but there wasn't any.\n"); + VIR_TEST_DEBUG("Error expected but there wasn't any."); ret = -1; goto cleanup; } -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:17PM +0200, Michal Privoznik wrote:
There is an inconsistency with VIR_TEST_DEBUG() calls. One half (roughly) of calls does have the newline character the other one doesn't. Well, it doesn't have it because it assumed blindly that new line will be printed, which is not the case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/bhyveargv2xmltest.c | 6 +++--- tests/bhyvexml2argvtest.c | 4 ++-- tests/bhyvexml2xmltest.c | 2 +- tests/cputest.c | 2 +- tests/qemuxml2argvtest.c | 2 +- tests/qemuxml2xmltest.c | 4 ++-- tests/storagepoolxml2argvtest.c | 8 ++++---- tests/testutils.c | 2 +- tests/testutils.h | 4 +++- tests/testutilsqemu.c | 2 +- tests/testutilsqemuschema.c | 2 +- tests/utiltest.c | 32 ++++++++++++++++---------------- tests/virbuftest.c | 8 ++++---- tests/virhostcputest.c | 2 +- tests/virjsontest.c | 6 +++--- tests/virlogtest.c | 14 +++++++------- tests/virnetdaemontest.c | 4 ++-- tests/virpcitest.c | 2 +- tests/virresctrltest.c | 2 +- 19 files changed, 55 insertions(+), 53 deletions(-)
This change breaks the build with my Clang: cputest.c:981:1: error: stack frame size of 34216 bytes in function 'mymain' [-Werror,-Wframe-larger-than=] mymain(void) ^ 1 error generated.
diff --git a/tests/testutils.h b/tests/testutils.h index 8c12fd7c12..cfc60084b2 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -89,8 +89,10 @@ unsigned int virTestGetRegenerate(void);
#define VIR_TEST_DEBUG(...) \ do { \ - if (virTestGetDebug()) \ + if (virTestGetDebug()) { \ fprintf(stderr, __VA_ARGS__); \ + fprintf(stderr, "\n"); \ + } \ } while (0)
#define VIR_TEST_VERBOSE(...) \
This can be done with a single invocation of fprintf: diff --git a/tests/testutils.h b/tests/testutils.h index 7d8d7bbece..be20d3168d 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -87,11 +87,10 @@ unsigned int virTestGetVerbose(void); unsigned int virTestGetExpensive(void); unsigned int virTestGetRegenerate(void); -#define VIR_TEST_DEBUG(...) \ +#define VIR_TEST_DEBUG(fmt, ...) \ do { \ if (virTestGetDebug()) { \ - fprintf(stderr, __VA_ARGS__); \ - fprintf(stderr, "\n"); \ + fprintf(stderr, fmt "\n", ## __VA_ARGS__); \ } \ } while (0) Alternatively, the limit might need tweaking. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Similarly to the previous commit, VIR_TEST_VERBOSE should put '\n' at the end of each call so that the output is not broken. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/cputest.c | 14 +++--- tests/qemublocktest.c | 10 ++--- tests/qemuhotplugtest.c | 10 ++--- tests/qemumonitorjsontest.c | 8 ++-- tests/securityselinuxlabeltest.c | 4 +- tests/testutils.h | 4 +- tests/testutilsqemu.c | 9 ++-- tests/testutilsqemuschema.c | 8 ++-- tests/virerrortest.c | 12 +++--- tests/virhashtest.c | 18 ++++---- tests/virjsontest.c | 74 ++++++++++++++++---------------- 11 files changed, 87 insertions(+), 84 deletions(-) diff --git a/tests/cputest.c b/tests/cputest.c index dfc01f7e75..7037bcc8bd 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -215,7 +215,7 @@ cpuTestCompare(const void *arg) virResetLastError(); if (data->result != result) { - VIR_TEST_VERBOSE("\nExpected result %s, got %s\n", + VIR_TEST_VERBOSE("\nExpected result %s, got %s", cpuTestCompResStr(data->result), cpuTestCompResStr(result)); /* Pad to line up with test name ... in virTestRun */ @@ -290,7 +290,7 @@ cpuTestGuestCPU(const void *arg) virResetLastError(); ret = 0; } else { - VIR_TEST_VERBOSE("\nExpected result %d, got %d\n", + VIR_TEST_VERBOSE("\nExpected result %d, got %d", data->result, ret); /* Pad to line up with test name ... in virTestRun */ VIR_TEST_VERBOSE("%74s", "... "); @@ -357,7 +357,7 @@ cpuTestBaseline(const void *arg) cmp = virCPUCompare(cpus[i]->arch, cpus[i], baseline, false); if (cmp != VIR_CPU_COMPARE_SUPERSET && cmp != VIR_CPU_COMPARE_IDENTICAL) { - VIR_TEST_VERBOSE("\nbaseline CPU is incompatible with CPU %zu\n", + VIR_TEST_VERBOSE("\nbaseline CPU is incompatible with CPU %zu", i); VIR_TEST_VERBOSE("%74s", "... "); ret = -1; @@ -438,7 +438,7 @@ cpuTestHasFeature(const void *arg) virResetLastError(); if (data->result != result) { - VIR_TEST_VERBOSE("\nExpected result %s, got %s\n", + VIR_TEST_VERBOSE("\nExpected result %s, got %s", cpuTestBoolWithErrorStr(data->result), cpuTestBoolWithErrorStr(result)); /* Pad to line up with test name ... in virTestRun */ @@ -712,7 +712,7 @@ cpuTestUpdateLiveCompare(virArch arch, return -1; if (STRNEQ(actual->model, expected->model)) { - VIR_TEST_VERBOSE("Actual CPU model '%s', expected '%s'\n", + VIR_TEST_VERBOSE("Actual CPU model '%s', expected '%s'", actual->model, expected->model); return -1; } @@ -765,7 +765,7 @@ cpuTestUpdateLiveCompare(virArch arch, featExp->policy == VIR_CPU_FEATURE_REQUIRE) || (cmp > 0 && featExp->policy == VIR_CPU_FEATURE_REQUIRE)) { - VIR_TEST_VERBOSE("Actual CPU lacks feature '%s'\n", + VIR_TEST_VERBOSE("Actual CPU lacks feature '%s'", featExp->name); ret = -1; continue; @@ -778,7 +778,7 @@ cpuTestUpdateLiveCompare(virArch arch, featAct->policy == VIR_CPU_FEATURE_REQUIRE) || (cmp > 0 && featExp->policy == VIR_CPU_FEATURE_DISABLE)) { - VIR_TEST_VERBOSE("Actual CPU has extra feature '%s'\n", + VIR_TEST_VERBOSE("Actual CPU has extra feature '%s'", cmp <= 0 ? featAct->name : featExp->name); ret = -1; } diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 9321531f6c..45ff230408 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -211,7 +211,7 @@ testQemuDiskXMLToProps(const void *opaque) if (qemuCheckDiskConfig(disk, data->qemuCaps) < 0 || qemuDomainDeviceDefValidateDisk(disk, data->qemuCaps) < 0) { - VIR_TEST_VERBOSE("invalid configuration for disk\n"); + VIR_TEST_VERBOSE("invalid configuration for disk"); goto cleanup; } @@ -229,11 +229,11 @@ testQemuDiskXMLToProps(const void *opaque) !(storageSrcOnlyProps = qemuBlockStorageSourceGetBackendProps(n, false, true, true)) || !(storageProps = qemuBlockStorageSourceGetBackendProps(n, false, false, true))) { if (!data->fail) { - VIR_TEST_VERBOSE("failed to generate qemu blockdev props\n"); + VIR_TEST_VERBOSE("failed to generate qemu blockdev props"); goto cleanup; } } else if (data->fail) { - VIR_TEST_VERBOSE("qemu blockdev props should have failed\n"); + VIR_TEST_VERBOSE("qemu blockdev props should have failed"); goto cleanup; } @@ -531,7 +531,7 @@ mymain(void) if (!(capslatest_x86_64 = testQemuGetLatestCapsForArch("x86_64", "xml"))) return EXIT_FAILURE; - VIR_TEST_VERBOSE("\nlatest caps x86_64: %s\n", capslatest_x86_64); + VIR_TEST_VERBOSE("\nlatest caps x86_64: %s", capslatest_x86_64); if (!(caps_x86_64 = qemuTestParseCapabilitiesArch(virArchFromString("x86_64"), capslatest_x86_64))) @@ -641,7 +641,7 @@ mymain(void) diskxmljsondata.schema, &diskxmljsondata.schemaroot) < 0 || !diskxmljsondata.schemaroot) { - VIR_TEST_VERBOSE("failed to find schema entry for blockdev-add\n"); + VIR_TEST_VERBOSE("failed to find schema entry for blockdev-add"); ret = -1; goto cleanup; } diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 6ad67c8902..b6aad330a9 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -131,7 +131,7 @@ testQemuHotplugAttach(virDomainObjPtr vm, ret = qemuDomainAttachWatchdog(&driver, vm, dev->data.watchdog); break; default: - VIR_TEST_VERBOSE("device type '%s' cannot be attached\n", + VIR_TEST_VERBOSE("device type '%s' cannot be attached", virDomainDeviceTypeToString(dev->type)); break; } @@ -154,7 +154,7 @@ testQemuHotplugDetach(virDomainObjPtr vm, ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async); break; default: - VIR_TEST_VERBOSE("device type '%s' cannot be detached\n", + VIR_TEST_VERBOSE("device type '%s' cannot be detached", virDomainDeviceTypeToString(dev->type)); break; } @@ -178,7 +178,7 @@ testQemuHotplugUpdate(virDomainObjPtr vm, ret = qemuDomainChangeGraphics(&driver, vm, dev->data.graphics); break; default: - VIR_TEST_VERBOSE("device type '%s' cannot be updated\n", + VIR_TEST_VERBOSE("device type '%s' cannot be updated", virDomainDeviceTypeToString(dev->type)); break; } @@ -203,7 +203,7 @@ testQemuHotplugCheckResult(virDomainObjPtr vm, if (STREQ(expected, actual)) { if (fail) - VIR_TEST_VERBOSE("domain XML should not match the expected result\n"); + VIR_TEST_VERBOSE("domain XML should not match the expected result"); ret = 0; } else { if (!fail) @@ -262,7 +262,7 @@ testQemuHotplug(const void *data) if (test->vm) { vm = test->vm; if (!vm->def) { - VIR_TEST_VERBOSE("test skipped due to failure of dependent test\n"); + VIR_TEST_VERBOSE("test skipped due to failure of dependent test"); goto cleanup; } } else { diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 522ff5a3d5..57b6e02b6e 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1643,7 +1643,7 @@ static void testQemuMonitorJSONGetBlockInfoPrint(const struct qemuDomainDiskInfo *d) { VIR_TEST_VERBOSE("removable: %d, tray: %d, tray_open: %d, empty: %d, " - "io_status: %d, nodename: '%s'\n", + "io_status: %d, nodename: '%s'", d->removable, d->tray, d->tray_open, d->empty, d->io_status, NULLSTR(d->nodename)); } @@ -2905,7 +2905,7 @@ testQAPISchemaValidate(const void *opaque) if ((testQEMUSchemaValidate(json, schemaroot, data->schema, &debug) == 0) != data->success) { if (!data->success) - VIR_TEST_VERBOSE("\nschema validation should have failed\n"); + VIR_TEST_VERBOSE("\nschema validation should have failed"); } else { ret = 0; } @@ -3026,7 +3026,7 @@ mymain(void) virEventRegisterDefaultImpl(); if (!(qapiData.schema = testQEMUSchemaLoad())) { - VIR_TEST_VERBOSE("failed to load qapi schema\n"); + VIR_TEST_VERBOSE("failed to load qapi schema"); ret = -1; goto cleanup; } @@ -3267,7 +3267,7 @@ mymain(void) if (!(metaschema = testQEMUSchemaGetLatest()) || !(metaschemastr = virJSONValueToString(metaschema, false))) { - VIR_TEST_VERBOSE("failed to load latest qapi schema\n"); + VIR_TEST_VERBOSE("failed to load latest qapi schema"); ret = -1; goto cleanup; } diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 76db2494bd..8c3cb29c41 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -329,7 +329,7 @@ testSELinuxLabeling(const void *opaque) } VIR_FREE(files); if (ret < 0) - VIR_TEST_VERBOSE("%s\n", virGetLastErrorMessage()); + VIR_TEST_VERBOSE("%s", virGetLastErrorMessage()); return ret; } @@ -349,7 +349,7 @@ mymain(void) if (!(mgr = virSecurityManagerNew("selinux", "QEMU", VIR_SECURITY_MANAGER_DEFAULT_CONFINED | VIR_SECURITY_MANAGER_PRIVILEGED))) { - VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n", + VIR_TEST_VERBOSE("Unable to initialize security driver: %s", virGetLastErrorMessage()); return EXIT_FAILURE; } diff --git a/tests/testutils.h b/tests/testutils.h index cfc60084b2..2d0cea7826 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -97,8 +97,10 @@ unsigned int virTestGetRegenerate(void); #define VIR_TEST_VERBOSE(...) \ do { \ - if (virTestGetVerbose()) \ + if (virTestGetVerbose()) { \ fprintf(stderr, __VA_ARGS__); \ + fprintf(stderr, "\n"); \ + } \ } while (0) char *virTestLogContentAndReset(void); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index cd04606cf0..41a7451fa1 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -851,7 +851,7 @@ testQemuGetLatestCapsForArch(const char *arch, goto cleanup; if (!maxname) { - VIR_TEST_VERBOSE("failed to find capabilities for '%s' in '%s'\n", + VIR_TEST_VERBOSE("failed to find capabilities for '%s' in '%s'", arch, TEST_QEMU_CAPS_PATH); goto cleanup; } @@ -882,7 +882,7 @@ testQemuGetLatestCaps(void) if (!(capslatest = virHashCreate(4, virHashValueFree))) goto error; - VIR_TEST_VERBOSE("\n"); + VIR_TEST_VERBOSE(NULL); for (i = 0; i < ARRAY_CARDINALITY(archs); ++i) { char *cap = testQemuGetLatestCapsForArch(archs[i], "xml"); @@ -890,10 +890,11 @@ testQemuGetLatestCaps(void) if (!cap || virHashAddEntry(capslatest, archs[i], cap) < 0) goto error; - VIR_TEST_VERBOSE("latest caps for %s: %s\n", archs[i], cap); + VIR_TEST_VERBOSE("latest caps for %s: %s", archs[i], cap); } - VIR_TEST_VERBOSE("\n"); + VIR_TEST_VERBOSE(NULL); + return capslatest; error: diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index 0728771bfe..3773f63ecb 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -534,7 +534,7 @@ testQEMUSchemaGetLatest(void) virJSONValuePtr schema = NULL; if (!(capsLatestFile = testQemuGetLatestCapsForArch("x86_64", "replies"))) { - VIR_TEST_VERBOSE("failed to find latest caps replies\n"); + VIR_TEST_VERBOSE("failed to find latest caps replies"); return NULL; } @@ -546,7 +546,7 @@ testQEMUSchemaGetLatest(void) if (!(schemaReply = strstr(capsLatest, "\"execute\": \"query-qmp-schema\"")) || !(schemaReply = strstr(schemaReply, "\n\n")) || !(end = strstr(schemaReply + 2, "\n\n"))) { - VIR_TEST_VERBOSE("failed to find reply to 'query-qmp-schema' in '%s'\n", + VIR_TEST_VERBOSE("failed to find reply to 'query-qmp-schema' in '%s'", capsLatestFile); goto cleanup; } @@ -555,13 +555,13 @@ testQEMUSchemaGetLatest(void) *end = '\0'; if (!(reply = virJSONValueFromString(schemaReply))) { - VIR_TEST_VERBOSE("failed to parse 'query-qmp-schema' reply from '%s'\n", + VIR_TEST_VERBOSE("failed to parse 'query-qmp-schema' reply from '%s'", capsLatestFile); goto cleanup; } if (!(schema = virJSONValueObjectStealArray(reply, "return"))) { - VIR_TEST_VERBOSE("missing qapi schema data in reply in '%s'\n", + VIR_TEST_VERBOSE("missing qapi schema data in reply in '%s'", capsLatestFile); goto cleanup; } diff --git a/tests/virerrortest.c b/tests/virerrortest.c index b49d1a9135..63ba84c2f9 100644 --- a/tests/virerrortest.c +++ b/tests/virerrortest.c @@ -31,11 +31,11 @@ virErrorTestMsgFormatInfoOne(const char *msg) for (next = (char *)msg; (next = strchr(next, '%')); next++) { if (next[1] != 's') { - VIR_TEST_VERBOSE("\nerror message '%s' contains disallowed printf modifiers\n", msg); + VIR_TEST_VERBOSE("\nerror message '%s' contains disallowed printf modifiers", msg); ret = -1; } else { if (found) { - VIR_TEST_VERBOSE("\nerror message '%s' contains multiple %%s modifiers\n", msg); + VIR_TEST_VERBOSE("\nerror message '%s' contains multiple %%s modifiers", msg); ret = -1; } else { found = true; @@ -44,7 +44,7 @@ virErrorTestMsgFormatInfoOne(const char *msg) } if (!found) { - VIR_TEST_VERBOSE("\nerror message '%s' does not contain any %%s modifiers\n", msg); + VIR_TEST_VERBOSE("\nerror message '%s' does not contain any %%s modifiers", msg); ret = -1; } @@ -65,17 +65,17 @@ virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED) err_info = virErrorMsg(i, ""); if (!err_noinfo) { - VIR_TEST_VERBOSE("\nmissing string without info for error id %zu\n", i); + VIR_TEST_VERBOSE("\nmissing string without info for error id %zu", i); ret = -1; } if (!err_info) { - VIR_TEST_VERBOSE("\nmissing string with info for error id %zu\n", i); + VIR_TEST_VERBOSE("\nmissing string with info for error id %zu", i); ret = -1; } if (err_noinfo && strchr(err_noinfo, '%')) { - VIR_TEST_VERBOSE("\nerror message id %zu contains formatting characters: '%s'\n", + VIR_TEST_VERBOSE("\nerror message id %zu contains formatting characters: '%s'", i, err_noinfo); ret = -1; } diff --git a/tests/virhashtest.c b/tests/virhashtest.c index 38f549848f..95634eec80 100644 --- a/tests/virhashtest.c +++ b/tests/virhashtest.c @@ -41,7 +41,7 @@ testHashInit(int size) for (i = 0; i < ARRAY_CARDINALITY(uuids); i++) { if (!virHashLookup(hash, uuids[i])) { - VIR_TEST_VERBOSE("\nentry \"%s\" could not be found\n", uuids[i]); + VIR_TEST_VERBOSE("\nentry \"%s\" could not be found", uuids[i]); virHashFree(hash); return NULL; } @@ -69,7 +69,7 @@ testHashCheckCount(virHashTablePtr hash, size_t count) size_t iter_count = 0; if (virHashSize(hash) != count) { - VIR_TEST_VERBOSE("\nhash contains %zd instead of %zu elements\n", + VIR_TEST_VERBOSE("\nhash contains %zd instead of %zu elements", virHashSize(hash), count); return -1; } @@ -77,7 +77,7 @@ testHashCheckCount(virHashTablePtr hash, size_t count) virHashForEach(hash, testHashCheckForEachCount, &iter_count); if (count != iter_count) { VIR_TEST_VERBOSE("\nhash claims to have %zu elements but iteration" - "finds %zu\n", count, iter_count); + "finds %zu", count, iter_count); return -1; } @@ -125,7 +125,7 @@ testHashUpdate(const void *data ATTRIBUTE_UNUSED) for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) { if (virHashUpdateEntry(hash, uuids_subset[i], (void *) 1) < 0) { - VIR_TEST_VERBOSE("\nentry \"%s\" could not be updated\n", + VIR_TEST_VERBOSE("\nentry \"%s\" could not be updated", uuids_subset[i]); goto cleanup; } @@ -133,7 +133,7 @@ testHashUpdate(const void *data ATTRIBUTE_UNUSED) for (i = 0; i < ARRAY_CARDINALITY(uuids_new); i++) { if (virHashUpdateEntry(hash, uuids_new[i], (void *) 1) < 0) { - VIR_TEST_VERBOSE("\nnew entry \"%s\" could not be updated\n", + VIR_TEST_VERBOSE("\nnew entry \"%s\" could not be updated", uuids_new[i]); goto cleanup; } @@ -163,7 +163,7 @@ testHashRemove(const void *data ATTRIBUTE_UNUSED) for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) { if (virHashRemoveEntry(hash, uuids_subset[i]) < 0) { - VIR_TEST_VERBOSE("\nentry \"%s\" could not be removed\n", + VIR_TEST_VERBOSE("\nentry \"%s\" could not be removed", uuids_subset[i]); goto cleanup; } @@ -257,7 +257,7 @@ testHashSteal(const void *data ATTRIBUTE_UNUSED) for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) { if (!virHashSteal(hash, uuids_subset[i])) { - VIR_TEST_VERBOSE("\nentry \"%s\" could not be stolen\n", + VIR_TEST_VERBOSE("\nentry \"%s\" could not be stolen", uuids_subset[i]); goto cleanup; } @@ -316,7 +316,7 @@ testHashRemoveSet(const void *data ATTRIBUTE_UNUSED) if (count != rcount) { VIR_TEST_VERBOSE("\nvirHashRemoveSet didn't remove expected number of" - " entries, %d != %u\n", + " entries, %d != %u", rcount, count); goto cleanup; } @@ -355,7 +355,7 @@ testHashSearch(const void *data ATTRIBUTE_UNUSED) entry = virHashSearch(hash, testHashSearchIter, NULL, NULL); if (!entry || STRNEQ(uuids_subset[testSearchIndex], entry)) { - VIR_TEST_VERBOSE("\nvirHashSearch didn't find entry '%s'\n", + VIR_TEST_VERBOSE("\nvirHashSearch didn't find entry '%s'", uuids_subset[testSearchIndex]); goto cleanup; } diff --git a/tests/virjsontest.c b/tests/virjsontest.c index dbc0b41cc8..81da89a6b0 100644 --- a/tests/virjsontest.c +++ b/tests/virjsontest.c @@ -39,7 +39,7 @@ testJSONFromFile(const void *data) if (!injson) { if (info->pass) { - VIR_TEST_VERBOSE("Failed to parse %s\n", info->doc); + VIR_TEST_VERBOSE("Failed to parse %s", info->doc); return -1; } else { VIR_TEST_DEBUG("As expected, failed to parse %s", info->doc); @@ -47,7 +47,7 @@ testJSONFromFile(const void *data) } } else { if (!info->pass) { - VIR_TEST_VERBOSE("Unexpected success while parsing %s\n", info->doc); + VIR_TEST_VERBOSE("Unexpected success while parsing %s", info->doc); return -1; } } @@ -74,7 +74,7 @@ testJSONFromString(const void *data) if (!json) { if (info->pass) { - VIR_TEST_VERBOSE("Failed to parse %s\n", info->doc); + VIR_TEST_VERBOSE("Failed to parse %s", info->doc); return -1; } else { VIR_TEST_DEBUG("As expected, failed to parse %s", info->doc); @@ -82,7 +82,7 @@ testJSONFromString(const void *data) } } else { if (!info->pass) { - VIR_TEST_VERBOSE("Unexpected success while parsing %s\n", info->doc); + VIR_TEST_VERBOSE("Unexpected success while parsing %s", info->doc); return -1; } } @@ -90,7 +90,7 @@ testJSONFromString(const void *data) VIR_TEST_DEBUG("Parsed %s", info->doc); if (!(formatted = virJSONValueToString(json, false))) { - VIR_TEST_VERBOSE("Failed to format json data\n"); + VIR_TEST_VERBOSE("Failed to format json data"); return -1; } @@ -125,14 +125,14 @@ testJSONAddRemove(const void *data) json = virJSONValueFromString(indata); if (!json) { - VIR_TEST_VERBOSE("Fail to parse %s\n", info->name); + VIR_TEST_VERBOSE("Fail to parse %s", info->name); return -1; } switch (virJSONValueObjectRemoveKey(json, "name", &name)) { case 1: if (!info->pass) { - VIR_TEST_VERBOSE("should not remove from non-object %s\n", + VIR_TEST_VERBOSE("should not remove from non-object %s", info->name); return -1; } @@ -141,29 +141,29 @@ testJSONAddRemove(const void *data) if (!info->pass) return 0; else - VIR_TEST_VERBOSE("Fail to recognize non-object %s\n", info->name); + VIR_TEST_VERBOSE("Fail to recognize non-object %s", info->name); return -1; default: - VIR_TEST_VERBOSE("unexpected result when removing from %s\n", + VIR_TEST_VERBOSE("unexpected result when removing from %s", info->name); return -1; } if (STRNEQ_NULLABLE(virJSONValueGetString(name), "sample")) { - VIR_TEST_VERBOSE("unexpected value after removing name: %s\n", + VIR_TEST_VERBOSE("unexpected value after removing name: %s", NULLSTR(virJSONValueGetString(name))); return -1; } if (virJSONValueObjectRemoveKey(json, "name", NULL)) { VIR_TEST_VERBOSE("%s", - "unexpected success when removing missing key\n"); + "unexpected success when removing missing key"); return -1; } if (virJSONValueObjectAppendString(json, "newname", "foo") < 0) { - VIR_TEST_VERBOSE("%s", "unexpected failure adding new key\n"); + VIR_TEST_VERBOSE("%s", "unexpected failure adding new key"); return -1; } if (!(actual = virJSONValueToString(json, false))) { - VIR_TEST_VERBOSE("%s", "failed to stringize result\n"); + VIR_TEST_VERBOSE("%s", "failed to stringize result"); return -1; } @@ -187,28 +187,28 @@ testJSONLookup(const void *data) json = virJSONValueFromString(info->doc); if (!json) { - VIR_TEST_VERBOSE("Fail to parse %s\n", info->doc); + VIR_TEST_VERBOSE("Fail to parse %s", info->doc); return -1; } value = virJSONValueObjectGetObject(json, "a"); if (value) { if (!info->pass) { - VIR_TEST_VERBOSE("lookup for 'a' in '%s' should have failed\n", + VIR_TEST_VERBOSE("lookup for 'a' in '%s' should have failed", info->doc); return -1; } else { result = virJSONValueToString(value, false); if (STRNEQ_NULLABLE(result, "{}")) { VIR_TEST_VERBOSE("lookup for 'a' in '%s' found '%s' but " - "should have found '{}'\n", + "should have found '{}'", info->doc, NULLSTR(result)); return -1; } VIR_FREE(result); } } else if (info->pass) { - VIR_TEST_VERBOSE("lookup for 'a' in '%s' should have succeeded\n", + VIR_TEST_VERBOSE("lookup for 'a' in '%s' should have succeeded", info->doc); return -1; } @@ -217,17 +217,17 @@ testJSONLookup(const void *data) rc = virJSONValueObjectGetNumberInt(json, "b", &number); if (rc == 0) { if (!info->pass) { - VIR_TEST_VERBOSE("lookup for 'b' in '%s' should have failed\n", + VIR_TEST_VERBOSE("lookup for 'b' in '%s' should have failed", info->doc); return -1; } else if (number != 1) { VIR_TEST_VERBOSE("lookup for 'b' in '%s' found %d but " - "should have found 1\n", + "should have found 1", info->doc, number); return -1; } } else if (info->pass) { - VIR_TEST_VERBOSE("lookup for 'b' in '%s' should have succeeded\n", + VIR_TEST_VERBOSE("lookup for 'b' in '%s' should have succeeded", info->doc); return -1; } @@ -235,16 +235,16 @@ testJSONLookup(const void *data) str = virJSONValueObjectGetString(json, "c"); if (str) { if (!info->pass) { - VIR_TEST_VERBOSE("lookup for 'c' in '%s' should have failed\n", + VIR_TEST_VERBOSE("lookup for 'c' in '%s' should have failed", info->doc); return -1; } else if (STRNEQ(str, "str")) { VIR_TEST_VERBOSE("lookup for 'c' in '%s' found '%s' but " - "should have found 'str'\n", info->doc, str); + "should have found 'str'", info->doc, str); return -1; } } else if (info->pass) { - VIR_TEST_VERBOSE("lookup for 'c' in '%s' should have succeeded\n", + VIR_TEST_VERBOSE("lookup for 'c' in '%s' should have succeeded", info->doc); return -1; } @@ -252,21 +252,21 @@ testJSONLookup(const void *data) value = virJSONValueObjectGetArray(json, "d"); if (value) { if (!info->pass) { - VIR_TEST_VERBOSE("lookup for 'd' in '%s' should have failed\n", + VIR_TEST_VERBOSE("lookup for 'd' in '%s' should have failed", info->doc); return -1; } else { result = virJSONValueToString(value, false); if (STRNEQ_NULLABLE(result, "[]")) { VIR_TEST_VERBOSE("lookup for 'd' in '%s' found '%s' but " - "should have found '[]'\n", + "should have found '[]'", info->doc, NULLSTR(result)); return -1; } VIR_FREE(result); } } else if (info->pass) { - VIR_TEST_VERBOSE("lookup for 'd' in '%s' should have succeeded\n", + VIR_TEST_VERBOSE("lookup for 'd' in '%s' should have succeeded", info->doc); return -1; } @@ -286,25 +286,25 @@ testJSONCopy(const void *data) json = virJSONValueFromString(info->doc); if (!json) { - VIR_TEST_VERBOSE("Failed to parse %s\n", info->doc); + VIR_TEST_VERBOSE("Failed to parse %s", info->doc); return -1; } jsonCopy = virJSONValueCopy(json); if (!jsonCopy) { - VIR_TEST_VERBOSE("Failed to copy JSON data\n"); + VIR_TEST_VERBOSE("Failed to copy JSON data"); return -1; } result = virJSONValueToString(json, false); if (!result) { - VIR_TEST_VERBOSE("Failed to format original JSON data\n"); + VIR_TEST_VERBOSE("Failed to format original JSON data"); return -1; } resultCopy = virJSONValueToString(json, false); if (!resultCopy) { - VIR_TEST_VERBOSE("Failed to format copied JSON data\n"); + VIR_TEST_VERBOSE("Failed to format copied JSON data"); return -1; } @@ -319,13 +319,13 @@ testJSONCopy(const void *data) result = virJSONValueToString(json, true); if (!result) { - VIR_TEST_VERBOSE("Failed to format original JSON data\n"); + VIR_TEST_VERBOSE("Failed to format original JSON data"); return -1; } resultCopy = virJSONValueToString(json, true); if (!resultCopy) { - VIR_TEST_VERBOSE("Failed to format copied JSON data\n"); + VIR_TEST_VERBOSE("Failed to format copied JSON data"); return -1; } @@ -364,7 +364,7 @@ testJSONDeflatten(const void *data) if ((deflattened = virJSONValueObjectDeflatten(injson))) { if (!info->pass) { - VIR_TEST_VERBOSE("%s: deflattening should have failed\n", info->name); + VIR_TEST_VERBOSE("%s: deflattening should have failed", info->name); return -1; } } else { @@ -450,22 +450,22 @@ testJSONObjectFormatSteal(const void *opaque ATTRIBUTE_UNUSED) } if (virJSONValueObjectCreate(&t1, "a:t", &a1, "s:f", NULL, NULL) != -1) { - VIR_TEST_VERBOSE("virJSONValueObjectCreate(t1) should have failed\n"); + VIR_TEST_VERBOSE("virJSONValueObjectCreate(t1) should have failed"); return -1; } if (a1) { - VIR_TEST_VERBOSE("appended object a1 was not consumed\n"); + VIR_TEST_VERBOSE("appended object a1 was not consumed"); return -1; } if (virJSONValueObjectCreate(&t2, "s:f", NULL, "a:t", &a1, NULL) != -1) { - VIR_TEST_VERBOSE("virJSONValueObjectCreate(t2) should have failed\n"); + VIR_TEST_VERBOSE("virJSONValueObjectCreate(t2) should have failed"); return -1; } if (!a2) { - VIR_TEST_VERBOSE("appended object a2 was consumed\n"); + VIR_TEST_VERBOSE("appended object a2 was consumed"); return -1; } -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:18PM +0200, Michal Privoznik wrote:
Similarly to the previous commit, VIR_TEST_VERBOSE should put '\n' at the end of each call so that the output is not broken.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/cputest.c | 14 +++--- tests/qemublocktest.c | 10 ++--- tests/qemuhotplugtest.c | 10 ++--- tests/qemumonitorjsontest.c | 8 ++-- tests/securityselinuxlabeltest.c | 4 +- tests/testutils.h | 4 +- tests/testutilsqemu.c | 9 ++-- tests/testutilsqemuschema.c | 8 ++-- tests/virerrortest.c | 12 +++--- tests/virhashtest.c | 18 ++++---- tests/virjsontest.c | 74 ++++++++++++++++---------------- 11 files changed, 87 insertions(+), 84 deletions(-)
diff --git a/tests/testutils.h b/tests/testutils.h index cfc60084b2..2d0cea7826 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -97,8 +97,10 @@ unsigned int virTestGetRegenerate(void);
#define VIR_TEST_VERBOSE(...) \ do { \ - if (virTestGetVerbose()) \ + if (virTestGetVerbose()) { \ fprintf(stderr, __VA_ARGS__); \ + fprintf(stderr, "\n"); \ + } \ } while (0)
char *virTestLogContentAndReset(void);
This macro is not used enough to reach the limit, but if you make the change in the first patch, please change this one as well. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhashtest.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/virhashtest.c b/tests/virhashtest.c index 95634eec80..2355e7bc63 100644 --- a/tests/virhashtest.c +++ b/tests/virhashtest.c @@ -47,9 +47,6 @@ testHashInit(int size) } } - if (size && size != virHashTableSize(hash)) - VIR_TEST_DEBUG("\n"); - return hash; } -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:19PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhashtest.c | 3 --- 1 file changed, 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

From: Michal Privoznik <miso.privoznik@gmail.com> There are few functions called from the test which return an integer but their retval is compared as if it was a pointer. Now, there is nothing wrong with that from machine POV, but from readability perspective it's wrong. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 20984f3442..2c65db2068 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -161,22 +161,22 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); - if (!virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, - &hostdevs[0], 1, 0)) + if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, + &hostdevs[0], 1, 0) == 0) goto cleanup; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); - if (!virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, - &hostdevs[1], 1, 0)) + if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, + &hostdevs[1], 1, 0) == 0) goto cleanup; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); - if (!virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, - &hostdevs[2], 1, 0)) + if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, + &hostdevs[2], 1, 0) == 0) goto cleanup; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); @@ -252,22 +252,22 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); - if (!virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, - &hostdevs[0], 1, 0)) + if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, + &hostdevs[0], 1, 0) == 0) goto cleanup; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); - if (!virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, - &hostdevs[1], 1, 0)) + if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, + &hostdevs[1], 1, 0) == 0) goto cleanup; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); - if (!virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, - &hostdevs[2], 1, 0)) + if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, + &hostdevs[2], 1, 0) == 0) goto cleanup; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:20PM +0200, Michal Privoznik wrote:
From: Michal Privoznik <miso.privoznik@gmail.com>
The e-mail here does not match the one in the signoff
There are few functions called from the test which return an integer but their retval is compared as if it was a pointer. Now, there is nothing wrong with that from machine POV, but from readability perspective it's wrong.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

From: Michal Privoznik <miso.privoznik@gmail.com> In this test there is this macro CHECK_LIST_COUNT() which checks if a list of PCI devices contains expected count. If it doesn't an error is reported and 'goto cleanup' is invoked. There's no real reason for that as even since its introduction there is no cleanup done and all 'cleanup' labels contains nothing but 'return'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 147 ++++++++++++----------------------------- 1 file changed, 42 insertions(+), 105 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 2c65db2068..713e0ff9a6 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -41,7 +41,7 @@ VIR_LOG_INIT("tests.hostdevtest"); virReportError(VIR_ERR_INTERNAL_ERROR, \ "Unexpected count of items in " #list ": %zu, " \ "expecting %zu", actualCount, (size_t) cnt); \ - goto cleanup; \ + return -1; \ } \ } while (0) @@ -132,7 +132,6 @@ myInit(void) static int testVirHostdevPreparePCIHostdevs_unmanaged(void) { - int ret = -1; size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) @@ -145,7 +144,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) VIR_DEBUG("Test 0 hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, NULL, 0, 0) < 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); @@ -153,7 +152,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) VIR_DEBUG("Test >=1 unmanaged hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs); @@ -163,35 +162,30 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0) == 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0) == 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0) == 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - ret = 0; - - cleanup: - return ret; - + return 0; } static int testVirHostdevReAttachPCIHostdevs_unmanaged(void) { - int ret = -1; size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { @@ -215,17 +209,12 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(void) CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + nhostdevs); - ret = 0; - - cleanup: - return ret; - + return 0; } static int testVirHostdevPreparePCIHostdevs_managed(bool mixed) { - int ret = -1; size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) @@ -238,7 +227,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) VIR_DEBUG("Test >=1 hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); /* If testing a mixed roundtrip, devices are already in the inactive list * before we start and are removed from it as soon as we attach them to @@ -254,35 +243,30 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0) == 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0) == 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0) == 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - ret = 0; - - cleanup: - return ret; - + return 0; } static int testVirHostdevReAttachPCIHostdevs_managed(bool mixed) { - int ret = -1; size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { @@ -311,82 +295,63 @@ testVirHostdevReAttachPCIHostdevs_managed(bool mixed) else CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - ret = 0; - - cleanup: - return ret; - + return 0; } static int testVirHostdevDetachPCINodeDevice(void) { - int ret = -1; size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceDetach(mgr, dev[i]) < 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + 1); } - ret = 0; - - cleanup: - return ret; + return 0; } static int testVirHostdevResetPCINodeDevice(void) { - int ret = -1; size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceReset(mgr, dev[i]) < 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); } - ret = 0; - - cleanup: - return ret; - + return 0; } static int testVirHostdevReAttachPCINodeDevice(void) { - int ret = -1; size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceReAttach(mgr, dev[i]) < 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - 1); } - ret = 0; - - cleanup: - return ret; - + return 0; } static int testVirHostdevUpdateActivePCIHostdevs(void) { - int ret = -1; size_t active_count, inactive_count; active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); @@ -395,21 +360,18 @@ testVirHostdevUpdateActivePCIHostdevs(void) VIR_DEBUG("Test 0 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, NULL, 0, drv_name, dom_name) < 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test >=1 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, hostdevs, nhostdevs, drv_name, dom_name) < 0) - goto cleanup; + return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - ret = 0; - - cleanup: - return ret; + return 0; } /** @@ -424,17 +386,12 @@ testVirHostdevUpdateActivePCIHostdevs(void) static int testVirHostdevRoundtripNoGuest(const void *opaque ATTRIBUTE_UNUSED) { - int ret = -1; - if (testVirHostdevDetachPCINodeDevice() < 0) - goto out; + return -1; if (testVirHostdevReAttachPCINodeDevice() < 0) - goto out; + return -1; - ret = 0; - - out: - return ret; + return 0; } /** @@ -451,21 +408,16 @@ testVirHostdevRoundtripNoGuest(const void *opaque ATTRIBUTE_UNUSED) static int testVirHostdevRoundtripUnmanaged(const void *opaque ATTRIBUTE_UNUSED) { - int ret = -1; - if (testVirHostdevDetachPCINodeDevice() < 0) - goto out; + return -1; if (testVirHostdevPreparePCIHostdevs_unmanaged() < 0) - goto out; + return -1; if (testVirHostdevReAttachPCIHostdevs_unmanaged() < 0) - goto out; + return -1; if (testVirHostdevReAttachPCINodeDevice() < 0) - goto out; + return -1; - ret = 0; - - out: - return ret; + return 0; } /** @@ -480,17 +432,12 @@ testVirHostdevRoundtripUnmanaged(const void *opaque ATTRIBUTE_UNUSED) static int testVirHostdevRoundtripManaged(const void *opaque ATTRIBUTE_UNUSED) { - int ret = -1; - if (testVirHostdevPreparePCIHostdevs_managed(false) < 0) - goto out; + return -1; if (testVirHostdevReAttachPCIHostdevs_managed(false) < 0) - goto out; + return -1; - ret = 0; - - out: - return ret; + return 0; } /** @@ -508,21 +455,16 @@ testVirHostdevRoundtripManaged(const void *opaque ATTRIBUTE_UNUSED) static int testVirHostdevRoundtripMixed(const void *opaque ATTRIBUTE_UNUSED) { - int ret = -1; - if (testVirHostdevDetachPCINodeDevice() < 0) - goto out; + return -1; if (testVirHostdevPreparePCIHostdevs_managed(true) < 0) - goto out; + return -1; if (testVirHostdevReAttachPCIHostdevs_managed(true) < 0) - goto out; + return -1; if (testVirHostdevReAttachPCINodeDevice() < 0) - goto out; + return -1; - ret = 0; - - out: - return ret; + return 0; } /** @@ -537,17 +479,12 @@ testVirHostdevRoundtripMixed(const void *opaque ATTRIBUTE_UNUSED) static int testVirHostdevOther(const void *opaque ATTRIBUTE_UNUSED) { - int ret = -1; - if (testVirHostdevResetPCINodeDevice() < 0) - goto out; + return -1; if (testVirHostdevUpdateActivePCIHostdevs() < 0) - goto out; + return -1; - ret = 0; - - out: - return ret; + return 0; } # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:21PM +0200, Michal Privoznik wrote:
From: Michal Privoznik <miso.privoznik@gmail.com>
Different e-mail.
In this test there is this macro CHECK_LIST_COUNT() which checks if a list of PCI devices contains expected count. If it doesn't an error is reported and 'goto cleanup' is invoked. There's no real reason for that as even since its introduction there is no cleanup done and all 'cleanup' labels contains nothing but 'return'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 147 ++++++++++++----------------------------- 1 file changed, 42 insertions(+), 105 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

On Mon, Aug 12, 2019 at 01:54:21PM +0200, Michal Privoznik wrote:
From: Michal Privoznik <miso.privoznik@gmail.com>
In this test there is this macro CHECK_LIST_COUNT() which checks if a list of PCI devices contains expected count. If it doesn't an error is reported and 'goto cleanup' is invoked. There's no real reason for that as even since its introduction there is no cleanup done and all 'cleanup' labels contains nothing but
s/contains/contain/
'return'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

From: Michal Privoznik <miso.privoznik@gmail.com> The virTestRun() already reports the same. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 713e0ff9a6..f6ea4c54fd 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -509,7 +509,6 @@ mymain(void) # define DO_TEST(fnc) \ do { \ - VIR_DEBUG("Testing: %s", #fnc); \ if (virTestRun(#fnc, fnc, NULL) < 0) \ ret = -1; \ } while (0) -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:22PM +0200, Michal Privoznik wrote:
From: Michal Privoznik <miso.privoznik@gmail.com>
The virTestRun() already reports the same.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

From: Michal Privoznik <miso.privoznik@gmail.com> There's no need to have VIR_DEBUG() really. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index f6ea4c54fd..248a5493f3 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -141,7 +141,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); /* Test normal functionality */ - VIR_DEBUG("Test 0 hostdevs"); + VIR_TEST_DEBUG("Test 0 hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, NULL, 0, 0) < 0) return -1; @@ -149,7 +149,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); /* Test unmanaged hostdevs */ - VIR_DEBUG("Test >=1 unmanaged hostdevs"); + VIR_TEST_DEBUG("Test >=1 unmanaged hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) return -1; @@ -159,21 +159,21 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) /* Test conflict */ active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); - VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); + VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver/domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0) == 0) return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); + VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0) == 0) return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); + VIR_TEST_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0) == 0) return -1; @@ -190,7 +190,7 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(void) for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != false) { - VIR_DEBUG("invalid test"); + VIR_TEST_DEBUG("invalid test"); return -1; } } @@ -198,12 +198,12 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(void) active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); - VIR_DEBUG("Test 0 hostdevs"); + VIR_TEST_DEBUG("Test 0 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, NULL, 0, NULL); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - VIR_DEBUG("Test >=1 unmanaged hostdevs"); + VIR_TEST_DEBUG("Test >=1 unmanaged hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); @@ -224,7 +224,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); /* Test normal functionality */ - VIR_DEBUG("Test >=1 hostdevs"); + VIR_TEST_DEBUG("Test >=1 hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) return -1; @@ -240,21 +240,21 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) /* Test conflict */ active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); - VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); + VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver/domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0) == 0) return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); + VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0) == 0) return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); + VIR_TEST_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0) == 0) return -1; @@ -271,7 +271,7 @@ testVirHostdevReAttachPCIHostdevs_managed(bool mixed) for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != true) { - VIR_DEBUG("invalid test"); + VIR_TEST_DEBUG("invalid test"); return -1; } } @@ -279,12 +279,12 @@ testVirHostdevReAttachPCIHostdevs_managed(bool mixed) active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); - VIR_DEBUG("Test 0 hostdevs"); + VIR_TEST_DEBUG("Test 0 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, NULL, 0, NULL); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - VIR_DEBUG("Test >=1 hostdevs"); + VIR_TEST_DEBUG("Test >=1 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); @@ -357,14 +357,14 @@ testVirHostdevUpdateActivePCIHostdevs(void) active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); - VIR_DEBUG("Test 0 hostdevs"); + VIR_TEST_DEBUG("Test 0 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, NULL, 0, drv_name, dom_name) < 0) return -1; CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); - VIR_DEBUG("Test >=1 hostdevs"); + VIR_TEST_DEBUG("Test >=1 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, hostdevs, nhostdevs, drv_name, dom_name) < 0) return -1; -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:23PM +0200, Michal Privoznik wrote:
From: Michal Privoznik <miso.privoznik@gmail.com>
E-mail.
There's no need to have VIR_DEBUG() really.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

If a libvirt error occurred during a test, then virTestRun() reports it (regardless of test returning success or failure). For instance, in this specific case, a hostdev is detached twice and the second attempt is expected to fail. It does fail and libvirt error is reported which is then printed onto stderr. Insert virResetLastError() calls on appropriate places to avoid that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 248a5493f3..b64ba58923 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -163,6 +163,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0) == 0) return -1; + virResetLastError(); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); @@ -170,6 +171,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0) == 0) return -1; + virResetLastError(); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); @@ -177,6 +179,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0) == 0) return -1; + virResetLastError(); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); @@ -244,6 +247,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0) == 0) return -1; + virResetLastError(); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); @@ -251,6 +255,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0) == 0) return -1; + virResetLastError(); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); @@ -258,6 +263,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed) if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0) == 0) return -1; + virResetLastError(); CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:24PM +0200, Michal Privoznik wrote:
If a libvirt error occurred during a test, then virTestRun() reports it (regardless of test returning success or failure). For instance, in this specific case, a hostdev is detached twice and the second attempt is expected to fail. It does fail and libvirt error is reported which is then printed onto stderr. Insert virResetLastError() calls on appropriate places to avoid that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

If a test fails, it's stderr is caught in the logs. That's how our CI works. But virReportError() is not. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virhostdevtest.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index b64ba58923..209cde5419 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -38,9 +38,8 @@ VIR_LOG_INIT("tests.hostdevtest"); do { \ size_t actualCount; \ if ((actualCount = cb(list)) != cnt) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, \ - "Unexpected count of items in " #list ": %zu, " \ - "expecting %zu", actualCount, (size_t) cnt); \ + fprintf(stderr, "Unexpected count of items in " #list ": %zu, " \ + "expecting %zu", actualCount, (size_t) cnt); \ return -1; \ } \ } while (0) -- 2.21.0

On Mon, Aug 12, 2019 at 01:54:25PM +0200, Michal Privoznik wrote:
If a test fails, it's stderr is caught in the logs. That's how our CI works. But virReportError() is not.
Where are you missing the error message? If you run the tested function through virTestRun, it calls virDispatchError which should log the error reported via virReportError to stderr If I adjust the test: diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 46627355c3..5b9799399f 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -157,8 +157,8 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void) CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs); /* Test conflict */ - active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); - inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs) + 1; + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs) - 1; VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver/domain again"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0) == 0) I can see the error messsage: $ VIR_TEST_DEBUG=1 ./run tests/virhostdevtest TEST: virhostdevtest 1) testVirHostdevRoundtripNoGuest ... OK 2) testVirHostdevRoundtripUnmanaged ... Test 0 hostdevs Test >=1 unmanaged hostdevs Test: prepare same hostdevs for same driver/domain again libvirt: error : internal error: Unexpected count of items in mgr->activePCIHostdevs: 3, expecting 4 FAILED 3) testVirHostdevRoundtripManaged ... Test >=1 hostdevs libvirt: error : Requested operation is not valid: PCI device 0000:00:01.0 is in use by driver test_driver, domain test_domain FAILED 4) testVirHostdevRoundtripMixed ... libvirt: error : Requested operation is not valid: PCI device 0000:00:01.0 is in use by driver test_driver, domain test_domain FAILED 5) testVirHostdevOther ... libvirt: error : internal error: Not resetting active device 0000:00:01.0 FAILED Jano

On 8/26/19 2:38 PM, Ján Tomko wrote:
On Mon, Aug 12, 2019 at 01:54:25PM +0200, Michal Privoznik wrote:
If a test fails, it's stderr is caught in the logs. That's how our CI works. But virReportError() is not.
Where are you missing the error message? If you run the tested function through virTestRun, it calls virDispatchError which should log the error reported via virReportError to stderr
Huh, that's a good question. I don't recall now. I'll drop this patch then. Thanks for the review. Michal
participants (2)
-
Ján Tomko
-
Michal Privoznik