[PATCH 5/6] virCommandSetSendBuffer: Provide saner semantics
Peter Krempa
pkrempa at redhat.com
Tue Mar 2 16:12:07 UTC 2021
The function is used to automatically feed a buffer into a pipe which
can be used by the command to read contents of the buffer.
Rather than passing in a pipe, let's create the pipe inside
virCommandSetSendBuffer and directly associate the reader end with the
command. This way the ownership of both ends of the pipe will end up
with the virCommand right away reducing the need of cleanup in callers.
The returned value then can be used just to format the appropriate
arguments without worrying about cleanup or failure.
Signed-off-by: Peter Krempa <pkrempa at redhat.com>
---
src/qemu/qemu_tpm.c | 50 +++++++++----------------------------------
src/util/vircommand.c | 39 ++++++++++++++++++++++-----------
src/util/vircommand.h | 6 +++---
tests/commandtest.c | 32 ++++++---------------------
4 files changed, 46 insertions(+), 81 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index d994816484..d8c518fc73 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -353,14 +353,14 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
* This function reads the passphrase and writes it into the
* write-end of a pipe so that the read-end of the pipe can be
* passed to the emulator for reading the passphrase from.
+ *
+ * Note that the returned FD is owned by @cmd.
*/
static int
qemuTPMSetupEncryption(const unsigned char *secretuuid,
virCommandPtr cmd)
{
- int ret = -1;
- int pipefd[2] = { -1, -1 };
- virConnectPtr conn;
+ g_autoptr(virConnect) conn = NULL;
g_autofree uint8_t *secret = NULL;
size_t secret_len;
virSecretLookupTypeDef seclookupdef = {
@@ -375,27 +375,9 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid,
if (virSecretGetSecretString(conn, &seclookupdef,
VIR_SECRET_USAGE_TYPE_VTPM,
&secret, &secret_len) < 0)
- goto error;
-
- if (virPipe(pipefd) < 0)
- goto error;
-
- if (virCommandSetSendBuffer(cmd, pipefd[1], secret, secret_len) < 0)
- goto error;
-
- secret = NULL;
- ret = pipefd[0];
-
- cleanup:
- virObjectUnref(conn);
-
- return ret;
-
- error:
- VIR_FORCE_CLOSE(pipefd[1]);
- VIR_FORCE_CLOSE(pipefd[0]);
+ return -1;
- goto cleanup;
+ return virCommandSetSendBuffer(cmd, g_steal_pointer(&secret), secret_len);
}
/*
@@ -549,8 +531,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
bool created = false;
g_autofree char *pidfile = NULL;
g_autofree char *swtpm = virTPMGetSwtpm();
- VIR_AUTOCLOSE pwdfile_fd = -1;
- VIR_AUTOCLOSE migpwdfile_fd = -1;
+ int pwdfile_fd = -1;
+ int migpwdfile_fd = -1;
const unsigned char *secretuuid = NULL;
if (!swtpm)
@@ -618,25 +600,13 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
}
pwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, cmd);
- if (pwdfile_fd) {
- migpwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid,
- cmd);
- }
-
- if (pwdfile_fd < 0 || migpwdfile_fd < 0)
- goto error;
+ migpwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, cmd);
virCommandAddArg(cmd, "--key");
- virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc",
- pwdfile_fd);
- virCommandPassFD(cmd, pwdfile_fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
- pwdfile_fd = -1;
+ virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", pwdfile_fd);
virCommandAddArg(cmd, "--migration-key");
- virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc",
- migpwdfile_fd);
- virCommandPassFD(cmd, migpwdfile_fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
- migpwdfile_fd = -1;
+ virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", migpwdfile_fd);
}
return g_steal_pointer(&cmd);
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 579c77fd9f..04964b730b 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1694,39 +1694,55 @@ virCommandFreeSendBuffers(virCommandPtr cmd)
/**
* virCommandSetSendBuffer
* @cmd: the command to modify
+ * @buffer: buffer to pass to the filedescriptror
+ * @buflen: length of @buffer
*
- * Pass a buffer to virCommand that will be written into the
- * given file descriptor. The buffer will be freed automatically
- * and the file descriptor closed.
+ * Registers @buffer as an input buffer for @cmd which will be accessible via
+ * the returned file descriptor. The returned file descriptor is already
+ * registered to be passed to @cmd, so callers must use it only to format the
+ * appropriate argument of @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.
*/
int
virCommandSetSendBuffer(virCommandPtr cmd,
- int fd,
- unsigned char *buffer, size_t buflen)
+ unsigned char *buffer,
+ size_t buflen)
{
+ g_autofree unsigned char *localbuf = g_steal_pointer(&buffer);
+ int pipefd[2] = { -1, -1 };
size_t i;
if (virCommandHasError(cmd))
return -1;
- if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) {
- virReportSystemError(errno, "%s",
- _("fcntl failed to set O_NONBLOCK"));
+ if (virPipeQuiet(pipefd) < 0) {
+ cmd->has_error = errno;
+ return -1;
+ }
+
+ if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) < 0) {
cmd->has_error = errno;
+ VIR_FORCE_CLOSE(pipefd[0]);
+ VIR_FORCE_CLOSE(pipefd[1]);
return -1;
}
i = virCommandGetNumSendBuffers(cmd);
ignore_value(VIR_REALLOC_N(cmd->sendBuffers, i + 1));
- cmd->sendBuffers[i].fd = fd;
- cmd->sendBuffers[i].buffer = buffer;
+ cmd->sendBuffers[i].fd = pipefd[1];
+ cmd->sendBuffers[i].buffer = g_steal_pointer(&localbuf);
cmd->sendBuffers[i].buflen = buflen;
cmd->sendBuffers[i].offset = 0;
cmd->numSendBuffers++;
- return 0;
+ virCommandPassFD(cmd, pipefd[0], VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+
+ return pipefd[0];
}
@@ -2835,7 +2851,6 @@ int virCommandHandshakeNotify(virCommandPtr cmd)
#else /* WIN32 */
int
virCommandSetSendBuffer(virCommandPtr cmd,
- int fd G_GNUC_UNUSED,
unsigned char *buffer G_GNUC_UNUSED,
size_t buflen G_GNUC_UNUSED)
{
diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index 9fb625ec4b..a00f30f51f 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -141,9 +141,9 @@ void virCommandSetWorkingDirectory(virCommandPtr cmd,
const char *pwd) ATTRIBUTE_NONNULL(2);
int virCommandSetSendBuffer(virCommandPtr cmd,
- int fd,
- unsigned char *buffer, size_t buflen)
- ATTRIBUTE_NONNULL(3);
+ unsigned char *buffer,
+ size_t buflen)
+ ATTRIBUTE_NONNULL(2);
void virCommandSetInputBuffer(virCommandPtr cmd,
const char *inbuf) ATTRIBUTE_NONNULL(2);
diff --git a/tests/commandtest.c b/tests/commandtest.c
index df86725f0e..524c959eba 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -1039,8 +1039,8 @@ static int test26(const void *unused G_GNUC_UNUSED)
static int test27(const void *unused G_GNUC_UNUSED)
{
g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper");
- int pipe1[2];
- int pipe2[2];
+ int buf1fd;
+ int buf2fd;
int ret = -1;
size_t buflen = 1024 * 128;
g_autofree char *buffer0 = NULL;
@@ -1078,30 +1078,14 @@ static int test27(const void *unused G_GNUC_UNUSED)
errexpect = g_strdup_printf(TEST27_ERREXPECT_TEMP,
buffer0, buffer1, buffer2);
- if (virPipeQuiet(pipe1) < 0 || virPipeQuiet(pipe2) < 0) {
- printf("Could not create pipe: %s\n", g_strerror(errno));
- goto cleanup;
- }
-
- if (virCommandSetSendBuffer(cmd, pipe1[1],
- (unsigned char *)buffer1, buflen - 1) < 0 ||
- virCommandSetSendBuffer(cmd, pipe2[1],
- (unsigned char *)buffer2, buflen - 1) < 0) {
- printf("Could not set send buffers\n");
- goto cleanup;
- }
- pipe1[1] = -1;
- pipe2[1] = -1;
- buffer1 = NULL;
- buffer2 = NULL;
+ buf1fd = virCommandSetSendBuffer(cmd, (unsigned char *) g_steal_pointer(&buffer1), buflen - 1);
+ buf2fd = virCommandSetSendBuffer(cmd, (unsigned char *) g_steal_pointer(&buffer2), buflen - 1);
virCommandAddArg(cmd, "--readfd");
- virCommandAddArgFormat(cmd, "%d", pipe1[0]);
- virCommandPassFD(cmd, pipe1[0], 0);
+ virCommandAddArgFormat(cmd, "%d", buf1fd);
virCommandAddArg(cmd, "--readfd");
- virCommandAddArgFormat(cmd, "%d", pipe2[0]);
- virCommandPassFD(cmd, pipe2[0], 0);
+ virCommandAddArgFormat(cmd, "%d", buf2fd);
virCommandSetInputBuffer(cmd, buffer0);
virCommandSetOutputBuffer(cmd, &outactual);
@@ -1130,10 +1114,6 @@ static int test27(const void *unused G_GNUC_UNUSED)
ret = 0;
cleanup:
- VIR_FORCE_CLOSE(pipe1[0]);
- VIR_FORCE_CLOSE(pipe2[0]);
- VIR_FORCE_CLOSE(pipe1[1]);
- VIR_FORCE_CLOSE(pipe2[1]);
return ret;
}
--
2.29.2
More information about the libvir-list
mailing list