12:04 p.m.
virCommandTransferFD promises that the fd is no longer owned by
the caller. Normally, we want the fd to remain open until the
child runs, but in error situations, we must close it earlier.
To avoid any risks of double-close due to misunderstanding about
this interface, change it to take a pointer which gets set to
-1 in the caller to record that the transfer was successful.
* src/util/command.h (virCommandTransferFD): Alter signature.
* src/util/command.c (virCommandTransferFD): Close fd now if we
can't track it to close later.
(virCommandKeepFD): Adjust helper to make this easier.
(virCommandRequireHandshake): Adjust callers.
* src/qemu/qemu_command.c (qemuBuildCommandLine): Likewise.
* src/uml/uml_conf.c (umlBuildCommandLineChr): Likewise.
* tests/commandtest.c (test3): Likewise.
* docs/internals/command.html.in: Update the docs.
---
v2: alter the signature, and adjust all callers, to make it foolproof
at avoiding a double-close
docs/internals/command.html.in | 2 +-
src/qemu/qemu_command.c | 8 ++++----
src/uml/uml_conf.c | 2 +-
src/util/command.c | 28 +++++++++++++++++-----------
src/util/command.h | 9 +++++----
tests/commandtest.c | 6 +++++-
6 files changed, 33 insertions(+), 22 deletions(-)
diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
index 8a194ec..21536c4 100644
--- a/docs/internals/command.html.in
+++ b/docs/internals/command.html.in
@@ -257,7 +257,7 @@
int sharedfd = open("cmd.log", "w+");
int childfd = open("conf.txt", "r");
virCommandPreserveFD(cmd, sharedfd);
- virCommandTransferFD(cmd, childfd);
+ virCommandTransferFD(cmd, &childfd);
if (VIR_CLOSE(sharedfd) < 0)
goto cleanup;
</pre>
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a3bce4a..b413339 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3698,7 +3698,7 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
last_good_net = i;
- virCommandTransferFD(cmd, tapfd);
+ virCommandTransferFD(cmd, &tapfd);
if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
tapfd) >= sizeof(tapfd_name))
@@ -3710,7 +3710,7 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
last_good_net = i;
- virCommandTransferFD(cmd, tapfd);
+ virCommandTransferFD(cmd, &tapfd);
if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
tapfd) >= sizeof(tapfd_name))
@@ -3727,7 +3727,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0)
goto error;
if (vhostfd >= 0) {
- virCommandTransferFD(cmd, vhostfd);
+ virCommandTransferFD(cmd, &vhostfd);
if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d",
vhostfd) >= sizeof(vhostfd_name))
@@ -4535,7 +4535,7 @@ qemuBuildCommandLine(virConnectPtr conn,
goto no_memory;
}
- virCommandTransferFD(cmd, configfd);
+ virCommandTransferFD(cmd, &configfd);
}
}
virCommandAddArg(cmd, "-device");
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index 0122472..4c2c2f1 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -376,7 +376,7 @@ umlBuildCommandLineChr(virDomainChrDefPtr def,
VIR_FORCE_CLOSE(fd_out);
return NULL;
}
- virCommandTransferFD(cmd, fd_out);
+ virCommandTransferFD(cmd, &fd_out);
}
break;
case VIR_DOMAIN_CHR_TYPE_PIPE:
diff --git a/src/util/command.c b/src/util/command.c
index 3c516ec..f583b37 100644
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -711,45 +711,51 @@ virCommandNewArgList(const char *binary, ...)
* closing it. FD must not be one of the three standard streams. If
* transfer is true, then fd will be closed in the parent after a call
* 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.
*/
-static void
+static bool
virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer)
{
if (!cmd)
- return;
+ return fd > STDERR_FILENO;
if (fd <= STDERR_FILENO || FD_SETSIZE <= fd) {
if (!cmd->has_error)
cmd->has_error = -1;
VIR_DEBUG("cannot preserve %d", fd);
- return;
+ return fd > STDERR_FILENO;
}
FD_SET(fd, &cmd->preserve);
if (transfer)
FD_SET(fd, &cmd->transfer);
+ return false;
}
/*
* Preserve the specified file descriptor
- * in the child, instead of closing it.
+ * in the child, instead of closing it on exec.
* The parent is still responsible for managing fd.
*/
void
virCommandPreserveFD(virCommandPtr cmd, int fd)
{
- return virCommandKeepFD(cmd, fd, false);
+ virCommandKeepFD(cmd, fd, false);
}
/*
* Transfer the specified file descriptor
- * to the child, instead of closing it.
- * Close the fd in the parent during Run/RunAsync/Free.
+ * 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)
+virCommandTransferFD(virCommandPtr cmd, int *fd)
{
- return virCommandKeepFD(cmd, fd, true);
+ if (virCommandKeepFD(cmd, *fd, true))
+ VIR_FORCE_CLOSE(*fd);
+ *fd = -1;
}
@@ -2109,8 +2115,8 @@ void virCommandRequireHandshake(virCommandPtr cmd)
VIR_DEBUG("Transfer handshake wait=%d notify=%d",
cmd->handshakeWait[1], cmd->handshakeNotify[0]);
- virCommandTransferFD(cmd, cmd->handshakeWait[1]);
- virCommandTransferFD(cmd, cmd->handshakeNotify[0]);
+ virCommandTransferFD(cmd, &cmd->handshakeWait[1]);
+ virCommandTransferFD(cmd, &cmd->handshakeNotify[0]);
cmd->handshake = true;
}
diff --git a/src/util/command.h b/src/util/command.h
index c8a04f1..9e14215 100644
--- a/src/util/command.h
+++ b/src/util/command.h
@@ -67,7 +67,7 @@ virCommandPtr virCommandNewArgList(const char *binary, ...)
/*
* Preserve the specified file descriptor
- * in the child, instead of closing it.
+ * in the child, instead of closing it on exec.
* The parent is still responsible for managing fd.
*/
void virCommandPreserveFD(virCommandPtr cmd,
@@ -75,11 +75,12 @@ void virCommandPreserveFD(virCommandPtr cmd,
/*
* Transfer the specified file descriptor
- * to the child, instead of closing it.
- * Close the fd in the parent during Run/RunAsync/Free.
+ * 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);
+ int *fd);
/*
* Save the child PID in a pidfile
diff --git a/tests/commandtest.c b/tests/commandtest.c
index 6757253..ee8e7c3 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -181,7 +181,11 @@ static int test3(const void *unused ATTRIBUTE_UNUSED)
int ret = -1;
virCommandPreserveFD(cmd, newfd1);
- virCommandTransferFD(cmd, newfd3);
+ virCommandTransferFD(cmd, &newfd3);
+ if (newfd3 != -1) {
+ puts("transfer should clear parent's fd");
+ goto cleanup;
+ }
if (virCommandRun(cmd, NULL) < 0) {
virErrorPtr err = virGetLastError();
--
1.7.4.4