Re: [libvirt] [PATCH 08/10] Introduce new APIs for spawning processes

On 11/19/2010 05:16 PM, Eric Blake wrote: On thinking about this more, I'm thinking that the user should be able to request whether the fd remains open after the command has been executed (for example, the client may want to share an fd among multiple child processes, in which case each virCommandRun must not close the fd); doable by adding another argument to virCommandPreserveFD. Okay, I see how you added that in your variant in today's locking series, along with virCommandToString; now folded into my v2. Here's the interdiff of what will be in my v2: diff --git c/src/util/command.c w/src/util/command.c index 71db2a1..4e1071d 100644 --- c/src/util/command.c +++ w/src/util/command.c @@ -33,6 +33,7 @@ #include "util.h" #include "logging.h" #include "files.h" +#include "buf.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -54,7 +55,9 @@ struct _virCommand { char *pwd; /* XXX Use int[] if we ever need to support more than FD_SETSIZE fd's. */ - fd_set preserve; + fd_set preserve; /* FDs to pass to child. */ + fd_set transfer; /* FDs to close in parent. */ + unsigned int flags; char *inbuf; @@ -99,6 +102,7 @@ virCommandNewArgs(const char *const*args) return NULL; FD_ZERO(&cmd->preserve); + FD_ZERO(&cmd->transfer); cmd->infd = cmd->outfd = cmd->errfd = -1; cmd->inpipe = -1; cmd->pid = -1; @@ -117,20 +121,25 @@ virCommandNewArgs(const char *const*args) /* * Preserve the specified file descriptor in the child, instead of * 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, otherwise caller is still responsible for fd. */ void -virCommandPreserveFD(virCommandPtr cmd, int fd) +virCommandPreserveFD(virCommandPtr cmd, int fd, bool transfer) { - if (!cmd || cmd->has_error) + if (!cmd) return; if (fd <= STDERR_FILENO || FD_SETSIZE <= fd) { - cmd->has_error = -1; + if (!cmd->has_error) + cmd->has_error = -1; VIR_DEBUG("cannot preserve %d", fd); return; } FD_SET(fd, &cmd->preserve); + if (transfer) + FD_SET(fd, &cmd->transfer); } /* @@ -569,6 +578,89 @@ virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque) /* + * Call after adding all arguments and environment settings, but before + * Run/RunAsync, to immediately output the environment and arguments of + * cmd to logfd. If virCommandRun cannot succeed (because of an + * out-of-memory condition while building cmd), nothing will be logged. + */ +void +virCommandWriteArgLog(virCommandPtr cmd, int logfd) +{ + int ioError = 0; + size_t i; + + /* Any errors will be reported later by virCommandRun, which means + * no command will be run, so there is nothing to log. */ + if (!cmd || cmd->has_error) + return; + + for (i = 0 ; i < cmd->nenv ; i++) { + if (safewrite(logfd, cmd->env[i], strlen(cmd->env[i])) < 0) + ioError = errno; + if (safewrite(logfd, " ", 1) < 0) + ioError = errno; + } + for (i = 0 ; i < cmd->nargs ; i++) { + if (safewrite(logfd, cmd->args[i], strlen(cmd->args[i])) < 0) + ioError = errno; + if (safewrite(logfd, i == cmd->nargs - 1 ? "\n" : " ", 1) < 0) + ioError = errno; + } + + if (ioError) { + char ebuf[1024]; + VIR_WARN("Unable to write command %s args to logfile: %s", + cmd->args[0], virStrerror(ioError, ebuf, sizeof ebuf)); + } +} + + +/* + * Call after adding all arguments and environment settings, but before + * Run/RunAsync, to return a string representation of the environment and + * arguments of cmd. If virCommandRun cannot succeed (because of an + * out-of-memory condition while building cmd), NULL will be returned. + * Caller is responsible for freeing the resulting string. + */ +char * +virCommandToString(virCommandPtr cmd) +{ + size_t i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + /* Cannot assume virCommandRun will be called; so report the error + * now. If virCommandRun is called, it will report the same error. */ + if (!cmd || cmd->has_error == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return NULL; + } + if (cmd->has_error == ENOMEM) { + virReportOOMError(); + return NULL; + } + + for (i = 0; i < cmd->nenv; i++) { + virBufferAdd(&buf, cmd->env[i], strlen(cmd->env[i])); + virBufferAddChar(&buf, ' '); + } + virBufferAdd(&buf, cmd->args[0], strlen(cmd->args[0])); + for (i = 1; i < cmd->nargs; i++) { + virBufferAddChar(&buf, ' '); + virBufferAdd(&buf, cmd->args[i], strlen(cmd->args[i])); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + + +/* * Manage input and output to the child process. */ static int @@ -823,6 +915,7 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) { int ret; char *str; + int i; if (!cmd || cmd->has_error == -1) { virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -868,6 +961,13 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) VIR_DEBUG("Command result %d, with PID %d", ret, (int)cmd->pid); + for (i = STDERR_FILENO + 1; i < FD_SETSIZE; i++) { + if (FD_ISSET(i, &cmd->transfer)) { + int tmpfd = i; + VIR_FORCE_CLOSE(tmpfd); + } + } + if (ret == 0 && pid) *pid = cmd->pid; diff --git c/src/util/command.h w/src/util/command.h index ca24869..a419140 100644 --- c/src/util/command.h +++ w/src/util/command.h @@ -22,6 +22,8 @@ #ifndef __VIR_COMMAND_H__ # define __VIR_COMMAND_H__ +# include <stdbool.h> + # include "internal.h" # include "util.h" @@ -40,15 +42,18 @@ virCommandPtr virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1); virCommandPtr virCommandNewArgs(const char *const*args) ATTRIBUTE_NONNULL(1); /* All error report from these setup APIs is - * delayed until the Run/Exec/Wait methods + * delayed until the Run/RunAsync methods */ /* * Preserve the specified file descriptor - * in the child, instead of closing it + * in the child, instead of closing it. + * If transfer is true, then fd will be closed in the parent after + * a call to Run/RunAsync, otherwise caller is still responsible for fd. */ void virCommandPreserveFD(virCommandPtr cmd, - int fd); + int fd, + bool transfer); /* * Save the child PID in a pidfile @@ -179,6 +184,24 @@ void virCommandSetPreExecHook(virCommandPtr cmd, void *opaque) ATTRIBUTE_NONNULL(2); /* + * Call after adding all arguments and environment settings, but before + * Run/RunAsync, to immediately output the environment and arguments of + * cmd to logfd. If virCommandRun cannot succeed (because of an + * out-of-memory condition while building cmd), nothing will be logged. + */ +void virCommandWriteArgLog(virCommandPtr cmd, + int logfd); + +/* + * Call after adding all arguments and environment settings, but before + * Run/RunAsync, to return a string representation of the environment and + * arguments of cmd. If virCommandRun cannot succeed (because of an + * out-of-memory condition while building cmd), NULL will be returned. + * Caller is responsible for freeing the resulting string. + */ +char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; + +/* * Run the command and wait for completion. * Returns -1 on any error executing the * command. Returns 0 if the command executed, diff --git c/tests/commanddata/test16.log w/tests/commanddata/test16.log new file mode 100644 index 0000000..2433a4d --- /dev/null +++ w/tests/commanddata/test16.log @@ -0,0 +1 @@ +A=B /bin/true C diff --git c/tests/commandhelper.c w/tests/commandhelper.c index dc8998f..2ee9153 100644 --- c/tests/commandhelper.c +++ w/tests/commandhelper.c @@ -29,6 +29,7 @@ #include "internal.h" #include "util.h" #include "memory.h" +#include "files.h" static int envsort(const void *a, const void *b) { @@ -102,7 +103,7 @@ int main(int argc, char **argv) { strcpy(cwd, ".../commanddata"); fprintf(log, "CWD:%s\n", cwd); - fclose(log); + VIR_FORCE_FCLOSE(log); char buf[1024]; ssize_t got; diff --git c/tests/commandtest.c w/tests/commandtest.c index 5479bfc..de1597d 100644 --- c/tests/commandtest.c +++ w/tests/commandtest.c @@ -25,6 +25,8 @@ #include <string.h> #include <unistd.h> #include <signal.h> +#include <sys/stat.h> +#include <fcntl.h> #include "testutils.h" #include "internal.h" @@ -32,6 +34,7 @@ #include "util.h" #include "memory.h" #include "command.h" +#include "files.h" #ifndef __linux__ @@ -166,10 +169,9 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) { int newfd1 = dup(STDERR_FILENO); int newfd2 = dup(STDERR_FILENO); int newfd3 = dup(STDERR_FILENO); - close(newfd2); - virCommandPreserveFD(cmd, newfd1); - virCommandPreserveFD(cmd, newfd3); + virCommandPreserveFD(cmd, newfd1, false); + virCommandPreserveFD(cmd, newfd3, true); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); @@ -177,7 +179,17 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) { return -1; } + if (fcntl(newfd1, F_GETFL) < 0 || + fcntl(newfd2, F_GETFL) < 0 || + fcntl(newfd3, F_GETFL) >= 0) { + puts("fds in wrong state"); + return -1; + } + virCommandFree(cmd); + VIR_FORCE_CLOSE(newfd1); + VIR_FORCE_CLOSE(newfd2); return checkoutput("test3"); } @@ -501,6 +513,52 @@ static int test15(const void *unused ATTRIBUTE_UNUSED) { return checkoutput("test15"); } +/* + * Don't run program; rather, log what would be run. + */ +static int test16(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew("/bin/true"); + char *outactual = NULL; + const char *outexpect = "A=B /bin/true C"; + int ret = -1; + int fd = -1; + + virCommandAddEnvPair(cmd, "A", "B"); + virCommandAddArg(cmd, "C"); + + if ((outactual = virCommandToString(cmd)) == NULL) { + virErrorPtr err = virGetLastError(); + printf("Cannot convert to string: %s\n", err->message); + return -1; + } + if ((fd = open(abs_builddir "/commandhelper.log", + O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) { + printf("Cannot open log file: %s\n", strerror (errno)); + goto cleanup; + } + virCommandWriteArgLog(cmd, fd); + if (VIR_CLOSE(fd) < 0) { + printf("Cannot close log file: %s\n", strerror (errno)); + goto cleanup; + } + + virCommandFree(cmd); + + if (checkoutput("test16") < 0) + goto cleanup; + + if (!STREQ(outactual, outexpect)) { + virtTestDifference(stderr, outactual, outexpect); + goto cleanup; + } + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + VIR_FREE(outactual); + return ret; +} + static int mymain(int argc, char **argv) { @@ -563,6 +621,7 @@ mymain(int argc, char **argv) DO_TEST(test13); DO_TEST(test14); DO_TEST(test15); + DO_TEST(test16); return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); } -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org