[PATCH v2 0/7] Do async IO when starting swtpm emulator
v2 of: https://listman.redhat.com/archives/libvir-list/2022-March/229433.html diff to v1: - More fixes - New test case, yay! Michal Prívozník (7): qemu_tpm: Do async IO when starting swtpm emulator vircommand: Document virCommandSetSendBuffer() behaviour wrt daemonize vircommand: Don't set nonblocking FDs in virCommandDoAsyncIO() commandtest: Use unsigned char in test27() virCommandSetSendBuffer: Take double pointer of @buffer commandtest: Test virCommandSetSendBuffer() with virCommandDoAsyncIO() commandtest: Use virTestCompareToFile() in checkoutput() src/qemu/qemu_tpm.c | 3 +- src/util/vircommand.c | 12 ++-- src/util/vircommand.h | 2 +- tests/commanddata/test29.log | 19 ++++++ tests/commandtest.c | 110 +++++++++++++++++++++++++++++------ 5 files changed, 121 insertions(+), 25 deletions(-) create mode 100644 tests/commanddata/test29.log -- 2.34.1
When vTPM is secured via virSecret libvirt passes the secret value via an FD when swtpm is started (arguments --key and --migration-key). The writing of the secret into the FDs is handled via virCommand, specifically qemu_tpm calls virCommandSetSendBuffer()) and then virCommandRunAsync() spawns a thread to handle writing into the FD via virCommandDoAsyncIOHelper. But the thread is not created unless VIR_EXEC_ASYNC_IO flag is set, which it isn't. In order to fix it, virCommandDoAsyncIO() must be called. The credit goes to Marc-André Lureau <marcandre.lureau@redhat.com> who has done all the debugging and proposed fix in the bugzilla. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2064115 Fixes: a9c500d2b50c5c041a1bb6ae9724402cf1cec8fe Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 50f9caabf3..56bccee128 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -899,6 +899,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, if (!(pidfile = qemuTPMEmulatorPidFileBuildPath(cfg->swtpmStateDir, shortName))) return -1; + virCommandDoAsyncIO(cmd); virCommandDaemonize(cmd); virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); -- 2.34.1
On Tue, Mar 22, 2022 at 17:02:00 +0100, Michal Privoznik wrote:
When vTPM is secured via virSecret libvirt passes the secret value via an FD when swtpm is started (arguments --key and --migration-key). The writing of the secret into the FDs is handled via virCommand, specifically qemu_tpm calls virCommandSetSendBuffer()) and then virCommandRunAsync() spawns a thread to handle writing into the FD via virCommandDoAsyncIOHelper. But the thread is not created unless VIR_EXEC_ASYNC_IO flag is set, which it isn't. In order to fix it, virCommandDoAsyncIO() must be called.
The credit goes to Marc-André Lureau <marcandre.lureau@redhat.com> who has done all the debugging and proposed fix in the bugzilla.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2064115 Fixes: a9c500d2b50c5c041a1bb6ae9724402cf1cec8fe Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 50f9caabf3..56bccee128 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -899,6 +899,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, if (!(pidfile = qemuTPMEmulatorPidFileBuildPath(cfg->swtpmStateDir, shortName))) return -1;
+ virCommandDoAsyncIO(cmd); virCommandDaemonize(cmd); virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); -- 2.34.1
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When virCommandSetSendBuffer() is used over a virCommand that is (or will be) daemonized, then VIR_EXEC_ASYNC_IO 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 41cf552d7b..5f22bd0ac3 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1719,6 +1719,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.34.1
Hi On Tue, Mar 22, 2022 at 8:02 PM Michal Privoznik <mprivozn@redhat.com> wrote:
When virCommandSetSendBuffer() is used over a virCommand that is (or will be) daemonized, then VIR_EXEC_ASYNC_IO 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 41cf552d7b..5f22bd0ac3 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1719,6 +1719,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().
Or else the RunAsync() should return an error, no? Why not call DoAsyncIO() implicitly in RunAsync() in this case? (sorry to repeat maybe my earlier question, trying to be more precise :) thanks -- Marc-André Lureau
On 3/23/22 08:48, Marc-André Lureau wrote:
Hi
On Tue, Mar 22, 2022 at 8:02 PM Michal Privoznik <mprivozn@redhat.com <mailto:mprivozn@redhat.com>> wrote:
When virCommandSetSendBuffer() is used over a virCommand that is (or will be) daemonized, then VIR_EXEC_ASYNC_IO 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 <mailto: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 41cf552d7b..5f22bd0ac3 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1719,6 +1719,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().
Or else the RunAsync() should return an error, no?
Yes.
Why not call DoAsyncIO() implicitly in RunAsync() in this case?
So what we could do, is to enable VIR_EXEC_ASYNC_IO flag in virCommandRun() and require explicit call to DoAsyncIO() for virCommandRunAsync() case. This way, it'll be consistent with the rest of buffer passing functions, like virCommandSetInputBuffer(), virCommandSetOutputBuffer() or virCommandSetErrorBuffer(). Would this work for you? Michal
Back in 2013 (v1.0.3-rc1~235) when I introduced virCommandDoAsyncIO() things were way different than today. We had one event loop for everything and asynchronous IO for virCommand was handled by the event loop. Therefore, it made sense to enable VIR_EXEC_NONBLOCK flag alongside with VIR_EXEC_ASYNC_IO. Well, sort of - a blocking FD can still be put into poll(). Anyway, this was reimplemented in v1.0.3-rc1~127 when a separate thread is created that handles IO. Therefore, there's no need to enable VIR_EXEC_NONBLOCK flag anymore. There's a separate API to request that flag: virCommandNonblockingFDs(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 5f22bd0ac3..ebd986cb13 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -3105,7 +3105,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 @@ -3117,7 +3116,7 @@ virCommandDoAsyncIO(virCommand *cmd) if (virCommandHasError(cmd)) return; - cmd->flags |= VIR_EXEC_ASYNC_IO | VIR_EXEC_NONBLOCK; + cmd->flags |= VIR_EXEC_ASYNC_IO; } -- 2.34.1
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 74e60a072b..9d4de151f0 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1043,8 +1043,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; @@ -1057,8 +1057,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'; @@ -1077,8 +1077,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.34.1
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 56bccee128..28d2b07903 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; - return virCommandSetSendBuffer(cmd, g_steal_pointer(&secret), secret_len); + return virCommandSetSendBuffer(cmd, &secret, secret_len); } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index ebd986cb13..ff0e60936b 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1725,10 +1725,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; @@ -2927,7 +2927,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 600806a987..63ae9b8a61 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -140,7 +140,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 9d4de151f0..10a051124d 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1077,8 +1077,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.34.1
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 | 19 ++++++++ tests/commandtest.c | 84 ++++++++++++++++++++++++++++++++++++ 2 files changed, 103 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..49993a4947 --- /dev/null +++ b/tests/commanddata/test29.log @@ -0,0 +1,19 @@ +ARG:--close-stdin +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 10a051124d..573a4f250d 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> @@ -1157,6 +1158,88 @@ 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, "--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 }; + int rc = 0; + + rc = read(outfd, buf, sizeof(buf)); + if (rc < 0) { + fprintf(stderr, "cannot read from output pipe: errno=%d\n", errno); + goto cleanup; + } + + if (rc == 0) + break; + + outactual = g_renew(char, outactual, outactuallen + rc + 1); + memcpy(outactual + outactuallen, buf, rc); + outactuallen += rc; + outactual[outactuallen] = '\0'; + } + + 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) { @@ -1254,6 +1337,7 @@ mymain(void) DO_TEST(test26); DO_TEST(test27); DO_TEST(test28); + DO_TEST(test29); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.34.1
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 573a4f250d..60daf6e48f 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -60,29 +60,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) @@ -1281,6 +1270,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.34.1
participants (4)
-
Jiri Denemark -
Marc-André Lureau -
Michal Privoznik -
Michal Prívozník