The function returns gboolean. Compare against the FALSE value from GLib. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; Fixes: 2c3353242337bb50fe5abc9454fd5fc98236d4ef --- tests/testutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testutils.c b/tests/testutils.c index 0cf0ac7e5c..83daed8940 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -836,7 +836,7 @@ virTestSetEnvPath(void) } if (new_path && - g_setenv("PATH", new_path, TRUE) < 0) + g_setenv("PATH", new_path, TRUE) == FALSE) goto cleanup; ret = 0; -- 2.21.1

Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- tests/testutils.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index 83daed8940..75cba98eb3 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -279,7 +279,7 @@ virTestLoadFileGetPath(const char *p, char * virTestLoadFilePath(const char *p, ...) { - char *path = NULL; + g_autofree char *path = NULL; char *ret = NULL; va_list ap; @@ -292,7 +292,6 @@ virTestLoadFilePath(const char *p, ...) cleanup: va_end(ap); - VIR_FREE(path); return ret; } @@ -309,8 +308,8 @@ virJSONValuePtr virTestLoadFileJSON(const char *p, ...) { virJSONValuePtr ret = NULL; - char *jsonstr = NULL; - char *path = NULL; + g_autofree char *jsonstr = NULL; + g_autofree char *path = NULL; va_list ap; va_start(ap, p); @@ -326,8 +325,6 @@ virTestLoadFileJSON(const char *p, ...) cleanup: va_end(ap); - VIR_FREE(jsonstr); - VIR_FREE(path); return ret; } @@ -420,7 +417,7 @@ static int virTestRewrapFile(const char *filename) { int ret = -1; - char *script = NULL; + g_autofree char *script = NULL; virCommandPtr cmd = NULL; if (!(virStringHasSuffix(filename, ".args") || @@ -435,7 +432,6 @@ virTestRewrapFile(const char *filename) ret = 0; cleanup: - VIR_FREE(script); virCommandFree(cmd); return ret; } @@ -669,8 +665,8 @@ virTestCompareToFile(const char *actual, const char *filename) { int ret = -1; - char *filecontent = NULL; - char *fixedcontent = NULL; + g_autofree char *filecontent = NULL; + g_autofree char *fixedcontent = NULL; const char *cmpcontent = actual; if (!cmpcontent) @@ -700,8 +696,6 @@ virTestCompareToFile(const char *actual, ret = 0; failure: - VIR_FREE(fixedcontent); - VIR_FREE(filecontent); return ret; } @@ -826,7 +820,7 @@ virTestSetEnvPath(void) { int ret = -1; const char *path = getenv("PATH"); - char *new_path = NULL; + g_autofree char *new_path = NULL; if (path) { if (strstr(path, abs_builddir) != path) @@ -841,7 +835,6 @@ virTestSetEnvPath(void) ret = 0; cleanup: - VIR_FREE(new_path); return ret; } @@ -1045,15 +1038,13 @@ virCapsPtr virTestGenericCapsInit(void) if (virTestGetDebug() > 1) { - char *caps_str; + g_autofree char *caps_str = NULL; caps_str = virCapabilitiesFormatXML(caps); if (!caps_str) goto error; VIR_TEST_DEBUG("Generic driver capabilities:\n%s", caps_str); - - VIR_FREE(caps_str); } return caps; @@ -1131,7 +1122,7 @@ testCompareDomXML2XMLFiles(virCapsPtr caps G_GNUC_UNUSED, unsigned int parseFlags, testCompareDomXML2XMLResult expectResult) { - char *actual = NULL; + g_autofree char *actual = NULL; int ret = -1; testCompareDomXML2XMLResult result; virDomainDefPtr def = NULL; @@ -1184,7 +1175,6 @@ testCompareDomXML2XMLFiles(virCapsPtr caps G_GNUC_UNUSED, expectResult, result); } - VIR_FREE(actual); virDomainDefFree(def); return ret; } -- 2.21.1

Use g_autoptr where possible. virTestCapsBuildNUMATopology is not converted completely, because while the VIR_FREE call on cell_cpus is technically wrong, neither VIR_ALLOC_N nor virBitmapNew can return an allocation error now so it is effectively dead code. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- tests/testutils.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index 75cba98eb3..e9431b9b5f 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -418,7 +418,7 @@ virTestRewrapFile(const char *filename) { int ret = -1; g_autofree char *script = NULL; - virCommandPtr cmd = NULL; + g_autoptr(virCommand) cmd = NULL; if (!(virStringHasSuffix(filename, ".args") || virStringHasSuffix(filename, ".ldargs"))) @@ -432,7 +432,6 @@ virTestRewrapFile(const char *filename) ret = 0; cleanup: - virCommandFree(cmd); return ret; } @@ -1001,7 +1000,7 @@ void virTestClearCommandPath(char *cmdset) virCapsPtr virTestGenericCapsInit(void) { - virCapsPtr caps; + g_autoptr(virCaps) caps = NULL; virCapsGuestPtr guest; if ((caps = virCapabilitiesNew(VIR_ARCH_X86_64, @@ -1047,10 +1046,9 @@ virCapsPtr virTestGenericCapsInit(void) VIR_TEST_DEBUG("Generic driver capabilities:\n%s", caps_str); } - return caps; + return g_steal_pointer(&caps); error: - virObjectUnref(caps); return NULL; } @@ -1066,7 +1064,7 @@ virCapsPtr virTestGenericCapsInit(void) virCapsHostNUMAPtr virTestCapsBuildNUMATopology(int seq) { - virCapsHostNUMAPtr caps = virCapabilitiesHostNUMANew(); + g_autoptr(virCapsHostNUMA) caps = virCapabilitiesHostNUMANew(); virCapsHostNUMACellCPUPtr cell_cpus = NULL; int core_id, cell_id; int id; @@ -1096,10 +1094,9 @@ virTestCapsBuildNUMATopology(int seq) cell_cpus = NULL; } - return caps; + return g_steal_pointer(&caps); error: - virCapabilitiesHostNUMAUnref(caps); VIR_FREE(cell_cpus); return NULL; } -- 2.21.1

The cleanups made some labels redundant. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- tests/testutils.c | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index e9431b9b5f..a7a8a19fff 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -416,7 +416,6 @@ virTestCaptureProgramOutput(const char *const argv[] G_GNUC_UNUSED, static int virTestRewrapFile(const char *filename) { - int ret = -1; g_autofree char *script = NULL; g_autoptr(virCommand) cmd = NULL; @@ -428,11 +427,9 @@ virTestRewrapFile(const char *filename) cmd = virCommandNewArgList(PYTHON, script, "--in-place", filename, NULL); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } /** @@ -663,7 +660,6 @@ int virTestCompareToFile(const char *actual, const char *filename) { - int ret = -1; g_autofree char *filecontent = NULL; g_autofree char *fixedcontent = NULL; const char *cmpcontent = actual; @@ -672,7 +668,7 @@ virTestCompareToFile(const char *actual, cmpcontent = ""; if (virTestLoadFile(filename, &filecontent) < 0 && !virTestGetRegenerate()) - goto failure; + return -1; if (filecontent) { size_t filecontentLen = strlen(filecontent); @@ -690,12 +686,10 @@ virTestCompareToFile(const char *actual, virTestDifferenceFull(stderr, filecontent, filename, cmpcontent, NULL); - goto failure; + return -1; } - ret = 0; - failure: - return ret; + return 0; } int @@ -817,7 +811,6 @@ virTestGetRegenerate(void) static int virTestSetEnvPath(void) { - int ret = -1; const char *path = getenv("PATH"); g_autofree char *new_path = NULL; @@ -830,11 +823,9 @@ virTestSetEnvPath(void) if (new_path && g_setenv("PATH", new_path, TRUE) == FALSE) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } int virTestMain(int argc, @@ -1010,30 +1001,30 @@ virCapsPtr virTestGenericCapsInit(void) if ((guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_I686, "/usr/bin/acme-virt", NULL, 0, NULL)) == NULL) - goto error; + return NULL; if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_TEST, NULL, NULL, 0, NULL)) - goto error; + return NULL; if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL)) - goto error; + return NULL; if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, NULL, NULL, 0, NULL)) - goto error; + return NULL; if ((guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64, "/usr/bin/acme-virt", NULL, 0, NULL)) == NULL) - goto error; + return NULL; if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_TEST, NULL, NULL, 0, NULL)) - goto error; + return NULL; if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL)) - goto error; + return NULL; if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, NULL, NULL, 0, NULL)) - goto error; + return NULL; if (virTestGetDebug() > 1) { @@ -1041,15 +1032,12 @@ virCapsPtr virTestGenericCapsInit(void) caps_str = virCapabilitiesFormatXML(caps); if (!caps_str) - goto error; + return NULL; VIR_TEST_DEBUG("Generic driver capabilities:\n%s", caps_str); } return g_steal_pointer(&caps); - - error: - return NULL; } -- 2.21.1

Use g_autofree and get rid of the cleanup label. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- tests/virshtest.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index 189238b826..83675710ea 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -60,26 +60,20 @@ static int testCompareOutputLit(const char *expectData, const char *filter, const char *const argv[]) { - int result = -1; - char *actualData = NULL; + g_autofree char *actualData = NULL; if (virTestCaptureProgramOutput(argv, &actualData, 4096) < 0) - goto cleanup; + return -1; if (filter && testFilterLine(actualData, filter) < 0) - goto cleanup; + return -1; if (STRNEQ(expectData, actualData)) { virTestDifference(stderr, expectData, actualData); - goto cleanup; + return -1; } - result = 0; - - cleanup: - VIR_FREE(actualData); - - return result; + return 0; } # define VIRSH_DEFAULT abs_top_builddir "/tools/virsh", \ -- 2.21.1

Our virCommand helper API already has the ability to capture program output, there's no need to open-code it. Apart from simplifying the code, the test is marginally faster due to recent improvements in virCommandMassClose. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- tests/virshtest.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index 83675710ea..add33215b7 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -5,6 +5,7 @@ #include "internal.h" #include "virxml.h" #include "testutils.h" +#include "vircommand.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -61,10 +62,26 @@ testCompareOutputLit(const char *expectData, const char *filter, const char *const argv[]) { g_autofree char *actualData = NULL; + const char *empty = ""; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *errbuf = NULL; - if (virTestCaptureProgramOutput(argv, &actualData, 4096) < 0) + if (!(cmd = virCommandNewArgs(argv))) return -1; + virCommandAddEnvString(cmd, "LANG=C"); + virCommandSetInputBuffer(cmd, empty); + virCommandSetOutputBuffer(cmd, &actualData); + virCommandSetErrorBuffer(cmd, &errbuf); + + if (virCommandRun(cmd, NULL) < 0) + return -1; + + if (STRNEQ(errbuf, "")) { + fprintf(stderr, "Command reported error: %s", errbuf); + return -1; + } + if (filter && testFilterLine(actualData, filter) < 0) return -1; -- 2.21.1

Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- tests/testutils.c | 84 ----------------------------------------------- tests/testutils.h | 2 -- 2 files changed, 86 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index a7a8a19fff..8326602c9c 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -329,90 +329,6 @@ virTestLoadFileJSON(const char *p, ...) } -#ifndef WIN32 -static -void virTestCaptureProgramExecChild(const char *const argv[], - int pipefd) -{ - size_t i; - int open_max; - int stdinfd = -1; - const char *const env[] = { - "LANG=C", - NULL - }; - - if ((stdinfd = open("/dev/null", O_RDONLY)) < 0) - goto cleanup; - - open_max = sysconf(_SC_OPEN_MAX); - if (open_max < 0) - goto cleanup; - - for (i = 0; i < open_max; i++) { - if (i != stdinfd && - i != pipefd) { - int tmpfd; - tmpfd = i; - VIR_FORCE_CLOSE(tmpfd); - } - } - - if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) - goto cleanup; - if (dup2(pipefd, STDOUT_FILENO) != STDOUT_FILENO) - goto cleanup; - if (dup2(pipefd, STDERR_FILENO) != STDERR_FILENO) - goto cleanup; - - /* SUS is crazy here, hence the cast */ - execve(argv[0], (char *const*)argv, (char *const*)env); - - cleanup: - VIR_FORCE_CLOSE(stdinfd); -} - -int -virTestCaptureProgramOutput(const char *const argv[], char **buf, int maxlen) -{ - int pipefd[2]; - int len; - - if (virPipeQuiet(pipefd) < 0) - return -1; - - pid_t pid = fork(); - switch (pid) { - case 0: - VIR_FORCE_CLOSE(pipefd[0]); - virTestCaptureProgramExecChild(argv, pipefd[1]); - - VIR_FORCE_CLOSE(pipefd[1]); - _exit(EXIT_FAILURE); - - case -1: - return -1; - - default: - VIR_FORCE_CLOSE(pipefd[1]); - len = virFileReadLimFD(pipefd[0], maxlen, buf); - VIR_FORCE_CLOSE(pipefd[0]); - if (virProcessWait(pid, NULL, false) < 0) - return -1; - - return len; - } -} -#else /* !WIN32 */ -int -virTestCaptureProgramOutput(const char *const argv[] G_GNUC_UNUSED, - char **buf G_GNUC_UNUSED, - int maxlen G_GNUC_UNUSED) -{ - return -1; -} -#endif /* !WIN32 */ - static int virTestRewrapFile(const char *filename) { diff --git a/tests/testutils.h b/tests/testutils.h index c1b365ab0d..3f32aa7200 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -46,8 +46,6 @@ char *virTestLoadFilePath(const char *p, ...) virJSONValuePtr virTestLoadFileJSON(const char *p, ...) G_GNUC_NULL_TERMINATED; -int virTestCaptureProgramOutput(const char *const argv[], char **buf, int maxlen); - void virTestClearCommandPath(char *cmdset); int virTestDifference(FILE *stream, -- 2.21.1