[libvirt] [PATCH 0/5] fix qemumonitorjsontest
I hit a segv in qemumonitorjsontest on a new machine, and traced it to through several layers of issues, including not having yajl-devel installed on my end. This series fixes the test failure, and several incidental things that I found along the way. Eric Blake (5): qemu: don't override earlier json error qemu: minor monitor lock cleanups tests: avoid segfault if json monitor not present tests: don't test json when not compiled in tests: uniformly report test failures cfg.mk | 7 +++++++ src/qemu/qemu_agent.c | 13 ++++++------- src/qemu/qemu_monitor.c | 12 ++++-------- src/qemu/qemu_monitor_json.c | 8 +++----- tests/commandhelper.c | 7 +++---- tests/qemumonitorjsontest.c | 14 ++++++++------ tests/qemumonitortestutils.c | 4 ++++ tests/seclabeltest.c | 8 ++++---- tests/securityselinuxlabeltest.c | 6 +++--- tests/securityselinuxtest.c | 6 +++--- tests/testutils.c | 8 ++++---- tests/testutils.h | 4 ++-- 12 files changed, 51 insertions(+), 46 deletions(-) -- 1.8.1.2
I built without json support, and noticed a strange failure message in qemumonitorjsontest: 2013-02-22 16:12:37.503+0000: 19812: error : virJSONValueToString:1119 : internal error No JSON parser implementation is available 2013-02-22 16:12:37.503+0000: 19812: error : qemuMonitorJSONCommandWithFd:253 : out of memory While a later patch will fix the test to skip when json is not present, this patch avoids overriding the more useful error message from virJSONValueToString returning NULL. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONCommandWithFd): Don't override message. (qemuMonitorJSONCheckError): Don't print NULL. * src/qemu/qemu_agent.c (qemuAgentCommand): Don't override message. (qemuAgentCheckError): Don't print NULL. (qemuAgentArbitraryCommand): Properly fail on OOM. --- src/qemu/qemu_agent.c | 13 ++++++------- src/qemu/qemu_monitor_json.c | 8 +++----- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index ebe777b..3e26cf1 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1,7 +1,7 @@ /* * qemu_agent.h: interaction with QEMU guest agent * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -985,10 +985,8 @@ qemuAgentCommand(qemuAgentPtr mon, memset(&msg, 0, sizeof(msg)); - if (!(cmdstr = virJSONValueToString(cmd, false))) { - virReportOOMError(); + if (!(cmdstr = virJSONValueToString(cmd, false))) goto cleanup; - } if (virAsprintf(&msg.txBuffer, "%s" LINE_ENDING, cmdstr) < 0) { virReportOOMError(); goto cleanup; @@ -1104,7 +1102,7 @@ qemuAgentCheckError(virJSONValuePtr cmd, /* Log the full JSON formatted command & error */ VIR_DEBUG("unable to execute QEMU agent command %s: %s", - cmdstr, replystr); + NULLSTR(cmdstr), NULLSTR(replystr)); /* Only send the user the command name + friendly error */ if (!error) @@ -1125,7 +1123,7 @@ qemuAgentCheckError(virJSONValuePtr cmd, char *replystr = virJSONValueToString(reply, false); VIR_DEBUG("Neither 'return' nor 'error' is set in the JSON reply %s: %s", - cmdstr, replystr); + NULLSTR(cmdstr), NULLSTR(replystr)); virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to execute QEMU agent command '%s'"), qemuAgentCommandName(cmd)); @@ -1420,7 +1418,8 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon, if (ret == 0) { ret = qemuAgentCheckError(cmd, reply); - *result = virJSONValueToString(reply, false); + if (!(*result = virJSONValueToString(reply, false))) + ret = -1; } virJSONValueFree(cmd); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f712321..b950b87 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -249,10 +249,8 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon, } } - if (!(cmdstr = virJSONValueToString(cmd, false))) { - virReportOOMError(); + if (!(cmdstr = virJSONValueToString(cmd, false))) goto cleanup; - } if (virAsprintf(&msg.txBuffer, "%s\r\n", cmdstr) < 0) { virReportOOMError(); goto cleanup; @@ -339,7 +337,7 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, /* Log the full JSON formatted command & error */ VIR_DEBUG("unable to execute QEMU command %s: %s", - cmdstr, replystr); + NULLSTR(cmdstr), NULLSTR(replystr)); /* Only send the user the command name + friendly error */ if (!error) @@ -360,7 +358,7 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, char *replystr = virJSONValueToString(reply, false); VIR_DEBUG("Neither 'return' nor 'error' is set in the JSON reply %s: %s", - cmdstr, replystr); + NULLSTR(cmdstr), NULLSTR(replystr)); virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to execute QEMU command '%s'"), qemuMonitorJSONCommandName(cmd)); -- 1.8.1.2
On 23.02.2013 00:09, Eric Blake wrote:
I built without json support, and noticed a strange failure message in qemumonitorjsontest:
2013-02-22 16:12:37.503+0000: 19812: error : virJSONValueToString:1119 : internal error No JSON parser implementation is available 2013-02-22 16:12:37.503+0000: 19812: error : qemuMonitorJSONCommandWithFd:253 : out of memory
While a later patch will fix the test to skip when json is not present, this patch avoids overriding the more useful error message from virJSONValueToString returning NULL.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONCommandWithFd): Don't override message. (qemuMonitorJSONCheckError): Don't print NULL. * src/qemu/qemu_agent.c (qemuAgentCommand): Don't override message. (qemuAgentCheckError): Don't print NULL. (qemuAgentArbitraryCommand): Properly fail on OOM. --- src/qemu/qemu_agent.c | 13 ++++++------- src/qemu/qemu_monitor_json.c | 8 +++----- 2 files changed, 9 insertions(+), 12 deletions(-)
ACK Michal
If virCondInit fails (okay, so that's unlikely), then we end up attempting a virObjectUnlock() on the cleanup path, even though we don't hold a lock. This is not guaranteed to be safe. While at it, I noticed a couple places where we were referencing mon->fd outside locks. * src/qemu/qemu_monitor.c (qemuMonitorOpenInternal): Minimize lock duration. mon-watch doesn't need clean up on error. (qemuMonitorGetBlockExtent, qemuMonitorBlockResize): Don't dereference fd outside of lock. --- src/qemu/qemu_monitor.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7f4a7a0..40eea75 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -717,7 +717,6 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, if (json) mon->wait_greeting = 1; mon->cb = cb; - virObjectLock(mon); if (virSetCloseExec(mon->fd) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -731,6 +730,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, } + virObjectLock(mon); virObjectRef(mon); if ((mon->watch = virEventAddHandle(mon->fd, VIR_EVENT_HANDLE_HANGUP | @@ -740,6 +740,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, mon, virObjectFreeCallback)) < 0) { virObjectUnref(mon); + virObjectUnlock(mon); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to register monitor events")); goto cleanup; @@ -759,11 +760,8 @@ cleanup: * so kill the callbacks now. */ mon->cb = NULL; - virObjectUnlock(mon); /* The caller owns 'fd' on failure */ mon->fd = -1; - if (mon->watch) - virEventRemoveHandle(mon->watch); qemuMonitorClose(mon); return NULL; } @@ -1508,8 +1506,7 @@ int qemuMonitorGetBlockExtent(qemuMonitorPtr mon, unsigned long long *extent) { int ret; - VIR_DEBUG("mon=%p, fd=%d, dev_name=%p", - mon, mon->fd, dev_name); + VIR_DEBUG("mon=%p, dev_name=%p", mon, dev_name); if (mon->json) ret = qemuMonitorJSONGetBlockExtent(mon, dev_name, extent); @@ -1524,8 +1521,7 @@ int qemuMonitorBlockResize(qemuMonitorPtr mon, unsigned long long size) { int ret; - VIR_DEBUG("mon=%p, fd=%d, devname=%p size=%llu", - mon, mon->fd, device, size); + VIR_DEBUG("mon=%p, devname=%p size=%llu", mon, device, size); if (mon->json) ret = qemuMonitorJSONBlockResize(mon, device, size); -- 1.8.1.2
On 23.02.2013 00:09, Eric Blake wrote:
If virCondInit fails (okay, so that's unlikely), then we end up attempting a virObjectUnlock() on the cleanup path, even though we don't hold a lock. This is not guaranteed to be safe. While at it, I noticed a couple places where we were referencing mon->fd outside locks.
* src/qemu/qemu_monitor.c (qemuMonitorOpenInternal): Minimize lock duration. mon-watch doesn't need clean up on error. (qemuMonitorGetBlockExtent, qemuMonitorBlockResize): Don't dereference fd outside of lock. --- src/qemu/qemu_monitor.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7f4a7a0..40eea75 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -717,7 +717,6 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, if (json) mon->wait_greeting = 1; mon->cb = cb; - virObjectLock(mon);
if (virSetCloseExec(mon->fd) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -731,6 +730,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, }
+ virObjectLock(mon); virObjectRef(mon); if ((mon->watch = virEventAddHandle(mon->fd, VIR_EVENT_HANDLE_HANGUP | @@ -740,6 +740,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, mon, virObjectFreeCallback)) < 0) { virObjectUnref(mon); + virObjectUnlock(mon);
Heh, it took me while to realize this is good actually. I thought it should be vice versa to prevent Unlock() being called on just freed memory. But then I realized after the Unref() mon refcount == 1.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to register monitor events")); goto cleanup; @@ -759,11 +760,8 @@ cleanup: * so kill the callbacks now. */ mon->cb = NULL; - virObjectUnlock(mon); /* The caller owns 'fd' on failure */ mon->fd = -1; - if (mon->watch) - virEventRemoveHandle(mon->watch); qemuMonitorClose(mon); return NULL; } @@ -1508,8 +1506,7 @@ int qemuMonitorGetBlockExtent(qemuMonitorPtr mon, unsigned long long *extent) { int ret; - VIR_DEBUG("mon=%p, fd=%d, dev_name=%p", - mon, mon->fd, dev_name); + VIR_DEBUG("mon=%p, dev_name=%p", mon, dev_name);
if (mon->json) ret = qemuMonitorJSONGetBlockExtent(mon, dev_name, extent); @@ -1524,8 +1521,7 @@ int qemuMonitorBlockResize(qemuMonitorPtr mon, unsigned long long size) { int ret; - VIR_DEBUG("mon=%p, fd=%d, devname=%p size=%llu", - mon, mon->fd, device, size); + VIR_DEBUG("mon=%p, devname=%p size=%llu", mon, device, size);
if (mon->json) ret = qemuMonitorJSONBlockResize(mon, device, size);
ACK Michal
On a machine without json headers, I was seeing random segfaults from qemumonitorjsontest (about 90% of the runs on my particular machine). The segfault was inside virClassIsDerivedFrom, which points to a case of a race leading to unreferencing a stale pointer to an object that had already been freed. I also noticed that if I got the segfault, I was seeing messages such as: 2013-02-22 16:12:37.504+0000: 19833: error : virNetSocketWriteWire:1361 : Cannot write data: Bad file descriptor which is also evidence of deferencing a stale pointer. I traced it to a race where qemuMonitorTestIO could execute late, after the main thread had already called qemuMonitorTestFree and called virNetSocketClose(test->client) but not clearing it out to NULL. Sure enough, after test->client has been closed, fd is -1, which causes an attempt to write to the socket to fail, which in turn triggers the error code of qemuMonitorTestIO that tries to re-close test->client. * tests/qemumonitortestutils.c (qemuMonitorTestIO): Don't attempt to free client again if test already quit. --- tests/qemumonitortestutils.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 1ed42ce..979623a 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -214,6 +214,10 @@ static void qemuMonitorTestIO(virNetSocketPtr sock, bool err = false; virMutexLock(&test->lock); + if (test->quit) { + virMutexUnlock(&test->lock); + return; + } if (events & VIR_EVENT_HANDLE_WRITABLE) { ssize_t ret; if ((ret = virNetSocketWrite(sock, -- 1.8.1.2
On 23.02.2013 00:09, Eric Blake wrote:
On a machine without json headers, I was seeing random segfaults from qemumonitorjsontest (about 90% of the runs on my particular machine). The segfault was inside virClassIsDerivedFrom, which points to a case of a race leading to unreferencing a stale pointer to an object that had already been freed. I also noticed that if I got the segfault, I was seeing messages such as:
2013-02-22 16:12:37.504+0000: 19833: error : virNetSocketWriteWire:1361 : Cannot write data: Bad file descriptor
which is also evidence of deferencing a stale pointer. I traced it to a race where qemuMonitorTestIO could execute late, after the main thread had already called qemuMonitorTestFree and called virNetSocketClose(test->client) but not clearing it out to NULL. Sure enough, after test->client has been closed, fd is -1, which causes an attempt to write to the socket to fail, which in turn triggers the error code of qemuMonitorTestIO that tries to re-close test->client.
* tests/qemumonitortestutils.c (qemuMonitorTestIO): Don't attempt to free client again if test already quit. --- tests/qemumonitortestutils.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 1ed42ce..979623a 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -214,6 +214,10 @@ static void qemuMonitorTestIO(virNetSocketPtr sock, bool err = false;
virMutexLock(&test->lock); + if (test->quit) { + virMutexUnlock(&test->lock); + return; + } if (events & VIR_EVENT_HANDLE_WRITABLE) { ssize_t ret; if ((ret = virNetSocketWrite(sock,
ACK Michal
Now that the segfault is solved, we can skip instead of fail the test when yajl is not present. * tests/qemumonitorjsontest.c (mymain): Skip if no yajl. --- tests/qemumonitorjsontest.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 55032d6..e2f8cb1 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2011-2012 Red Hat, Inc. + * Copyright (C) 2011-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -444,2 +443,2 @@ mymain(void) int ret = 0; virCapsPtr caps; +#if !WITH_YAJL + fprintf(stderr, "libvirt not compiled with yajl, skipping"); + return EXIT_AM_SKIP; +#endif + if (virThreadInitialize() < 0) exit(EXIT_FAILURE); -- 1.8.1.2
On 23.02.2013 00:09, Eric Blake wrote:
Now that the segfault is solved, we can skip instead of fail the test when yajl is not present.
* tests/qemumonitorjsontest.c (mymain): Skip if no yajl. --- tests/qemumonitorjsontest.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 55032d6..e2f8cb1 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2011-2012 Red Hat, Inc. + * Copyright (C) 2011-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -444,2 +443,2 @@ mymain(void) int ret = 0; virCapsPtr caps;
+#if !WITH_YAJL + fprintf(stderr, "libvirt not compiled with yajl, skipping"); + return EXIT_AM_SKIP; +#endif + if (virThreadInitialize() < 0) exit(EXIT_FAILURE);
ACK Michal
testutils.c likes to print summaries after a test completes, including if it failed. But if the test outright exit()s, this summary is skipped. Enforce that we return instead of exit. * cfg.mk (sc_prohibit_exit_tests): New syntax check. * tests/commandhelper.c (main): Fix offenders. * tests/qemumonitorjsontest.c (mymain): Likewise. * tests/seclabeltest.c (main): Likewise. * tests/securityselinuxlabeltest.c (mymain): Likewise. * tests/securityselinuxtest.c (mymain): Likewise. * tests/testutils.h (VIRT_TEST_MAIN_PRELOAD): Likewise. * tests/testutils.c (virtTestMain): Likewise. (virtTestCaptureProgramOutput): Use symbolic name. --- cfg.mk | 7 +++++++ tests/commandhelper.c | 7 +++---- tests/qemumonitorjsontest.c | 8 +++----- tests/seclabeltest.c | 8 ++++---- tests/securityselinuxlabeltest.c | 6 +++--- tests/securityselinuxtest.c | 6 +++--- tests/testutils.c | 8 ++++---- tests/testutils.h | 4 ++-- 8 files changed, 29 insertions(+), 25 deletions(-) diff --git a/cfg.mk b/cfg.mk index d56f7e3..b95a90b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -709,6 +709,13 @@ sc_prohibit_semicolon_at_eol_in_python: halt="Don't use semicolon at eol in python files" \ $(_sc_search_regexp) +# mymain() in test files should use return, not exit, for nicer output +sc_prohibit_exit_in_tests: + @prohibit='\<exit *\(' \ + in_vc_files='^tests/' \ + halt='use return, not exit(), in tests' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 39f3c53..92f031f 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -1,7 +1,7 @@ /* * commandhelper.c: Auxiliary program for commandtest * - * Copyright (C) 2010-2012 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -71,9 +71,8 @@ int main(int argc, char **argv) { origenv++; } - if (VIR_ALLOC_N(newenv, n) < 0) { - exit(EXIT_FAILURE); - } + if (VIR_ALLOC_N(newenv, n) < 0) + return EXIT_FAILURE; origenv = environ; n = i = 0; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index e2f8cb1..fa5ef7c 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -448,11 +448,9 @@ mymain(void) return EXIT_AM_SKIP; #endif - if (virThreadInitialize() < 0) - exit(EXIT_FAILURE); - - if (!(caps = testQemuCapsInit())) - exit(EXIT_FAILURE); + if (virThreadInitialize() < 0 || + !(caps = testQemuCapsInit())) + return EXIT_FAILURE; virEventRegisterDefaultImpl(); diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index 93b4902..cd34b6b 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -15,12 +15,12 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) const char *doi, *model; if (virThreadInitialize() < 0) - exit(EXIT_FAILURE); + return EXIT_FAILURE; mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false); if (mgr == NULL) { fprintf(stderr, "Failed to start security driver"); - exit(EXIT_FAILURE); + return EXIT_FAILURE; } model = virSecurityManagerGetModel(mgr); @@ -28,7 +28,7 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) { fprintf(stderr, "Failed to copy secModel model: %s", strerror(errno)); - exit(EXIT_FAILURE); + return EXIT_FAILURE; } doi = virSecurityManagerGetDOI(mgr); @@ -36,7 +36,7 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) { fprintf(stderr, "Failed to copy secModel DOI: %s", strerror(errno)); - exit(EXIT_FAILURE); + return EXIT_FAILURE; } virObjectUnref(mgr); diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 7445ab6..2454772 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -317,15 +317,15 @@ mymain(void) if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) { virErrorPtr err = virGetLastError(); if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) - exit(EXIT_AM_SKIP); + return EXIT_AM_SKIP; fprintf(stderr, "Unable to initialize security driver: %s\n", err->message); - exit(EXIT_FAILURE); + return EXIT_FAILURE; } if ((caps = testQemuCapsInit()) == NULL) - exit(EXIT_FAILURE); + return EXIT_FAILURE; #define DO_TEST_LABELING(name) \ if (virtTestRun("Labelling " # name, 1, testSELinuxLabeling, name) < 0) \ diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index 0dff872..ba00010 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2011-2012 Red Hat, Inc. + * Copyright (C) 2011-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -274,11 +274,11 @@ mymain(void) if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) { virErrorPtr err = virGetLastError(); if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) - exit(EXIT_AM_SKIP); + return EXIT_AM_SKIP; fprintf(stderr, "Unable to initialize security driver: %s\n", err->message); - exit(EXIT_FAILURE); + return EXIT_FAILURE; } #define DO_TEST_GEN_LABEL(desc, pidcon, \ diff --git a/tests/testutils.c b/tests/testutils.c index 7b2ea51..ea46c09 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -317,7 +317,7 @@ virtTestCaptureProgramOutput(const char *const argv[], char **buf, int maxlen) virtTestCaptureProgramExecChild(argv, pipefd[1]); VIR_FORCE_CLOSE(pipefd[1]); - _exit(1); + _exit(EXIT_FAILURE); case -1: return -1; @@ -586,7 +586,7 @@ int virtTestMain(int argc, abs_srcdir_cleanup = true; } if (!abs_srcdir) - exit(EXIT_AM_HARDFAIL); + return EXIT_AM_HARDFAIL; progname = last_component(argv[0]); if (STRPREFIX(progname, "lt-")) @@ -603,13 +603,13 @@ int virtTestMain(int argc, if (virThreadInitialize() < 0 || virErrorInitialize() < 0) - return 1; + return EXIT_FAILURE; virLogSetFromEnv(); if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) { if (virLogDefineOutput(virtTestLogOutput, virtTestLogClose, &testLog, VIR_LOG_DEBUG, VIR_LOG_TO_STDERR, NULL, 0) < 0) - return 1; + return EXIT_FAILURE; } #if TEST_OOM diff --git a/tests/testutils.h b/tests/testutils.h index 581bd3e..546c9ae 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -1,7 +1,7 @@ /* * utils.c: test utils * - * Copyright (C) 2005, 2008-2012 Red Hat, Inc. + * Copyright (C) 2005, 2008-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -78,7 +78,7 @@ int virtTestMain(int argc, if (virAsprintf(&newenv, "%s%s%s", preload ? preload : "", \ preload ? ":" : "", lib) < 0) { \ perror("virAsprintf"); \ - exit(EXIT_FAILURE); \ + return EXIT_FAILURE; \ } \ setenv("LD_PRELOAD", newenv, 1); \ execv(argv[0], argv); \ -- 1.8.1.2
On 23.02.2013 00:09, Eric Blake wrote:
testutils.c likes to print summaries after a test completes, including if it failed. But if the test outright exit()s, this summary is skipped. Enforce that we return instead of exit.
* cfg.mk (sc_prohibit_exit_tests): New syntax check. * tests/commandhelper.c (main): Fix offenders. * tests/qemumonitorjsontest.c (mymain): Likewise. * tests/seclabeltest.c (main): Likewise. * tests/securityselinuxlabeltest.c (mymain): Likewise. * tests/securityselinuxtest.c (mymain): Likewise. * tests/testutils.h (VIRT_TEST_MAIN_PRELOAD): Likewise. * tests/testutils.c (virtTestMain): Likewise. (virtTestCaptureProgramOutput): Use symbolic name. --- cfg.mk | 7 +++++++ tests/commandhelper.c | 7 +++---- tests/qemumonitorjsontest.c | 8 +++----- tests/seclabeltest.c | 8 ++++---- tests/securityselinuxlabeltest.c | 6 +++--- tests/securityselinuxtest.c | 6 +++--- tests/testutils.c | 8 ++++---- tests/testutils.h | 4 ++-- 8 files changed, 29 insertions(+), 25 deletions(-)
ACK Michal
On 02/25/2013 03:38 AM, Michal Privoznik wrote:
On 23.02.2013 00:09, Eric Blake wrote:
testutils.c likes to print summaries after a test completes, including if it failed. But if the test outright exit()s, this summary is skipped. Enforce that we return instead of exit.
ACK
Thanks for the reviews. Series pushed now, as this improves the testsuite for 1.0.3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik