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

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 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

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

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