[PATCH 0/9] Couple of virCommand related improvements
Found these on a abandoned branch, but they are still worth merging. Polished them a bit though. Michal Prívozník (9): vircommand: Document virCommandSetSendBuffer() behaviour wrt daemonize virCommandDoAsyncIO: Drop misleading statement about main event loop commandtest: Use unsigned char in test27() virCommandSetSendBuffer: Take double pointer of @buffer commandtest: Test virCommandSetSendBuffer() with virCommandDoAsyncIO() commandtest: Use virTestCompareToFile() in checkoutput() virbuftest: Cleanup code around virTestDifference() tests: Don't wrap virTestDifference() arguments in NULLSTR() tests: Use virTestCompareToString() more src/qemu/qemu_tpm.c | 2 +- src/util/vircommand.c | 11 ++- src/util/vircommand.h | 2 +- tests/commanddata/test29.log | 20 ++++ tests/commandtest.c | 152 +++++++++++++++++++++++-------- tests/esxutilstest.c | 10 +- tests/nwfilterebiptablestest.c | 21 ++--- tests/openvzutilstest.c | 6 +- tests/sockettest.c | 3 +- tests/utiltest.c | 3 +- tests/vboxsnapshotxmltest.c | 3 +- tests/virbuftest.c | 25 ++--- tests/virfirewalltest.c | 30 ++---- tests/virjsontest.c | 6 +- tests/virkmodtest.c | 3 +- tests/virnetdevbandwidthtest.c | 5 +- tests/virnetdevopenvswitchtest.c | 10 +- tests/virnetsockettest.c | 3 +- tests/virshtest.c | 3 +- 19 files changed, 182 insertions(+), 136 deletions(-) create mode 100644 tests/commanddata/test29.log -- 2.37.4
When virCommandSetSendBuffer() is used over a virCommand that is (or will be) daemonized, then the command must have VIR_EXEC_ASYNC_IO flag set no later than at virCommandRunAsync() phase so that the thread that's doing IO is spawned and thus buffers can be sent to the process. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index bbfbe19706..cdc74bc2fd 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1693,6 +1693,9 @@ virCommandFreeSendBuffers(virCommand *cmd) * @buffer is always stolen regardless of the return value. This function * doesn't raise a libvirt error, but rather propagates the error via virCommand. * Thus callers don't need to take a special action if -1 is returned. + * + * When the @cmd is daemonized via virCommandDaemonize() remember to request + * asynchronous IO via virCommandDoAsyncIO(). */ int virCommandSetSendBuffer(virCommand *cmd, -- 2.37.4
Back in v1.0.3-rc1~235 when I was adding virCommandDoAsyncIO(), the main event loop was used to poll() on the pipe to the child process. But this was promptly changed to a separate thread handling I/O in v1.0.3-rc1~127. However, the corresponding comment to virCommandDoAsyncIO() still documents the original state. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index cdc74bc2fd..4f60432c0a 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -3078,8 +3078,6 @@ virCommandFree(virCommand *cmd) * * ... * - * - * The libvirt's event loop is used for handling stdios of @cmd. * Since current implementation uses strlen to determine length * of data to be written to @cmd's stdin, don't pass any binary * data. If you want to re-run command, you need to call this and -- 2.37.4
In test27() the virCommandSetSendBuffer() is used, which expects unsigned char. Use that type for variables which are passed to the function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/commandtest.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 95d1bfa91b..9bbf6fb260 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1041,8 +1041,8 @@ static int test27(const void *unused G_GNUC_UNUSED) int buf2fd; size_t buflen = 1024 * 128; g_autofree char *buffer0 = NULL; - g_autofree char *buffer1 = NULL; - g_autofree char *buffer2 = NULL; + g_autofree unsigned char *buffer1 = NULL; + g_autofree unsigned char *buffer2 = NULL; g_autofree char *outactual = NULL; g_autofree char *erractual = NULL; g_autofree char *outexpect = NULL; @@ -1055,8 +1055,8 @@ static int test27(const void *unused G_GNUC_UNUSED) "END STDERR\n" buffer0 = g_new0(char, buflen); - buffer1 = g_new0(char, buflen); - buffer2 = g_new0(char, buflen); + buffer1 = g_new0(unsigned char, buflen); + buffer2 = g_new0(unsigned char, buflen); memset(buffer0, 'H', buflen - 2); buffer0[buflen - 2] = '\n'; @@ -1075,8 +1075,8 @@ static int test27(const void *unused G_GNUC_UNUSED) errexpect = g_strdup_printf(TEST27_ERREXPECT_TEMP, buffer0, buffer1, buffer2); - buf1fd = virCommandSetSendBuffer(cmd, (unsigned char *) g_steal_pointer(&buffer1), buflen - 1); - buf2fd = virCommandSetSendBuffer(cmd, (unsigned char *) g_steal_pointer(&buffer2), buflen - 1); + buf1fd = virCommandSetSendBuffer(cmd, g_steal_pointer(&buffer1), buflen - 1); + buf2fd = virCommandSetSendBuffer(cmd, g_steal_pointer(&buffer2), buflen - 1); virCommandAddArg(cmd, "--readfd"); virCommandAddArgFormat(cmd, "%d", buf1fd); -- 2.37.4
The virCommandSetSendBuffer() function consumes passed @buffer, but takes it only as plain pointer. Switch to a double pointer to make this obvious. This allows us then to drop all g_steal_pointer() in callers. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 2 +- src/util/vircommand.c | 6 +++--- src/util/vircommand.h | 2 +- tests/commandtest.c | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index bdce060db8..d2f5bfb055 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -265,7 +265,7 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid, &secret, &secret_len) < 0) return -1; - *fd = virCommandSetSendBuffer(cmd, g_steal_pointer(&secret), secret_len); + *fd = virCommandSetSendBuffer(cmd, &secret, secret_len); return 0; } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 4f60432c0a..0917bc9cfb 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1699,10 +1699,10 @@ virCommandFreeSendBuffers(virCommand *cmd) */ int virCommandSetSendBuffer(virCommand *cmd, - unsigned char *buffer, + unsigned char **buffer, size_t buflen) { - g_autofree unsigned char *localbuf = g_steal_pointer(&buffer); + g_autofree unsigned char *localbuf = g_steal_pointer(buffer); int pipefd[2] = { -1, -1 }; size_t i; @@ -2901,7 +2901,7 @@ int virCommandHandshakeNotify(virCommand *cmd) #else /* WIN32 */ int virCommandSetSendBuffer(virCommand *cmd, - unsigned char *buffer G_GNUC_UNUSED, + unsigned char **buffer G_GNUC_UNUSED, size_t buflen G_GNUC_UNUSED) { if (virCommandHasError(cmd)) diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 98788bcbf7..e0002103b6 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -132,7 +132,7 @@ void virCommandSetWorkingDirectory(virCommand *cmd, const char *pwd) ATTRIBUTE_NONNULL(2); int virCommandSetSendBuffer(virCommand *cmd, - unsigned char *buffer, + unsigned char **buffer, size_t buflen) ATTRIBUTE_NONNULL(2); diff --git a/tests/commandtest.c b/tests/commandtest.c index 9bbf6fb260..8c5a9245a1 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1075,8 +1075,8 @@ static int test27(const void *unused G_GNUC_UNUSED) errexpect = g_strdup_printf(TEST27_ERREXPECT_TEMP, buffer0, buffer1, buffer2); - buf1fd = virCommandSetSendBuffer(cmd, g_steal_pointer(&buffer1), buflen - 1); - buf2fd = virCommandSetSendBuffer(cmd, g_steal_pointer(&buffer2), buflen - 1); + buf1fd = virCommandSetSendBuffer(cmd, &buffer1, buflen - 1); + buf2fd = virCommandSetSendBuffer(cmd, &buffer2, buflen - 1); virCommandAddArg(cmd, "--readfd"); virCommandAddArgFormat(cmd, "%d", buf1fd); -- 2.37.4
Introduce a test case which ensures that a daemonized process can work with virCommandSetSendBuffer() when async IO is enabled. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/commanddata/test29.log | 20 ++++++++ tests/commandtest.c | 97 ++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) create mode 100644 tests/commanddata/test29.log diff --git a/tests/commanddata/test29.log b/tests/commanddata/test29.log new file mode 100644 index 0000000000..962f8526f1 --- /dev/null +++ b/tests/commanddata/test29.log @@ -0,0 +1,20 @@ +ARG:--close-stdin +ARG:--check-daemonize +ARG:--readfd +ARG:3 +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=test +ENV:PATH=/usr/bin:/bin +ENV:TMPDIR=/tmp +ENV:USER=test +FD:0 +FD:1 +FD:2 +FD:3 +FD:6 +DAEMON:yes +CWD:/ +UMASK:0022 diff --git a/tests/commandtest.c b/tests/commandtest.c index 8c5a9245a1..6d45ff196f 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -25,6 +25,7 @@ #include <sys/stat.h> #ifndef WIN32 # include <sys/wait.h> +# include <poll.h> #endif #include <fcntl.h> @@ -1155,6 +1156,101 @@ test28(const void *unused G_GNUC_UNUSED) } +static int +test29(const void *unused G_GNUC_UNUSED) +{ + g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper"); + g_autofree char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper"); + pid_t pid; + int buffd; + VIR_AUTOCLOSE outfd = -1; + size_t buflen = 1024 * 10; + g_autofree unsigned char *buffer = NULL; + g_autofree char *outactual = NULL; + g_autofree char *outexpect = NULL; + size_t i; + size_t outactuallen = 0; + int ret = -1; + + if (!pidfile) + return -1; + + buffer = g_new0(unsigned char, buflen + 1); + for (i = 0; i < buflen; i++) { + buffer[i] = 'a' + i % ('z' - 'a' + 1); + } + buffer[buflen] = '\0'; + + outexpect = g_strdup_printf("BEGIN STDOUT\n%sEND STDOUT\n", buffer); + + buffd = virCommandSetSendBuffer(cmd, &buffer, buflen); + + virCommandAddArg(cmd, "--close-stdin"); + virCommandAddArg(cmd, "--check-daemonize"); + virCommandAddArg(cmd, "--readfd"); + virCommandAddArgFormat(cmd, "%d", buffd); + + virCommandSetOutputFD(cmd, &outfd); + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + virCommandDoAsyncIO(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + fprintf(stderr, "Cannot run child %s\n", virGetLastErrorMessage()); + goto cleanup; + } + + if (virPidFileReadPath(pidfile, &pid) < 0) { + fprintf(stderr, "cannot read pidfile: %s\n", pidfile); + goto cleanup; + } + + while (1) { + char buf[1024] = { 0 }; + struct pollfd pfd = {.fd = outfd, .events = POLLIN, .revents = 0}; + int rc = 0; + + rc = poll(&pfd, 1, 1000); + if (rc < 0) { + if (errno == EINTR) + continue; + + fprintf(stderr, "poll() returned errno = %d\n", errno); + goto cleanup; + } + + if (pfd.revents & POLLIN) { + rc = read(outfd, buf, sizeof(buf)); + if (rc < 0) { + fprintf(stderr, "cannot read from output pipe: errno=%d\n", errno); + goto cleanup; + } + + outactual = g_renew(char, outactual, outactuallen + rc + 1); + memcpy(outactual + outactuallen, buf, rc); + outactuallen += rc; + outactual[outactuallen] = '\0'; + } else if (pfd.revents & POLLERR || + pfd.revents & POLLHUP) { + break; + } + } + + if (STRNEQ_NULLABLE(outactual, outexpect)) { + virTestDifference(stderr, outexpect, outactual); + goto cleanup; + } + + ret = checkoutput("test29"); + + cleanup: + if (pidfile) + unlink(pidfile); + + return ret; +} + + static int mymain(void) { @@ -1252,6 +1348,7 @@ mymain(void) DO_TEST(test26); DO_TEST(test27); DO_TEST(test28); + DO_TEST(test29); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.37.4
In the commandtest there is checkoutput() function which checks the latest log of commandhelper (containing things like cmd line arguments, env vars, FDs, CWD, etc.) and compares that against expected output. Well, the way this function implements that is effectively by open coding virTestCompareToFile() except for the nice feature that the virTestCompareToFile() has: VIR_TEST_OUTPUT_REGENERATE. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/commandtest.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 6d45ff196f..ffc4b24ef4 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -58,29 +58,18 @@ static int checkoutput(const char *testname) { int ret = -1; g_autofree char *expectname = NULL; - g_autofree char *expectlog = NULL; g_autofree char *actualname = NULL; g_autofree char *actuallog = NULL; expectname = g_strdup_printf("%s/commanddata/%s.log", abs_srcdir, testname); actualname = g_strdup_printf("%s/commandhelper.log", abs_builddir); - if (virFileReadAll(expectname, 1024*64, &expectlog) < 0) { - fprintf(stderr, "cannot read %s\n", expectname); - goto cleanup; - } - if (virFileReadAll(actualname, 1024*64, &actuallog) < 0) { fprintf(stderr, "cannot read %s\n", actualname); goto cleanup; } - if (STRNEQ(expectlog, actuallog)) { - virTestDifference(stderr, expectlog, actuallog); - goto cleanup; - } - - ret = 0; + ret = virTestCompareToFile(actuallog, expectname); cleanup: if (actualname) @@ -1292,6 +1281,7 @@ mymain(void) * since we're about to reset 'environ' */ ignore_value(virTestGetDebug()); ignore_value(virTestGetVerbose()); + ignore_value(virTestGetRegenerate()); /* Make sure to not leak fd's */ virinitret = virInitialize(); -- 2.37.4
Two things are happening here: 1) Call to virTestDifference() is guarded by '!result || STRNEQ(result, _)' check. This is suboptimal since we have STRNEQ_NULLABLE(). 2) There are couple of VIR_TEST_DEBUG() printings, which are useless. If debug is off they don't print anything, and if it is on, then much more information is printed by subsequent virTestDifference(). This makes the STRNEQ() + virTestDifference() combo look similar to the rest of tests and thus can be picked up by spatch later. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virbuftest.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/virbuftest.c b/tests/virbuftest.c index 0ca7927c3f..144df6e66b 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -92,7 +92,7 @@ static int testBufAutoIndent(const void *data G_GNUC_UNUSED) virBufferAddChar(buf, '\n'); result = virBufferContentAndReset(buf); - if (!result || STRNEQ(result, expected)) { + if (STRNEQ_NULLABLE(result, expected)) { virTestDifference(stderr, expected, result); ret = -1; } @@ -122,7 +122,7 @@ static int testBufTrim(const void *data G_GNUC_UNUSED) virBufferTrim(buf, ",,"); result = virBufferContentAndReset(buf); - if (!result || STRNEQ(result, expected)) { + if (STRNEQ_NULLABLE(result, expected)) { virTestDifference(stderr, expected, result); return -1; } @@ -146,7 +146,6 @@ testBufTrimChars(const void *opaque) } if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufEscapeStr(): Strings don't match:"); virTestDifference(stderr, data->expect, actual); return -1; } @@ -278,7 +277,6 @@ testBufAddStr(const void *opaque) } if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufAddStr(): Strings don't match:"); virTestDifference(stderr, data->expect, actual); return -1; } @@ -306,7 +304,6 @@ testBufEscapeStr(const void *opaque) } if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufEscapeStr(): Strings don't match:"); virTestDifference(stderr, data->expect, actual); return -1; } @@ -330,7 +327,6 @@ testBufEscapeRegex(const void *opaque) } if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufEscapeRegex: Strings don't match:"); virTestDifference(stderr, data->expect, actual); return -1; } -- 2.37.4
The virTestDifference() is perfectly capable of handling NULL arguments. There's no need to wrap arguments in NULLSTR(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 4 +--- tests/virnetdevopenvswitchtest.c | 8 ++------ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index d5e4092fc3..fced657811 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -93,9 +93,7 @@ testVirNetDevBandwidthSet(const void *data) } if (STRNEQ_NULLABLE(exp_cmd, actual_cmd)) { - virTestDifference(stderr, - NULLSTR(exp_cmd), - NULLSTR(actual_cmd)); + virTestDifference(stderr, exp_cmd, actual_cmd); return -1; } diff --git a/tests/virnetdevopenvswitchtest.c b/tests/virnetdevopenvswitchtest.c index 57860f4bc4..e5883eb076 100644 --- a/tests/virnetdevopenvswitchtest.c +++ b/tests/virnetdevopenvswitchtest.c @@ -176,9 +176,7 @@ testVirNetDevOpenvswitchInterfaceSetQos(const void *data) } if (STRNEQ_NULLABLE(info->exp_cmd, actual_cmd)) { - virTestDifference(stderr, - NULLSTR(info->exp_cmd), - NULLSTR(actual_cmd)); + virTestDifference(stderr, info->exp_cmd, actual_cmd); return -1; } @@ -207,9 +205,7 @@ testVirNetDevOpenvswitchInterfaceClearQos(const void *data) } if (STRNEQ_NULLABLE(info->exp_cmd, actual_cmd)) { - virTestDifference(stderr, - NULLSTR(info->exp_cmd), - NULLSTR(actual_cmd)); + virTestDifference(stderr, info->exp_cmd, actual_cmd); return -1; } -- 2.37.4
Instead of using: if (STRNEQ(a, b)) { virTestDifference(stderr, a, b); ... } we can use: if (virTestCompareToString(a, b) < ) { ... } Generated by the following spatch: @@ expression a, b; @@ - if (STRNEQ(a, b)) { + if (virTestCompareToString(a, b) < 0) { ... - virTestDifference(stderr, a, b); ... } and its variations (STRNEQ_NULLABLE() instead of STRNEQ(), then in some cases variables passed to STRNEQ() are in reversed order when compared to virTestCompareToString()). However, coccinelle failed to recognize the pattern in testNWFilterEBIPTablesAllTeardown() so I had to fix it manually. Also, I manually fixed testFormat() in tests/sockettest.c as I didn't bother writing another spatch rule just for that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/commandtest.c | 33 +++++++++++--------------------- tests/esxutilstest.c | 10 +++------- tests/nwfilterebiptablestest.c | 21 +++++++------------- tests/openvzutilstest.c | 6 ++---- tests/sockettest.c | 3 +-- tests/utiltest.c | 3 +-- tests/vboxsnapshotxmltest.c | 3 +-- tests/virbuftest.c | 21 +++++++------------- tests/virfirewalltest.c | 30 ++++++++++------------------- tests/virjsontest.c | 6 ++---- tests/virkmodtest.c | 3 +-- tests/virnetdevbandwidthtest.c | 3 +-- tests/virnetdevopenvswitchtest.c | 6 ++---- tests/virnetsockettest.c | 3 +-- tests/virshtest.c | 3 +-- 15 files changed, 51 insertions(+), 103 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index ffc4b24ef4..62275ba96d 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -436,8 +436,7 @@ static int test13(const void *unused G_GNUC_UNUSED) g_clear_pointer(&cmd, virCommandFree); - if (STRNEQ(outactual, outexpect)) { - virTestDifference(stderr, outexpect, outactual); + if (virTestCompareToString(outexpect, outactual) < 0) { goto cleanup; } @@ -497,16 +496,13 @@ static int test14(const void *unused G_GNUC_UNUSED) if (!jointactual) goto cleanup; - if (STRNEQ(outactual, outexpect)) { - virTestDifference(stderr, outexpect, outactual); + if (virTestCompareToString(outexpect, outactual) < 0) { goto cleanup; } - if (STRNEQ(erractual, errexpect)) { - virTestDifference(stderr, errexpect, erractual); + if (virTestCompareToString(errexpect, erractual) < 0) { goto cleanup; } - if (STRNEQ(jointactual, jointexpect)) { - virTestDifference(stderr, jointexpect, jointactual); + if (virTestCompareToString(jointexpect, jointactual) < 0) { goto cleanup; } @@ -569,8 +565,7 @@ static int test16(const void *unused G_GNUC_UNUSED) return -1; } - if (STRNEQ(outactual, outexpect)) { - virTestDifference(stderr, outexpect, outactual); + if (virTestCompareToString(outexpect, outactual) < 0) { return -1; } @@ -774,13 +769,11 @@ static int test21(const void *unused G_GNUC_UNUSED) if (virTestGetVerbose()) printf("STDOUT:%s\nSTDERR:%s\n", NULLSTR(outbuf), NULLSTR(errbuf)); - if (STRNEQ_NULLABLE(outbuf, outbufExpected)) { - virTestDifference(stderr, outbufExpected, outbuf); + if (virTestCompareToString(outbufExpected, outbuf) < 0) { return -1; } - if (STRNEQ_NULLABLE(errbuf, errbufExpected)) { - virTestDifference(stderr, errbufExpected, errbuf); + if (virTestCompareToString(errbufExpected, errbuf) < 0) { return -1; } @@ -1016,8 +1009,7 @@ static int test26(const void *unused G_GNUC_UNUSED) return -1; } - if (STRNEQ(outactual, outexpect)) { - virTestDifference(stderr, outexpect, outactual); + if (virTestCompareToString(outexpect, outactual) < 0) { return -1; } @@ -1086,12 +1078,10 @@ static int test27(const void *unused G_GNUC_UNUSED) if (!outactual || !erractual) return -1; - if (STRNEQ(outactual, outexpect)) { - virTestDifference(stderr, outexpect, outactual); + if (virTestCompareToString(outexpect, outactual) < 0) { return -1; } - if (STRNEQ(erractual, errexpect)) { - virTestDifference(stderr, errexpect, erractual); + if (virTestCompareToString(errexpect, erractual) < 0) { return -1; } @@ -1225,8 +1215,7 @@ test29(const void *unused G_GNUC_UNUSED) } } - if (STRNEQ_NULLABLE(outactual, outexpect)) { - virTestDifference(stderr, outexpect, outactual); + if (virTestCompareToString(outexpect, outactual) < 0) { goto cleanup; } diff --git a/tests/esxutilstest.c b/tests/esxutilstest.c index 4591cf49db..5cebc10e44 100644 --- a/tests/esxutilstest.c +++ b/tests/esxutilstest.c @@ -50,19 +50,15 @@ testParseDatastorePath(const void *data G_GNUC_UNUSED) if (paths[i].result < 0) continue; - if (STRNEQ(paths[i].datastoreName, datastoreName)) { - virTestDifference(stderr, paths[i].datastoreName, datastoreName); + if (virTestCompareToString(paths[i].datastoreName, datastoreName) < 0) { return -1; } - if (STRNEQ(paths[i].directoryName, directoryName)) { - virTestDifference(stderr, paths[i].directoryName, directoryName); + if (virTestCompareToString(paths[i].directoryName, directoryName) < 0) { return -1; } - if (STRNEQ(paths[i].directoryAndFileName, directoryAndFileName)) { - virTestDifference(stderr, paths[i].directoryAndFileName, - directoryAndFileName); + if (virTestCompareToString(paths[i].directoryAndFileName, directoryAndFileName) < 0) { return -1; } } diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c index f76f13fd25..fe0d8e869c 100644 --- a/tests/nwfilterebiptablestest.c +++ b/tests/nwfilterebiptablestest.c @@ -107,8 +107,7 @@ testNWFilterEBIPTablesAllTeardown(const void *opaque G_GNUC_UNUSED) actual = virBufferContentAndReset(&buf); - if (STRNEQ_NULLABLE(actual, expected)) { - virTestDifference(stderr, expected, actual); + if (virTestCompareToString(actual, expected) < 0) { return -1; } @@ -169,8 +168,7 @@ testNWFilterEBIPTablesTearOldRules(const void *opaque G_GNUC_UNUSED) actual = virBufferContentAndReset(&buf); - if (STRNEQ_NULLABLE(actual, expected)) { - virTestDifference(stderr, expected, actual); + if (virTestCompareToString(expected, actual) < 0) { return -1; } @@ -209,8 +207,7 @@ testNWFilterEBIPTablesRemoveBasicRules(const void *opaque G_GNUC_UNUSED) actual = virBufferContentAndReset(&buf); - if (STRNEQ_NULLABLE(actual, expected)) { - virTestDifference(stderr, expected, actual); + if (virTestCompareToString(expected, actual) < 0) { return -1; } @@ -234,8 +231,7 @@ testNWFilterEBIPTablesTearNewRules(const void *opaque G_GNUC_UNUSED) actual = virBufferContentAndReset(&buf); - if (STRNEQ_NULLABLE(actual, expected)) { - virTestDifference(stderr, expected, actual); + if (virTestCompareToString(expected, actual) < 0) { return -1; } @@ -297,8 +293,7 @@ testNWFilterEBIPTablesApplyBasicRules(const void *opaque G_GNUC_UNUSED) actual = virBufferContentAndReset(&buf); - if (STRNEQ_NULLABLE(actual, expected)) { - virTestDifference(stderr, expected, actual); + if (virTestCompareToString(expected, actual) < 0) { return -1; } @@ -378,8 +373,7 @@ testNWFilterEBIPTablesApplyDHCPOnlyRules(const void *opaque G_GNUC_UNUSED) actual = virBufferContentAndReset(&buf); - if (STRNEQ_NULLABLE(actual, expected)) { - virTestDifference(stderr, expected, actual); + if (virTestCompareToString(expected, actual) < 0) { return -1; } @@ -442,8 +436,7 @@ testNWFilterEBIPTablesApplyDropAllRules(const void *opaque G_GNUC_UNUSED) actual = virBufferContentAndReset(&buf); - if (STRNEQ_NULLABLE(actual, expected)) { - virTestDifference(stderr, expected, actual); + if (virTestCompareToString(expected, actual) < 0) { return -1; } diff --git a/tests/openvzutilstest.c b/tests/openvzutilstest.c index 77e086e390..6ebfb0449f 100644 --- a/tests/openvzutilstest.c +++ b/tests/openvzutilstest.c @@ -49,8 +49,7 @@ testReadConfigParam(const void *data G_GNUC_UNUSED) if (configParams[i].ret != 1) continue; - if (STRNEQ(configParams[i].value, value)) { - virTestDifference(stderr, configParams[i].value, value); + if (virTestCompareToString(configParams[i].value, value) < 0) { return -1; } } @@ -114,8 +113,7 @@ testReadNetworkConf(const void *data G_GNUC_UNUSED) goto cleanup; } - if (STRNEQ(expected, actual)) { - virTestDifference(stderr, expected, actual); + if (virTestCompareToString(expected, actual) < 0) { goto cleanup; } diff --git a/tests/sockettest.c b/tests/sockettest.c index f2b75a2e53..6b9063e11d 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -48,8 +48,7 @@ static int testFormat(virSocketAddr *addr, const char *addrstr, bool pass) if (!newaddrstr) return pass ? -1 : 0; - if (STRNEQ(newaddrstr, addrstr)) { - virTestDifference(stderr, addrstr, newaddrstr); + if (virTestCompareToString(newaddrstr, addrstr) < 0) { return pass ? -1 : 0; } else { return pass ? 0 : -1; diff --git a/tests/utiltest.c b/tests/utiltest.c index e7a79f808f..e90baca65f 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -47,8 +47,7 @@ testIndexToDiskName(const void *data G_GNUC_UNUSED) diskName = virIndexToDiskName(i, "sd"); - if (STRNEQ(diskNames[i], diskName)) { - virTestDifference(stderr, diskNames[i], diskName); + if (virTestCompareToString(diskNames[i], diskName) < 0) { return -1; } } diff --git a/tests/vboxsnapshotxmltest.c b/tests/vboxsnapshotxmltest.c index 3ad8298895..ab65999df1 100644 --- a/tests/vboxsnapshotxmltest.c +++ b/tests/vboxsnapshotxmltest.c @@ -69,8 +69,7 @@ testCompareXMLtoXMLFiles(const char *xml) if (!(xmlData = testFilterXML(xmlData))) goto cleanup; - if (STRNEQ(actual, xmlData)) { - virTestDifference(stderr, xmlData, actual); + if (virTestCompareToString(xmlData, actual) < 0) { goto cleanup; } diff --git a/tests/virbuftest.c b/tests/virbuftest.c index 144df6e66b..6b810381fb 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -92,8 +92,7 @@ static int testBufAutoIndent(const void *data G_GNUC_UNUSED) virBufferAddChar(buf, '\n'); result = virBufferContentAndReset(buf); - if (STRNEQ_NULLABLE(result, expected)) { - virTestDifference(stderr, expected, result); + if (virTestCompareToString(expected, result) < 0) { ret = -1; } return ret; @@ -122,8 +121,7 @@ static int testBufTrim(const void *data G_GNUC_UNUSED) virBufferTrim(buf, ",,"); result = virBufferContentAndReset(buf); - if (STRNEQ_NULLABLE(result, expected)) { - virTestDifference(stderr, expected, result); + if (virTestCompareToString(expected, result) < 0) { return -1; } @@ -145,8 +143,7 @@ testBufTrimChars(const void *opaque) return -1; } - if (STRNEQ_NULLABLE(actual, data->expect)) { - virTestDifference(stderr, data->expect, actual); + if (virTestCompareToString(data->expect, actual) < 0) { return -1; } @@ -250,8 +247,7 @@ static int testBufAddBuffer(const void *data G_GNUC_UNUSED) } result = virBufferContentAndReset(&buf1); - if (STRNEQ_NULLABLE(result, expected)) { - virTestDifference(stderr, expected, result); + if (virTestCompareToString(expected, result) < 0) { return -1; } @@ -276,8 +272,7 @@ testBufAddStr(const void *opaque) return -1; } - if (STRNEQ_NULLABLE(actual, data->expect)) { - virTestDifference(stderr, data->expect, actual); + if (virTestCompareToString(data->expect, actual) < 0) { return -1; } @@ -303,8 +298,7 @@ testBufEscapeStr(const void *opaque) return -1; } - if (STRNEQ_NULLABLE(actual, data->expect)) { - virTestDifference(stderr, data->expect, actual); + if (virTestCompareToString(data->expect, actual) < 0) { return -1; } @@ -326,8 +320,7 @@ testBufEscapeRegex(const void *opaque) return -1; } - if (STRNEQ_NULLABLE(actual, data->expect)) { - virTestDifference(stderr, data->expect, actual); + if (virTestCompareToString(data->expect, actual) < 0) { return -1; } diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index e3d15fb67b..51c8006331 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -89,9 +89,8 @@ testFirewallSingleGroup(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -136,9 +135,8 @@ testFirewallRemoveRule(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -190,9 +188,8 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -265,9 +262,8 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -318,9 +314,8 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -365,9 +360,8 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -431,9 +425,8 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -500,9 +493,8 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -599,9 +591,8 @@ testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) actual = virBufferCurrentContent(&cmdbuf); - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } @@ -763,9 +754,8 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) return -1; } - if (STRNEQ_NULLABLE(expected, actual)) { + if (virTestCompareToString(expected, actual) < 0) { fprintf(stderr, "Unexpected command execution\n"); - virTestDifference(stderr, expected, actual); return -1; } diff --git a/tests/virjsontest.c b/tests/virjsontest.c index 78283b632a..294889a795 100644 --- a/tests/virjsontest.c +++ b/tests/virjsontest.c @@ -93,8 +93,7 @@ testJSONFromString(const void *data) return -1; } - if (STRNEQ(expectstr, formatted)) { - virTestDifference(stderr, expectstr, formatted); + if (virTestCompareToString(expectstr, formatted) < 0) { return -1; } @@ -424,8 +423,7 @@ testJSONEscapeObj(const void *data G_GNUC_UNUSED) return -1; } - if (STRNEQ(parsednestedstr, neststr)) { - virTestDifference(stderr, neststr, parsednestedstr); + if (virTestCompareToString(neststr, parsednestedstr) < 0) { return -1; } diff --git a/tests/virkmodtest.c b/tests/virkmodtest.c index 0e662878b2..ec28ef1282 100644 --- a/tests/virkmodtest.c +++ b/tests/virkmodtest.c @@ -40,8 +40,7 @@ checkOutput(virBuffer *buf, const char *exp_cmd) return -1; } - if (STRNEQ(exp_cmd, actual_cmd)) { - virTestDifference(stderr, exp_cmd, actual_cmd); + if (virTestCompareToString(exp_cmd, actual_cmd) < 0) { return -1; } diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index fced657811..f7c38faa2e 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -92,8 +92,7 @@ testVirNetDevBandwidthSet(const void *data) * Maybe that's expected, actually. */ } - if (STRNEQ_NULLABLE(exp_cmd, actual_cmd)) { - virTestDifference(stderr, exp_cmd, actual_cmd); + if (virTestCompareToString(exp_cmd, actual_cmd) < 0) { return -1; } diff --git a/tests/virnetdevopenvswitchtest.c b/tests/virnetdevopenvswitchtest.c index e5883eb076..6e93f5e65a 100644 --- a/tests/virnetdevopenvswitchtest.c +++ b/tests/virnetdevopenvswitchtest.c @@ -175,8 +175,7 @@ testVirNetDevOpenvswitchInterfaceSetQos(const void *data) * Maybe that's expected, actually. */ } - if (STRNEQ_NULLABLE(info->exp_cmd, actual_cmd)) { - virTestDifference(stderr, info->exp_cmd, actual_cmd); + if (virTestCompareToString(info->exp_cmd, actual_cmd) < 0) { return -1; } @@ -204,8 +203,7 @@ testVirNetDevOpenvswitchInterfaceClearQos(const void *data) * Maybe that's expected, actually. */ } - if (STRNEQ_NULLABLE(info->exp_cmd, actual_cmd)) { - virTestDifference(stderr, info->exp_cmd, actual_cmd); + if (virTestCompareToString(info->exp_cmd, actual_cmd) < 0) { return -1; } diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 396005899a..2295e29777 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -488,8 +488,7 @@ static int testSocketSSH(const void *opaque) } buf[rv] = '\0'; - if (STRNEQ(buf, data->expectOut)) { - virTestDifference(stderr, data->expectOut, buf); + if (virTestCompareToString(data->expectOut, buf) < 0) { goto cleanup; } diff --git a/tests/virshtest.c b/tests/virshtest.c index 3d297a1db2..cf834bb847 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -134,8 +134,7 @@ testCompareOutputLit(const char *expectData, if (filter && testFilterLine(actualData, filter) < 0) return -1; - if (STRNEQ(expectData, actualData)) { - virTestDifference(stderr, expectData, actualData); + if (virTestCompareToString(expectData, actualData) < 0) { return -1; } -- 2.37.4
On 11/30/22 3:48 AM, Michal Privoznik wrote:
Found these on a abandoned branch, but they are still worth merging. Polished them a bit though.
Michal Prívozník (9): vircommand: Document virCommandSetSendBuffer() behaviour wrt daemonize virCommandDoAsyncIO: Drop misleading statement about main event loop commandtest: Use unsigned char in test27() virCommandSetSendBuffer: Take double pointer of @buffer commandtest: Test virCommandSetSendBuffer() with virCommandDoAsyncIO() commandtest: Use virTestCompareToFile() in checkoutput() virbuftest: Cleanup code around virTestDifference() tests: Don't wrap virTestDifference() arguments in NULLSTR() tests: Use virTestCompareToString() more
src/qemu/qemu_tpm.c | 2 +- src/util/vircommand.c | 11 ++- src/util/vircommand.h | 2 +- tests/commanddata/test29.log | 20 ++++ tests/commandtest.c | 152 +++++++++++++++++++++++-------- tests/esxutilstest.c | 10 +- tests/nwfilterebiptablestest.c | 21 ++--- tests/openvzutilstest.c | 6 +- tests/sockettest.c | 3 +- tests/utiltest.c | 3 +- tests/vboxsnapshotxmltest.c | 3 +- tests/virbuftest.c | 25 ++--- tests/virfirewalltest.c | 30 ++---- tests/virjsontest.c | 6 +- tests/virkmodtest.c | 3 +- tests/virnetdevbandwidthtest.c | 5 +- tests/virnetdevopenvswitchtest.c | 10 +- tests/virnetsockettest.c | 3 +- tests/virshtest.c | 3 +- 19 files changed, 182 insertions(+), 136 deletions(-) create mode 100644 tests/commanddata/test29.log
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
participants (2)
-
Jonathon Jongsma -
Michal Privoznik