[libvirt PATCH 0/7] virshtest: remove virTestCaptureProgramOutput
Use virCommand instead of open-coding it and do some other cleanups found along the way. Ján Tomko (7): testutils: check return value of g_setenv testutils: use g_autofree testutils: use g_autoptr testutils: remove unnecessary labels virshtest: refactor testCompareOutputLit virshtest: use virCommand instead of custom impl testutils: remove now unused virTestCaptureProgramOutput tests/testutils.c | 171 +++++++++------------------------------------- tests/testutils.h | 2 - tests/virshtest.c | 35 ++++++---- 3 files changed, 54 insertions(+), 154 deletions(-) -- 2.21.1
The function returns gboolean. Compare against the FALSE value from GLib. Signed-off-by: Ján Tomko <jtomko@redhat.com> 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
On Sun, Feb 09, 2020 at 02:32:31AM +0100, Ján Tomko wrote:
The function returns gboolean. Compare against the FALSE value from GLib.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 2c3353242337bb50fe5abc9454fd5fc98236d4ef --- tests/testutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- 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
On Sun, Feb 09, 2020 at 02:32:32AM +0100, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/testutils.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@redhat.com> --- 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
On Sun, Feb 09, 2020 at 02:32:33AM +0100, Ján Tomko wrote:
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@redhat.com> --- tests/testutils.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The cleanups made some labels redundant. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- 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
On Sun, Feb 09, 2020 at 02:32:34AM +0100, Ján Tomko wrote:
The cleanups made some labels redundant.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/testutils.c | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Use g_autofree and get rid of the cleanup label. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- 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
On Sun, Feb 09, 2020 at 02:32:35AM +0100, Ján Tomko wrote:
Use g_autofree and get rid of the cleanup label.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/virshtest.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@redhat.com> --- 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
On Sun, Feb 09, 2020 at 02:32:36AM +0100, Ján Tomko wrote:
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@redhat.com> --- 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; + }
The current method merges stdout and stderr into the same 'actualData' buffer, so this impl is not functionally the same. That said, assuming any commands we run don't write to stderr, I think this impl is better. Can you just mention that this is an intended change in the commit message to remind us, in case we have a surprise later. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- 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
On Sun, Feb 09, 2020 at 02:32:37AM +0100, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/testutils.c | 84 ----------------------------------------------- tests/testutils.h | 2 -- 2 files changed, 86 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Sun, Feb 09, 2020 at 02:32:30AM +0100, Ján Tomko wrote:
Use virCommand instead of open-coding it and do some other cleanups found along the way.
Ohh, great, I thought about this when I did the FreeBSD mass close patch, but didn't fancy tackling it myself yet. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Ján Tomko