Merge virCommandPreserveFD / virCommandTransferFD

Merge the virCommandPreserveFD / virCommandTransferFD methods
into a single virCommandPasFD method, and use a new
VIR_COMMAND_PASS_FD_CLOSE_PARENT to indicate their difference
in behaviour

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

docs/internals/command.html.in

@@ -258,8 +258,9 @@
 <pre>
   int sharedfd = open("cmd.log", "w+");
   int childfd = open("conf.txt", "r");
-  virCommandPreserveFD(cmd, sharedfd);
+  virCommandPassFD(cmd, sharedfd, 0);
-  virCommandTransferFD(cmd, childfd);
+  virCommandPassFD(cmd, childfd,
+                   VIR_COMMAND_PASS_FD_CLOSE_PARENT);
   if (VIR_CLOSE(sharedfd) &lt; 0)
       goto cleanup;
 </pre>

src/fdstream.c

@@ -648,7 +648,8 @@ virFDStreamOpenFileInternal(virStreamPtr st,
                                    path,
                                    NULL);
         virCommandAddArgFormat(cmd, "%llu", length);
-        virCommandTransferFD(cmd, fd);
+        virCommandPassFD(cmd, fd,
+                         VIR_COMMAND_PASS_FD_CLOSE_PARENT);
         virCommandAddArgFormat(cmd, "%d", fd);
 
         if ((oflags & O_ACCMODE) == O_RDONLY) {

src/libvirt_private.syms

@@ -1230,7 +1230,7 @@ virCommandNew;
 virCommandNewArgList;
 virCommandNewArgs;
 virCommandNonblockingFDs;
-virCommandPreserveFD;
+virCommandPassFD;
 virCommandRequireHandshake;
 virCommandRun;
 virCommandRunAsync;
@@ -1251,7 +1251,6 @@ virCommandSetSELinuxLabel;
 virCommandSetUID;
 virCommandSetWorkingDirectory;
 virCommandToString;
-virCommandTransferFD;
 virCommandWait;
 virCommandWriteArgLog;
 virFork;

src/lxc/lxc_process.c

@@ -853,13 +853,13 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver,
     for (i = 0; i < nttyFDs; i++) {
         virCommandAddArg(cmd, "--console");
         virCommandAddArgFormat(cmd, "%d", ttyFDs[i]);
-        virCommandPreserveFD(cmd, ttyFDs[i]);
+        virCommandPassFD(cmd, ttyFDs[i], 0);
     }
 
     for (i = 0; i < nfiles; i++) {
         virCommandAddArg(cmd, "--passfd");
         virCommandAddArgFormat(cmd, "%d", files[i]);
-        virCommandPreserveFD(cmd, files[i]);
+        virCommandPassFD(cmd, files[i], 0);
     }
 
     virCommandAddArgPair(cmd, "--security",
@@ -873,7 +873,7 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver,
         virCommandAddArgList(cmd, "--veth", veths[i], NULL);
     }
 
-    virCommandPreserveFD(cmd, handshakefd);
+    virCommandPassFD(cmd, handshakefd, 0);
 
     return cmd;
 cleanup:

src/qemu/qemu_command.c

@@ -245,7 +245,8 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
         virCommandAddArgFormat(cmd, "--use-vnet");
     virCommandAddArgFormat(cmd, "--br=%s", brname);
     virCommandAddArgFormat(cmd, "--fd=%d", pair[1]);
-    virCommandTransferFD(cmd, pair[1]);
+    virCommandPassFD(cmd, pair[1],
+                     VIR_COMMAND_PASS_FD_CLOSE_PARENT);
     virCommandClearCaps(cmd);
 #ifdef CAP_NET_ADMIN
     virCommandAllowCap(cmd, CAP_NET_ADMIN);
@@ -6587,13 +6588,15 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
     }
 
     for (i = 0; i < tapfdSize; i++) {
-        virCommandTransferFD(cmd, tapfd[i]);
+        virCommandPassFD(cmd, tapfd[i],
+                         VIR_COMMAND_PASS_FD_CLOSE_PARENT);
         if (virAsprintf(&tapfdName[i], "%d", tapfd[i]) < 0)
             goto cleanup;
     }
 
     for (i = 0; i < vhostfdSize; i++) {
-        virCommandTransferFD(cmd, vhostfd[i]);
+        virCommandPassFD(cmd, vhostfd[i],
+                         VIR_COMMAND_PASS_FD_CLOSE_PARENT);
         if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0)
             goto cleanup;
     }
@@ -8365,7 +8368,8 @@ qemuBuildCommandLine(virConnectPtr conn,
                             goto error;
                         }
 
-                        virCommandTransferFD(cmd, configfd);
+                        virCommandPassFD(cmd, configfd,
+                                         VIR_COMMAND_PASS_FD_CLOSE_PARENT);
                     }
                 }
                 virCommandAddArg(cmd, "-device");
@@ -8431,7 +8435,7 @@ qemuBuildCommandLine(virConnectPtr conn,
         } else if (STREQ(migrateFrom, "stdio")) {
             if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) {
                 virCommandAddArgFormat(cmd, "fd:%d", migrateFd);
-                virCommandPreserveFD(cmd, migrateFd);
+                virCommandPassFD(cmd, migrateFd, 0);
             } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) {
                 virCommandAddArg(cmd, "exec:cat");
                 virCommandSetInputFD(cmd, migrateFd);
@@ -8460,7 +8464,7 @@ qemuBuildCommandLine(virConnectPtr conn,
                 goto error;
             }
             virCommandAddArg(cmd, migrateFrom);
-            virCommandPreserveFD(cmd, migrateFd);
+            virCommandPassFD(cmd, migrateFd, 0);
         } else if (STRPREFIX(migrateFrom, "unix")) {
             if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

src/uml/uml_conf.c

@@ -332,7 +332,8 @@ umlBuildCommandLineChr(virDomainChrDefPtr def,
                 VIR_FORCE_CLOSE(fd_out);
                 return NULL;
             }
-            virCommandTransferFD(cmd, fd_out);
+            virCommandPassFD(cmd, fd_out,
+                             VIR_COMMAND_PASS_FD_CLOSE_PARENT);
         }
         break;
    case VIR_DOMAIN_CHR_TYPE_PIPE:

src/util/vircommand.c

@@ -64,6 +64,14 @@ enum {
     VIR_EXEC_ASYNC_IO   = (1 << 4),
 };
 
+typedef struct _virCommandFD virCommandFD;
+typedef virCommandFD *virCommandFDPtr;
+
+struct _virCommandFD {
+    int fd;
+    unsigned int flags;
+};
+
 struct _virCommand {
     int has_error; /* ENOMEM on allocation failure, -1 for anything else.  */
 
@@ -77,10 +85,8 @@ struct _virCommand {
 
     char *pwd;
 
-    int *preserve; /* FDs to pass to child. */
+    size_t npassfd;
-    int preserve_size;
+    virCommandFDPtr passfd;
-    int *transfer; /* FDs to close in parent. */
-    int transfer_size;
 
     unsigned int flags;
 
@@ -135,14 +141,15 @@ struct _virCommand {
  * false otherwise.
  */
 static bool
-virCommandFDIsSet(int fd,
+virCommandFDIsSet(virCommandPtr cmd,
-                  const int *set,
+                  int fd)
-                  int set_size)
 {
     size_t i = 0;
+    if (!cmd)
+        return false;
 
-    while (i < set_size)
+    while (i < cmd->npassfd)
-        if (set[i++] == fd)
+        if (cmd->passfd[i++].fd == fd)
             return true;
 
     return false;
@@ -163,22 +170,21 @@ virCommandFDIsSet(int fd,
  *          ENOMEM on OOM
  */
 static int
-virCommandFDSet(int fd,
+virCommandFDSet(virCommandPtr cmd,
-                int **set,
+                int fd,
-                int *set_size)
+                unsigned int flags)
 {
-    if (fd < 0 || !set || !set_size)
+    if (!cmd || fd < 0)
         return -1;
 
-    if (virCommandFDIsSet(fd, *set, *set_size))
+    if (virCommandFDIsSet(cmd, fd))
         return 0;
 
-    if (VIR_REALLOC_N(*set, *set_size + 1) < 0) {
+    if (VIR_EXPAND_N(cmd->passfd, cmd->npassfd, 1) < 0)
         return ENOMEM;
-    }
 
-    (*set)[*set_size] = fd;
+    cmd->passfd[cmd->npassfd - 1].fd = fd;
-    (*set_size)++;
+    cmd->passfd[cmd->npassfd - 1].flags = flags;
 
     return 0;
 }
@@ -525,8 +531,7 @@ virExec(virCommandPtr cmd)
     for (fd = 3; fd < openmax; fd++) {
         if (fd == childin || fd == childout || fd == childerr)
             continue;
-        if (!cmd->preserve ||
+        if (!virCommandFDIsSet(cmd, fd)) {
-            !virCommandFDIsSet(fd, cmd->preserve, cmd->preserve_size)) {
             tmpfd = fd;
             VIR_MASS_CLOSE(tmpfd);
         } else if (virSetInherit(fd, true) < 0) {
@@ -882,68 +887,52 @@ virCommandNewVAList(const char *binary, va_list list)
 }
 
 
-/*
+#define VIR_COMMAND_MAYBE_CLOSE_FD(fd, flags)       \
- * Preserve the specified file descriptor in the child, instead of
+    if ((fd > STDERR_FILENO) &&                     \
- * closing it.  FD must not be one of the three standard streams.  If
+        (flags & VIR_COMMAND_PASS_FD_CLOSE_PARENT)) \
- * transfer is true, then fd will be closed in the parent after a call
+        VIR_FORCE_CLOSE(fd)
- * to Run/RunAsync/Free, otherwise caller is still responsible for fd.
+
- * Returns true if a transferring caller should close FD now, and
+/**
- * false if the transfer is successfully recorded.
+ * virCommandPassFD:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ * @flags: the flags
+ *
+ * Transfer the specified file descriptor to the child, instead
+ * of closing it on exec. @fd must not be one of the three
+ * standard streams.
+ *
+ * If the flag VIR_COMMAND_PASS_FD_CLOSE_PARENT is set then fd will
+ * be closed in the parent no later than Run/RunAsync/Free. The parent
+ * should cease using the @fd when this call completes
  */
-static bool
+void
-virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer)
+virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags)
 {
     int ret = 0;
 
-    if (!cmd)
+    if (!cmd) {
-        return fd > STDERR_FILENO;
+        VIR_COMMAND_MAYBE_CLOSE_FD(fd, flags);
-
+        return;
-    if (fd <= STDERR_FILENO ||
-        (ret = virCommandFDSet(fd, &cmd->preserve, &cmd->preserve_size)) ||
-        (transfer && (ret = virCommandFDSet(fd, &cmd->transfer,
-                                            &cmd->transfer_size)))) {
-        if (!cmd->has_error)
-            cmd->has_error = ret ? ret : -1;
-        VIR_DEBUG("cannot preserve %d", fd);
-        return fd > STDERR_FILENO;
     }
 
-    return false;
+    if (fd <= STDERR_FILENO) {
-}
+        VIR_DEBUG("invalid fd %d", fd);
+        VIR_COMMAND_MAYBE_CLOSE_FD(fd, flags);
+        if (!cmd->has_error)
+            cmd->has_error = -1;
+        return;
+    }
 
-/**
+    if ((ret = virCommandFDSet(cmd, fd, flags)) != 0) {
- * virCommandPreserveFD:
+        if (!cmd->has_error)
- * @cmd: the command to modify
+            cmd->has_error = ret;
- * @fd: fd to mark for inheritance into child
+        VIR_DEBUG("cannot preserve %d", fd);
- *
+        VIR_COMMAND_MAYBE_CLOSE_FD(fd, flags);
- * Preserve the specified file descriptor
+        return;
- * in the child, instead of closing it on exec.
+    }
- * The parent is still responsible for managing fd.
- */
-void
-virCommandPreserveFD(virCommandPtr cmd, int fd)
-{
-    virCommandKeepFD(cmd, fd, false);
 }
 
-/**
- * virCommandTransferFD:
- * @cmd: the command to modify
- * @fd: fd to reassign to the child
- *
- * Transfer the specified file descriptor
- * to the child, instead of closing it on exec.
- * The parent should no longer use fd, and the parent's copy will
- * be automatically closed no later than during Run/RunAsync/Free.
- */
-void
-virCommandTransferFD(virCommandPtr cmd, int fd)
-{
-    if (virCommandKeepFD(cmd, fd, true))
-        VIR_FORCE_CLOSE(fd);
-}
-
-
 /**
  * virCommandSetPidFile:
  * @cmd: the command to modify
@@ -2252,11 +2241,12 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
     VIR_DEBUG("Command result %d, with PID %d",
               ret, (int)cmd->pid);
 
-    for (i = 0; i < cmd->transfer_size; i++) {
+    for (i = 0; i < cmd->npassfd; i++) {
-        VIR_FORCE_CLOSE(cmd->transfer[i]);
+        if (cmd->passfd[i].flags & VIR_COMMAND_PASS_FD_CLOSE_PARENT)
+            VIR_FORCE_CLOSE(cmd->passfd[i].fd);
     }
-    cmd->transfer_size = 0;
+    cmd->npassfd = 0;
-    VIR_FREE(cmd->transfer);
+    VIR_FREE(cmd->passfd);
 
     if (ret == 0 && pid)
         *pid = cmd->pid;
@@ -2431,8 +2421,10 @@ void virCommandRequireHandshake(virCommandPtr cmd)
               "keep handshake wait=%d notify=%d",
               cmd->handshakeWait[1], cmd->handshakeNotify[0],
               cmd->handshakeWait[0], cmd->handshakeNotify[1]);
-    virCommandTransferFD(cmd, cmd->handshakeWait[1]);
+    virCommandPassFD(cmd, cmd->handshakeWait[1],
-    virCommandTransferFD(cmd, cmd->handshakeNotify[0]);
+                     VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+    virCommandPassFD(cmd, cmd->handshakeNotify[0],
+                     VIR_COMMAND_PASS_FD_CLOSE_PARENT);
     cmd->handshake = true;
 }
 
@@ -2555,9 +2547,12 @@ virCommandFree(virCommandPtr cmd)
     if (!cmd)
         return;
 
-    for (i = 0; i < cmd->transfer_size; i++) {
+    for (i = 0; i < cmd->npassfd; i++) {
-        VIR_FORCE_CLOSE(cmd->transfer[i]);
+        if (cmd->passfd[i].flags & VIR_COMMAND_PASS_FD_CLOSE_PARENT)
+            VIR_FORCE_CLOSE(cmd->passfd[i].fd);
     }
+    cmd->npassfd = 0;
+    VIR_FREE(cmd->passfd);
 
     if (cmd->asyncioThread) {
         virThreadJoin(cmd->asyncioThread);
@@ -2579,7 +2574,7 @@ virCommandFree(virCommandPtr cmd)
 
     if (cmd->handshake) {
         /* The other 2 fds in these arrays are closed
-         * due to use with virCommandTransferFD
+         * due to use with virCommandPassFD
          */
         VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
         VIR_FORCE_CLOSE(cmd->handshakeNotify[1]);
@@ -2590,8 +2585,6 @@ virCommandFree(virCommandPtr cmd)
     if (cmd->reap)
         virCommandAbort(cmd);
 
-    VIR_FREE(cmd->transfer);
-    VIR_FREE(cmd->preserve);
 #if defined(WITH_SECDRIVER_SELINUX)
     VIR_FREE(cmd->seLinuxLabel);
 #endif

src/util/vircommand.h

@@ -51,11 +51,14 @@ virCommandPtr virCommandNewVAList(const char *binary, va_list list)
  * delayed until the Run/RunAsync methods
  */
 
-void virCommandPreserveFD(virCommandPtr cmd,
+enum {
-                          int fd);
+    /* Close the FD in the parent */
+    VIR_COMMAND_PASS_FD_CLOSE_PARENT = (1 << 0),
+};
 
-void virCommandTransferFD(virCommandPtr cmd,
+void virCommandPassFD(virCommandPtr cmd,
-                          int fd);
+                      int fd,
+                      unsigned int flags);
 
 void virCommandSetPidFile(virCommandPtr cmd,
                           const char *pidfile) ATTRIBUTE_NONNULL(2);

tests/commandtest.c

@@ -194,8 +194,9 @@ static int test3(const void *unused ATTRIBUTE_UNUSED)
     int newfd3 = dup(STDERR_FILENO);
     int ret = -1;
 
-    virCommandPreserveFD(cmd, newfd1);
+    virCommandPassFD(cmd, newfd1, 0);
-    virCommandTransferFD(cmd, newfd3);
+    virCommandPassFD(cmd, newfd3,
+                     VIR_COMMAND_PASS_FD_CLOSE_PARENT);
 
     if (virCommandRun(cmd, NULL) < 0) {
         virErrorPtr err = virGetLastError();