[libvirt] [PATCH 0/4] Fix up the mess in commandtest
commandtest is quite messy with lots of memory leaks, some filedescriptor leaks and few other small issues. This patches fixes them. Jiri Denemark (4): tests: Fix code formating in commandtest tests: Fix leaks in commandtest tests: Fix commandtest in VPATH build tests: Fix detection of expected error tests/commandtest.c | 282 +++++++++++++++++++++++++++++++++------------------ 1 files changed, 184 insertions(+), 98 deletions(-) -- 1.7.3.2
--- tests/commandtest.c | 55 +++++++++++++++++++++++++++++++++----------------- 1 files changed, 36 insertions(+), 19 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index ace2f33..9ccbcef 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -50,7 +50,8 @@ static char *progname; static char *abs_srcdir; -static int checkoutput(const char *testname) { +static int checkoutput(const char *testname) +{ int ret = -1; char cwd[1024]; char *expectname = NULL; @@ -82,7 +83,6 @@ static int checkoutput(const char *testname) { goto cleanup; } - ret = 0; cleanup: @@ -99,7 +99,8 @@ cleanup: * Only stdin/out/err open * No slot for return status must log error. */ -static int test0(const void *unused ATTRIBUTE_UNUSED) { +static int test0(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd; char *log; int ret = -1; @@ -125,7 +126,8 @@ cleanup: * Only stdin/out/err open * Capturing return status must not log error. */ -static int test1(const void *unused ATTRIBUTE_UNUSED) { +static int test1(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd; int ret = -1; int status; @@ -146,7 +148,8 @@ cleanup: * Run program (twice), no args, inherit all ENV, keep CWD. * Only stdin/out/err open */ -static int test2(const void *unused ATTRIBUTE_UNUSED) { +static int test2(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); int ret; @@ -176,7 +179,8 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) { * Run program, no args, inherit all ENV, keep CWD. * stdin/out/err + two extra FD open */ -static int test3(const void *unused ATTRIBUTE_UNUSED) { +static int test3(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); int newfd1 = dup(STDERR_FILENO); int newfd2 = dup(STDERR_FILENO); @@ -211,7 +215,8 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) { * Only stdin/out/err open. * Daemonized */ -static int test4(const void *unused ATTRIBUTE_UNUSED) { +static int test4(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); pid_t pid; char *pidfile = virFilePid(abs_builddir, "commandhelper"); @@ -244,7 +249,8 @@ static int test4(const void *unused ATTRIBUTE_UNUSED) { * Run program, no args, inherit filtered ENV, keep CWD. * Only stdin/out/err open */ -static int test5(const void *unused ATTRIBUTE_UNUSED) { +static int test5(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); virCommandAddEnvPassCommon(cmd); @@ -265,7 +271,8 @@ static int test5(const void *unused ATTRIBUTE_UNUSED) { * Run program, no args, inherit filtered ENV, keep CWD. * Only stdin/out/err open */ -static int test6(const void *unused ATTRIBUTE_UNUSED) { +static int test6(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); virCommandAddEnvPass(cmd, "DISPLAY"); @@ -287,7 +294,8 @@ static int test6(const void *unused ATTRIBUTE_UNUSED) { * Run program, no args, inherit filtered ENV, keep CWD. * Only stdin/out/err open */ -static int test7(const void *unused ATTRIBUTE_UNUSED) { +static int test7(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); virCommandAddEnvPassCommon(cmd); @@ -309,7 +317,8 @@ static int test7(const void *unused ATTRIBUTE_UNUSED) { * Run program, no args, inherit filtered ENV, keep CWD. * Only stdin/out/err open */ -static int test8(const void *unused ATTRIBUTE_UNUSED) { +static int test8(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); virCommandAddEnvString(cmd, "LANG=C"); @@ -331,7 +340,8 @@ static int test8(const void *unused ATTRIBUTE_UNUSED) { * Run program, some args, inherit all ENV, keep CWD. * Only stdin/out/err open */ -static int test9(const void *unused ATTRIBUTE_UNUSED) { +static int test9(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); const char* const args[] = { "arg1", "arg2", NULL }; @@ -356,7 +366,8 @@ static int test9(const void *unused ATTRIBUTE_UNUSED) { * Run program, some args, inherit all ENV, keep CWD. * Only stdin/out/err open */ -static int test10(const void *unused ATTRIBUTE_UNUSED) { +static int test10(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); const char *const args[] = { "-version", "-log=bar.log", NULL, @@ -379,7 +390,8 @@ static int test10(const void *unused ATTRIBUTE_UNUSED) { * Run program, some args, inherit all ENV, keep CWD. * Only stdin/out/err open */ -static int test11(const void *unused ATTRIBUTE_UNUSED) { +static int test11(const void *unused ATTRIBUTE_UNUSED) +{ const char *args[] = { abs_builddir "/commandhelper", "-version", "-log=bar.log", NULL, @@ -401,7 +413,8 @@ static int test11(const void *unused ATTRIBUTE_UNUSED) { * Run program, no args, inherit all ENV, keep CWD. * Only stdin/out/err open. Set stdin data */ -static int test12(const void *unused ATTRIBUTE_UNUSED) { +static int test12(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); virCommandSetInputBuffer(cmd, "Hello World\n"); @@ -421,7 +434,8 @@ static int test12(const void *unused ATTRIBUTE_UNUSED) { * Run program, no args, inherit all ENV, keep CWD. * Only stdin/out/err open. Set stdin data */ -static int test13(const void *unused ATTRIBUTE_UNUSED) { +static int test13(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); char *outactual = NULL; const char *outexpect = "BEGIN STDOUT\n" @@ -459,7 +473,8 @@ cleanup: * Run program, no args, inherit all ENV, keep CWD. * Only stdin/out/err open. Set stdin data */ -static int test14(const void *unused ATTRIBUTE_UNUSED) { +static int test14(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); char *outactual = NULL; const char *outexpect = "BEGIN STDOUT\n" @@ -508,7 +523,8 @@ cleanup: * Run program, no args, inherit all ENV, change CWD. * Only stdin/out/err open */ -static int test15(const void *unused ATTRIBUTE_UNUSED) { +static int test15(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); virCommandSetWorkingDirectory(cmd, abs_builddir "/commanddata"); @@ -527,7 +543,8 @@ static int test15(const void *unused ATTRIBUTE_UNUSED) { /* * Don't run program; rather, log what would be run. */ -static int test16(const void *unused ATTRIBUTE_UNUSED) { +static int test16(const void *unused ATTRIBUTE_UNUSED) +{ virCommandPtr cmd = virCommandNew("/bin/true"); char *outactual = NULL; const char *outexpect = "A=B /bin/true C"; -- 1.7.3.2
On 12/06/2010 05:03 AM, Jiri Denemark wrote:
--- tests/commandtest.c | 55 +++++++++++++++++++++++++++++++++----------------- 1 files changed, 36 insertions(+), 19 deletions(-)
ACK: No semantic changes. (Can you tell where I used copy and paste? :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
tests/commandtest.c | 55 +++++++++++++++++++++++++++++++++----------------- 1 files changed, 36 insertions(+), 19 deletions(-)
ACK: No semantic changes.
Thanks, I pushed this and patch 4. Jirka
--- tests/commandtest.c | 213 +++++++++++++++++++++++++++++++++----------------- 1 files changed, 140 insertions(+), 73 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 9ccbcef..48c6335 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -107,7 +107,7 @@ static int test0(const void *unused ATTRIBUTE_UNUSED) free(virtTestLogContentAndReset()); cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); - if (virCommandRun(cmd, NULL) == 0) + if (!cmd || virCommandRun(cmd, NULL) == 0) goto cleanup; if ((log = virtTestLogContentAndReset()) == NULL) goto cleanup; @@ -133,7 +133,7 @@ static int test1(const void *unused ATTRIBUTE_UNUSED) int status; cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); - if (virCommandRun(cmd, &status) < 0) + if (!cmd || virCommandRun(cmd, &status) < 0) goto cleanup; if (status == 0) goto cleanup; @@ -151,28 +151,32 @@ cleanup: static int test2(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); - int ret; + virErrorPtr err = NULL; + int ret = -1; - if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + if (!cmd) return -1; - } - if ((ret = checkoutput("test2")) != 0) { - virCommandFree(cmd); - return ret; + if (virCommandRun(cmd, NULL) < 0) { + err = virGetLastError(); + goto cleanup; } + if (checkoutput("test2") != 0) + goto cleanup; + if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); - return -1; + err = virGetLastError(); + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test2"); - return checkoutput("test2"); +cleanup: + if (err) + printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); + return ret; } /* @@ -185,6 +189,10 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) int newfd1 = dup(STDERR_FILENO); int newfd2 = dup(STDERR_FILENO); int newfd3 = dup(STDERR_FILENO); + int ret = -1; + + if (!cmd) + goto cleanup; virCommandPreserveFD(cmd, newfd1); virCommandTransferFD(cmd, newfd3); @@ -192,21 +200,23 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } if (fcntl(newfd1, F_GETFL) < 0 || fcntl(newfd2, F_GETFL) < 0 || fcntl(newfd3, F_GETFL) >= 0) { puts("fds in wrong state"); - return -1; + goto cleanup; } + ret = checkoutput("test3"); + +cleanup: virCommandFree(cmd); VIR_FORCE_CLOSE(newfd1); VIR_FORCE_CLOSE(newfd2); - - return checkoutput("test3"); + return ret; } @@ -218,8 +228,12 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) static int test4(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); - pid_t pid; char *pidfile = virFilePid(abs_builddir, "commandhelper"); + pid_t pid; + int ret = -1; + + if (!cmd || !pidfile) + goto cleanup; virCommandSetPidFile(cmd, pidfile); virCommandDaemonize(cmd); @@ -227,21 +241,22 @@ static int test4(const void *unused ATTRIBUTE_UNUSED) if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } if (virFileReadPid(abs_builddir, "commandhelper", &pid) != 0) { printf("cannot read pidfile\n"); - return -1; + goto cleanup; } while (kill(pid, 0) != -1) usleep(100*1000); - virCommandFree(cmd); + ret = checkoutput("test4"); +cleanup: + virCommandFree(cmd); VIR_FREE(pidfile); - - return checkoutput("test4"); + return ret; } @@ -252,18 +267,24 @@ static int test4(const void *unused ATTRIBUTE_UNUSED) static int test5(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + int ret = -1; + + if (!cmd) + return -1; virCommandAddEnvPassCommon(cmd); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test5"); - return checkoutput("test5"); +cleanup: + virCommandFree(cmd); + return ret; } @@ -274,6 +295,10 @@ static int test5(const void *unused ATTRIBUTE_UNUSED) static int test6(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + int ret = -1; + + if (!cmd) + return -1; virCommandAddEnvPass(cmd, "DISPLAY"); virCommandAddEnvPass(cmd, "DOESNOTEXIST"); @@ -281,12 +306,14 @@ static int test6(const void *unused ATTRIBUTE_UNUSED) if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test6"); - return checkoutput("test6"); +cleanup: + virCommandFree(cmd); + return ret; } @@ -297,6 +324,10 @@ static int test6(const void *unused ATTRIBUTE_UNUSED) static int test7(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + int ret = -1; + + if (!cmd) + return -1; virCommandAddEnvPassCommon(cmd); virCommandAddEnvPass(cmd, "DISPLAY"); @@ -305,12 +336,14 @@ static int test7(const void *unused ATTRIBUTE_UNUSED) if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test7"); - return checkoutput("test7"); +cleanup: + virCommandFree(cmd); + return ret; } /* @@ -320,6 +353,10 @@ static int test7(const void *unused ATTRIBUTE_UNUSED) static int test8(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + int ret = -1; + + if (!cmd) + return -1; virCommandAddEnvString(cmd, "LANG=C"); virCommandAddEnvPair(cmd, "USER", "test"); @@ -327,12 +364,14 @@ static int test8(const void *unused ATTRIBUTE_UNUSED) if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test8"); - return checkoutput("test8"); +cleanup: + virCommandFree(cmd); + return ret; } @@ -344,6 +383,10 @@ static int test9(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); const char* const args[] = { "arg1", "arg2", NULL }; + int ret = -1; + + if (!cmd) + return -1; virCommandAddArg(cmd, "-version"); virCommandAddArgPair(cmd, "-log", "bar.log"); @@ -353,12 +396,14 @@ static int test9(const void *unused ATTRIBUTE_UNUSED) if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test9"); - return checkoutput("test9"); +cleanup: + virCommandFree(cmd); + return ret; } @@ -372,18 +417,24 @@ static int test10(const void *unused ATTRIBUTE_UNUSED) const char *const args[] = { "-version", "-log=bar.log", NULL, }; + int ret = -1; + + if (!cmd) + return -1; virCommandAddArgSet(cmd, args); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test10"); - return checkoutput("test10"); +cleanup: + virCommandFree(cmd); + return ret; } /* @@ -397,16 +448,22 @@ static int test11(const void *unused ATTRIBUTE_UNUSED) "-version", "-log=bar.log", NULL, }; virCommandPtr cmd = virCommandNewArgs(args); + int ret = -1; + + if (!cmd) + return -1; if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test11"); - return checkoutput("test11"); +cleanup: + virCommandFree(cmd); + return ret; } /* @@ -416,18 +473,24 @@ static int test11(const void *unused ATTRIBUTE_UNUSED) static int test12(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + int ret = -1; + + if (!cmd) + return -1; virCommandSetInputBuffer(cmd, "Hello World\n"); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test12"); - return checkoutput("test12"); +cleanup: + virCommandFree(cmd); + return ret; } /* @@ -443,28 +506,27 @@ static int test13(const void *unused ATTRIBUTE_UNUSED) "END STDOUT\n"; int ret = -1; + if (!cmd) + return -1; + virCommandSetInputBuffer(cmd, "Hello World\n"); virCommandSetOutputBuffer(cmd, &outactual); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); - if (!STREQ(outactual, outexpect)) { virtTestDifference(stderr, outactual, outexpect); goto cleanup; } - if (checkoutput("test13") < 0) - goto cleanup; - - ret = 0; + ret = checkoutput("test13"); cleanup: + virCommandFree(cmd); VIR_FREE(outactual); return ret; } @@ -486,6 +548,9 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) "END STDERR\n"; int ret = -1; + if (!cmd) + return -1; + virCommandSetInputBuffer(cmd, "Hello World\n"); virCommandSetOutputBuffer(cmd, &outactual); virCommandSetErrorBuffer(cmd, &erractual); @@ -493,11 +558,9 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); - if (!STREQ(outactual, outexpect)) { virtTestDifference(stderr, outactual, outexpect); goto cleanup; @@ -507,12 +570,10 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - if (checkoutput("test14") < 0) - goto cleanup; - - ret = 0; + ret = checkoutput("test14"); cleanup: + virCommandFree(cmd); VIR_FREE(outactual); VIR_FREE(erractual); return ret; @@ -526,18 +587,24 @@ cleanup: static int test15(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + int ret = -1; + + if (!cmd) + return -1; virCommandSetWorkingDirectory(cmd, abs_builddir "/commanddata"); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } - virCommandFree(cmd); + ret = checkoutput("test15"); - return checkoutput("test15"); +cleanup: + virCommandFree(cmd); + return ret; } /* @@ -551,13 +618,16 @@ static int test16(const void *unused ATTRIBUTE_UNUSED) int ret = -1; int fd = -1; + if (!cmd) + return -1; + virCommandAddEnvPair(cmd, "A", "B"); virCommandAddArg(cmd, "C"); if ((outactual = virCommandToString(cmd)) == NULL) { virErrorPtr err = virGetLastError(); printf("Cannot convert to string: %s\n", err->message); - return -1; + goto cleanup; } if ((fd = open(abs_builddir "/commandhelper.log", O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) { @@ -570,18 +640,15 @@ static int test16(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - virCommandFree(cmd); - - if (checkoutput("test16") < 0) - goto cleanup; - if (!STREQ(outactual, outexpect)) { virtTestDifference(stderr, outactual, outexpect); goto cleanup; } - ret = 0; + + ret = checkoutput("test16"); cleanup: + virCommandFree(cmd); VIR_FORCE_CLOSE(fd); VIR_FREE(outactual); return ret; -- 1.7.3.2
On Mon, Dec 06, 2010 at 01:03:25PM +0100, Jiri Denemark wrote:
--- tests/commandtest.c | 213 +++++++++++++++++++++++++++++++++----------------- 1 files changed, 140 insertions(+), 73 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c index 9ccbcef..48c6335 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -107,7 +107,7 @@ static int test0(const void *unused ATTRIBUTE_UNUSED)
free(virtTestLogContentAndReset()); cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); - if (virCommandRun(cmd, NULL) == 0) + if (!cmd || virCommandRun(cmd, NULL) == 0)
The API explicitly does *not* require you to check !cmd after virCommandNew. virCommandRun() and other APis will check that for you and return ENOMEM. Daniel
free(virtTestLogContentAndReset()); cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); - if (virCommandRun(cmd, NULL) == 0) + if (!cmd || virCommandRun(cmd, NULL) == 0)
The API explicitly does *not* require you to check !cmd after virCommandNew. virCommandRun() and other APis will check that for you and return ENOMEM.
That was my suspicion too but then I looked at virCommandRun implementation and saw this: if (!cmd || cmd->has_error == -1) { virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; } if (cmd->has_error == ENOMEM) { virReportOOMError(); return -1; } That is, if virCommandNew() returns NULL for either reason, by running virCommandRun() on it, the original error (probably OOM) gets overwritten by an internal error. Jirka
On 12/06/2010 05:44 AM, Jiri Denemark wrote:
free(virtTestLogContentAndReset()); cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); - if (virCommandRun(cmd, NULL) == 0) + if (!cmd || virCommandRun(cmd, NULL) == 0)
The API explicitly does *not* require you to check !cmd after virCommandNew. virCommandRun() and other APis will check that for you and return ENOMEM.
That was my suspicion too but then I looked at virCommandRun implementation and saw this:
if (!cmd || cmd->has_error == -1) { virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; } if (cmd->has_error == ENOMEM) { virReportOOMError(); return -1; }
That is, if virCommandNew() returns NULL for either reason, by running virCommandRun() on it, the original error (probably OOM) gets overwritten by an internal error.
Oh, I see. The correct fix is to swap that precedence in command.c (I'm working on a patch): if (!cmd || cmd->has_error == ENOMEM) { virReportOOMError(); return -1; } if (cmd->has_error) { report invalid use of command as internal error } Then you can safely ignore allocation failure on virCommandNew up until virCommandRun complains about the OOM failure, and it won't be treated as an internal usage error. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/util/command.c (virCommandAddEnvString): Remove duplicate code. (virCommandToString, virCommandRun, virCommandRunAsync) (virCommandWait): Report NULL command as ENOMEM, not invalid usage. Reported by Jiri Denemark. --- New patch. Fixes the root cause, so that patch 2 can be smaller... src/util/command.c | 43 ++++++++++++++++++++----------------------- 1 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 38d462b..c0520ec 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -292,9 +292,6 @@ virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) void virCommandAddEnvString(virCommandPtr cmd, const char *str) { - if (!cmd || cmd->has_error) - return; - char *env; if (!cmd || cmd->has_error) @@ -710,13 +707,13 @@ virCommandToString(virCommandPtr cmd) /* Cannot assume virCommandRun will be called; so report the error * now. If virCommandRun is called, it will report the same error. */ - if (!cmd || cmd->has_error == -1) { - virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (!cmd ||cmd->has_error == ENOMEM) { + virReportOOMError(); return NULL; } - if (cmd->has_error == ENOMEM) { - virReportOOMError(); + if (cmd->has_error) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); return NULL; } @@ -876,13 +873,13 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) char *errbuf = NULL; int infd[2]; - if (!cmd || cmd->has_error == -1) { - virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (!cmd ||cmd->has_error == ENOMEM) { + virReportOOMError(); return -1; } - if (cmd->has_error == ENOMEM) { - virReportOOMError(); + if (cmd->has_error) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); return -1; } @@ -994,13 +991,13 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) char *str; int i; - if (!cmd || cmd->has_error == -1) { - virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (!cmd || cmd->has_error == ENOMEM) { + virReportOOMError(); return -1; } - if (cmd->has_error == ENOMEM) { - virReportOOMError(); + if (cmd->has_error) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); return -1; } @@ -1065,13 +1062,13 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) int ret; int status; - if (!cmd || cmd->has_error == -1) { - virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (!cmd ||cmd->has_error == ENOMEM) { + virReportOOMError(); return -1; } - if (cmd->has_error == ENOMEM) { - virReportOOMError(); + if (cmd->has_error) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); return -1; } -- 1.7.3.2
* src/util/command.c (virCommandAddEnvString): Remove duplicate code. (virCommandToString, virCommandRun, virCommandRunAsync) (virCommandWait): Report NULL command as ENOMEM, not invalid usage. Reported by Jiri Denemark. ---
New patch. Fixes the root cause, so that patch 2 can be smaller...
Yeah, makes sense. I didn't realize the expected behavior was not to require error checking even after virCommandNew*
diff --git a/src/util/command.c b/src/util/command.c index 38d462b..c0520ec 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -292,9 +292,6 @@ virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) void virCommandAddEnvString(virCommandPtr cmd, const char *str) { - if (!cmd || cmd->has_error) - return; - char *env;
if (!cmd || cmd->has_error)
Eh.
@@ -710,13 +707,13 @@ virCommandToString(virCommandPtr cmd)
/* Cannot assume virCommandRun will be called; so report the error * now. If virCommandRun is called, it will report the same error. */ - if (!cmd || cmd->has_error == -1) { - virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (!cmd ||cmd->has_error == ENOMEM) { + virReportOOMError(); return NULL; } - if (cmd->has_error == ENOMEM) { - virReportOOMError(); + if (cmd->has_error) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); return NULL; }
I checked has_error is either 0, -1, or ENOMEM so we are not hiding any error here. ACK Jirka
Most leaks could only occur on error cleanup paths. --- Respin of Jiri's 2/4 v1 patch, now much smaller by relying on guaranteed semantics. Jiri's patch 1 and 4 of the v1 series are still okay to apply as-is, but patch 3/4 needs rebasing onto this patch. tests/commandtest.c | 72 +++++++++++++++++++++++++++++++------------------- 1 files changed, 45 insertions(+), 27 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index ace2f33..2666d61 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -153,6 +153,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -164,6 +165,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -181,6 +183,7 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) { int newfd1 = dup(STDERR_FILENO); int newfd2 = dup(STDERR_FILENO); int newfd3 = dup(STDERR_FILENO); + int ret = -1; virCommandPreserveFD(cmd, newfd1); virCommandTransferFD(cmd, newfd3); @@ -188,21 +191,23 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } if (fcntl(newfd1, F_GETFL) < 0 || fcntl(newfd2, F_GETFL) < 0 || fcntl(newfd3, F_GETFL) >= 0) { puts("fds in wrong state"); - return -1; + goto cleanup; } + ret = checkoutput("test3"); + +cleanup: virCommandFree(cmd); VIR_FORCE_CLOSE(newfd1); VIR_FORCE_CLOSE(newfd2); - - return checkoutput("test3"); + return ret; } @@ -213,8 +218,12 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) { */ static int test4(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); - pid_t pid; char *pidfile = virFilePid(abs_builddir, "commandhelper"); + pid_t pid; + int ret = -1; + + if (!pidfile) + goto cleanup; virCommandSetPidFile(cmd, pidfile); virCommandDaemonize(cmd); @@ -222,21 +231,22 @@ static int test4(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } if (virFileReadPid(abs_builddir, "commandhelper", &pid) != 0) { printf("cannot read pidfile\n"); - return -1; + goto cleanup; } while (kill(pid, 0) != -1) usleep(100*1000); - virCommandFree(cmd); + ret = checkoutput("test4"); +cleanup: + virCommandFree(cmd); VIR_FREE(pidfile); - - return checkoutput("test4"); + return ret; } @@ -252,6 +262,7 @@ static int test5(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -274,6 +285,7 @@ static int test6(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -297,6 +309,7 @@ static int test7(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -318,6 +331,7 @@ static int test8(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -343,6 +357,7 @@ static int test9(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -367,6 +382,7 @@ static int test10(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -389,6 +405,7 @@ static int test11(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -409,6 +426,7 @@ static int test12(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -435,22 +453,23 @@ static int test13(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } + if (!outactual) + goto cleanup; virCommandFree(cmd); + cmd = NULL; if (!STREQ(outactual, outexpect)) { virtTestDifference(stderr, outactual, outexpect); goto cleanup; } - if (checkoutput("test13") < 0) - goto cleanup; - - ret = 0; + ret = checkoutput("test13"); cleanup: + virCommandFree(cmd); VIR_FREE(outactual); return ret; } @@ -478,10 +497,13 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - return -1; + goto cleanup; } + if (!outactual || !erractual) + goto cleanup; virCommandFree(cmd); + cmd = NULL; if (!STREQ(outactual, outexpect)) { virtTestDifference(stderr, outactual, outexpect); @@ -492,12 +514,10 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) { goto cleanup; } - if (checkoutput("test14") < 0) - goto cleanup; - - ret = 0; + ret = checkoutput("test14"); cleanup: + virCommandFree(cmd); VIR_FREE(outactual); VIR_FREE(erractual); return ret; @@ -516,6 +536,7 @@ static int test15(const void *unused ATTRIBUTE_UNUSED) { if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); + virCommandFree(cmd); return -1; } @@ -540,7 +561,7 @@ static int test16(const void *unused ATTRIBUTE_UNUSED) { if ((outactual = virCommandToString(cmd)) == NULL) { virErrorPtr err = virGetLastError(); printf("Cannot convert to string: %s\n", err->message); - return -1; + goto cleanup; } if ((fd = open(abs_builddir "/commandhelper.log", O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) { @@ -553,18 +574,15 @@ static int test16(const void *unused ATTRIBUTE_UNUSED) { goto cleanup; } - virCommandFree(cmd); - - if (checkoutput("test16") < 0) - goto cleanup; - if (!STREQ(outactual, outexpect)) { virtTestDifference(stderr, outactual, outexpect); goto cleanup; } - ret = 0; + + ret = checkoutput("test16"); cleanup: + virCommandFree(cmd); VIR_FORCE_CLOSE(fd); VIR_FREE(outactual); return ret; -- 1.7.3.2
Most leaks could only occur on error cleanup paths. ---
Respin of Jiri's 2/4 v1 patch, now much smaller by relying on guaranteed semantics.
Jiri's patch 1 and 4 of the v1 series are still okay to apply as-is, but patch 3/4 needs rebasing onto this patch.
tests/commandtest.c | 72 +++++++++++++++++++++++++++++++------------------- 1 files changed, 45 insertions(+), 27 deletions(-)
ACK Jirka
--- tests/commandtest.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 48c6335..e8decd3 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -587,12 +587,15 @@ cleanup: static int test15(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + char *cwd = NULL; int ret = -1; if (!cmd) return -1; - virCommandSetWorkingDirectory(cmd, abs_builddir "/commanddata"); + if (virAsprintf(&cwd, "%s/commanddata", abs_srcdir) < 0) + goto cleanup; + virCommandSetWorkingDirectory(cmd, cwd); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); @@ -603,6 +606,7 @@ static int test15(const void *unused ATTRIBUTE_UNUSED) ret = checkoutput("test15"); cleanup: + VIR_FREE(cwd); virCommandFree(cmd); return ret; } -- 1.7.3.2
From: Jiri Denemark <jdenemar@redhat.com> --- Resovling the rebase conflict wasn't as hard as I had first feared. tests/commandtest.c | 15 +++++++++++---- 1 files changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 2666d61..b7261e9 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -530,19 +530,26 @@ cleanup: */ static int test15(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + char *cwd = NULL; + int ret = -1; - virCommandSetWorkingDirectory(cmd, abs_builddir "/commanddata"); + if (virAsprintf(&cwd, "%s/commanddata", abs_srcdir) < 0) + goto cleanup; + virCommandSetWorkingDirectory(cmd, cwd); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); - virCommandFree(cmd); - return -1; + goto cleanup; } + ret = checkoutput("test15"); + +cleanup: + VIR_FREE(cwd); virCommandFree(cmd); - return checkoutput("test15"); + return ret; } /* -- 1.7.3.2
On 12/06/2010 05:03 AM, Jiri Denemark wrote:
--- tests/commandtest.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c index 48c6335..e8decd3 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -587,12 +587,15 @@ cleanup: static int test15(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + char *cwd = NULL; int ret = -1;
if (!cmd) return -1;
- virCommandSetWorkingDirectory(cmd, abs_builddir "/commanddata"); + if (virAsprintf(&cwd, "%s/commanddata", abs_srcdir) < 0) + goto cleanup; + virCommandSetWorkingDirectory(cmd, cwd);
ACK; I've gone ahead and pushed my rewrite of patch 2 in your series, as well as the rebase of this patch 3 from your series. I still haven't tried running commandtest under valgrind to see if there are any other leaks to plug (so far, it's just been patches by inspection and prove that the testsuite still passes), but I hope to get to that point as well. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
--- tests/commandtest.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index e8decd3..fe7c08b 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -102,17 +102,15 @@ cleanup: static int test0(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd; - char *log; int ret = -1; - free(virtTestLogContentAndReset()); cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); if (!cmd || virCommandRun(cmd, NULL) == 0) goto cleanup; - if ((log = virtTestLogContentAndReset()) == NULL) - goto cleanup; - if (strstr(log, ": error :") == NULL) + + if (virGetLastError() == NULL) goto cleanup; + virResetLastError(); ret = 0; -- 1.7.3.2
On 12/06/2010 05:03 AM, Jiri Denemark wrote:
--- tests/commandtest.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c index e8decd3..fe7c08b 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -102,17 +102,15 @@ cleanup: static int test0(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd; - char *log; int ret = -1;
- free(virtTestLogContentAndReset()); cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); if (!cmd || virCommandRun(cmd, NULL) == 0) goto cleanup; - if ((log = virtTestLogContentAndReset()) == NULL) - goto cleanup; - if (strstr(log, ": error :") == NULL) + + if (virGetLastError() == NULL) goto cleanup;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark