On 11/19/2010 05:16 PM, Eric Blake wrote:
> On 11/18/2010 02:55 AM, Daniel P. Berrange wrote:
>> On Wed, Nov 17, 2010 at 09:29:00PM -0700, Eric Blake wrote:
>>> From: Daniel P. Berrange <berrange@xxxxxxxxxx>
>>>
>>> This introduces a new set of APIs in src/util/command.h
>>> to use for invoking commands. This is intended to replace
>>> all current usage of virRun and virExec variants, with a
>>> more flexible and less error prone API.
>>>
>>
>>
>> My code forgot to ever close() the fds in cmd->preserve. We definitely
>> need todo it in virCommandFree(), but there's a small argument to say
>> we should also do it in virCommandRun/virCommandRunAsync so that if
>> the caller keeps the virCommandPtr alive for a long time, we don't
>> have the open FDs.
>
> I'll look into this more.
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.
>
>>
>> It would also be useful to have a generic API for logging info about
>> the command to an FD (to let us remove that logging code from UML
>> and QEMU & LXC drivers).
>>
>> eg
>>
>> +void virCommandWriteArgLog(virCommandPtr cmd, int logfd)
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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list