virCommandSetSendBuffer: Provide saner semantics

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@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index e00313543c405b6cb8bfd665b3e00e5c97d99359..65a6d3781b354d13e3c35051aaf02e2a5866bafa 100644 (file)
--- 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 7f5e053efcd1df7859b23907f66ebe24c6f7feb8..6a189babca0fc012be73a7cdfcf2b938bbaa9134 100644 (file)
--- 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];
 }
 
 
@@ -2851,7 +2867,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 9fb625ec4baf9a52bfb13190bb93a310a9b2d053..a00f30f51fc9ca710cfb5f7d841e3c4ee7574b0d 100644 (file)
--- 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 df86725f0e7befa576450ab49de606c082475d80..524c959ebadbf418a34218e43254c85155ee51bb 100644 (file)
--- 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;
 }