4:33 p.m.
On 12/01/2010 02:42 PM, Matthias Bolte wrote:
> +++ b/cfg.mk
> @@ -77,6 +77,7 @@ useless_free_options = \
> --name=virCapabilitiesFreeHostNUMACell \
> --name=virCapabilitiesFreeMachines \
> --name=virCgroupFree \
> + --name=virCommandFree \
> --name=virConfFreeList \
> --name=virConfFreeValue \
> --name=virDomainChrDefFree \
You should also add virCommandError to the msg_gen_function list.
Good catch, and done.
> +void
> +virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (cmd->outfd != -1) {
To detect double assignment properly you need to check for this I think:
if (cmd->outfd != -1 || cmd->outfdptr || cmd->outbuf) {
Almost. There are really only two functions that affect stdout settings
before a command (set fd assigns outfdptr, set buffer assigns outbuf and
outfdptr), so checking cmd->outfdptr for NULL is sufficient to check for
no duplicate call.
> +void
> +virCommandSetInputFD(virCommandPtr cmd, int infd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (infd < 0 || cmd->inbuf) {
I think you meant to check here for this instead:
if (cmd->infd != -1 || cmd->inbuf) {
Hmm; actually, there's two issues. One of validating that input is set
at most once (cmd->infd != -1 || cmd->inbuf), and one of validating that
the incoming argument is valid (infd < 0), worth two separate messages.
Thanks for forcing me to think about this more.
> +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;
> + }
As written this will only save the last occurred error in ioError. Is
this intended? Looks like a best effort approach, try to write as much
as possible ignoring errors.
Well, the function has no return value, so yes, that's the best we can
do - log as much as possible, and issue a VIR_WARN if we didn't log
everything. But I couldn't seem to justify returning an error and
stopping the log at the first error - how do you log that you had an
error logging, when logging is orthogonal to running the command in the
first place?
In virCommandProcessIO you say that cmd->{out|err}fd is a valid FD
(created in virExecWithHook called by virCommandRunAsync) when
cmd->{out|err}buf is not NULL. As far as I can tell from the code that
is true when the caller had requested to capture stdout and stderr.
But in case that caller didn't do this then you setup buffers to
capture stdout and stderr for logging. In that case
virCommandProcessIO will be called with cmd->{out|err}buf being
non-NULL but cmd->{out|err}fd being invalid as you explicitly set them
to -1 in the two if blocks before the call to virCommandProcessIO.
Yep, that needed reordering. If virCommandRun detects that nothing set
output, then outfd needs to be set prior to virCommandRunAsync so as to
force the creation of a pipe rather than assigning fds to /dev/null.
> +
> +#ifndef __linux__
What's Linux specific in this test? Shouldn't the command API and this
test be working on all systems that support fork/exec?
It should really be #if !HAVE_FORK (aka #ifdef WIN32).
I'm testing the impacts of squashing this in...
diff --git i/cfg.mk w/cfg.mk
index 8a8da18..29de9d9 100644
--- i/cfg.mk
+++ w/cfg.mk
@@ -369,9 +369,9 @@ msg_gen_function += umlReportError
msg_gen_function += vah_error
msg_gen_function += vah_warning
msg_gen_function += vboxError
+msg_gen_function += virCommandError
msg_gen_function += virConfError
msg_gen_function += virDomainReportError
-msg_gen_function += virSecurityReportError
msg_gen_function += virHashError
msg_gen_function += virLibConnError
msg_gen_function += virLibDomainError
@@ -380,6 +380,7 @@ msg_gen_function += virNodeDeviceReportError
msg_gen_function += virRaiseError
msg_gen_function += virReportErrorHelper
msg_gen_function += virReportSystemError
+msg_gen_function += virSecurityReportError
msg_gen_function += virSexprError
msg_gen_function += virStorageReportError
msg_gen_function += virXMLError
diff --git i/src/util/command.c w/src/util/command.c
index 948a957..aa43f76 100644
--- i/src/util/command.c
+++ w/src/util/command.c
@@ -91,7 +91,7 @@ virCommandNew(const char *binary)
/*
* Create a new command with a NULL terminated
- * set of args, taking binary from argv[0]
+ * set of args, taking binary from args[0]
*/
virCommandPtr
virCommandNewArgs(const char *const*args)
@@ -543,7 +543,7 @@ virCommandSetOutputBuffer(virCommandPtr cmd, char
**outbuf)
if (!cmd || cmd->has_error)
return;
- if (cmd->outfd != -1) {
+ if (cmd->outfdptr) {
cmd->has_error = -1;
VIR_DEBUG0("cannot specify output twice");
return;
@@ -563,7 +563,7 @@ virCommandSetErrorBuffer(virCommandPtr cmd, char
**errbuf)
if (!cmd || cmd->has_error)
return;
- if (cmd->errfd != -1) {
+ if (cmd->errfdptr) {
cmd->has_error = -1;
VIR_DEBUG0("cannot specify stderr twice");
return;
@@ -583,11 +583,16 @@ virCommandSetInputFD(virCommandPtr cmd, int infd)
if (!cmd || cmd->has_error)
return;
- if (infd < 0 || cmd->inbuf) {
+ if (cmd->infd != -1 || cmd->inbuf) {
cmd->has_error = -1;
VIR_DEBUG0("cannot specify input twice");
return;
}
+ if (infd < 0) {
+ cmd->has_error = -1;
+ VIR_DEBUG0("cannot specify invalid input fd");
+ return;
+ }
cmd->infd = infd;
}
@@ -602,7 +607,7 @@ virCommandSetOutputFD(virCommandPtr cmd, int *outfd)
if (!cmd || cmd->has_error)
return;
- if (!outfd || cmd->outfd != -1) {
+ if (cmd->outfdptr) {
cmd->has_error = -1;
VIR_DEBUG0("cannot specify output twice");
return;
@@ -621,7 +626,7 @@ virCommandSetErrorFD(virCommandPtr cmd, int *errfd)
if (!cmd || cmd->has_error)
return;
- if (!errfd || cmd->errfd != -1) {
+ if (cmd->errfdptr) {
cmd->has_error = -1;
VIR_DEBUG0("cannot specify stderr twice");
return;
@@ -642,6 +647,11 @@ virCommandSetPreExecHook(virCommandPtr cmd,
virExecHook hook, void *opaque)
if (!cmd || cmd->has_error)
return;
+ if (cmd->hook) {
+ cmd->has_error = -1;
+ VIR_DEBUG0("cannot specify hook twice");
+ return;
+ }
cmd->hook = hook;
cmd->opaque = opaque;
}
@@ -778,7 +788,7 @@ virCommandProcessIO(virCommandPtr cmd)
if (nfds == 0)
break;
- if (poll(fds,nfds, -1) < 0) {
+ if (poll(fds, nfds, -1) < 0) {
if ((errno == EAGAIN) || (errno == EINTR))
continue;
virReportSystemError(errno, "%s",
@@ -882,12 +892,25 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
if (pipe(infd) < 0) {
virReportSystemError(errno, "%s",
_("unable to open pipe"));
+ cmd->has_error = -1;
return -1;
}
cmd->infd = infd[0];
cmd->inpipe = infd[1];
}
+ /* If caller hasn't requested capture of stdout/err, then capture
+ * it ourselves so we can log it.
+ */
+ if (!cmd->outfdptr) {
+ cmd->outfdptr = &cmd->outfd;
+ cmd->outbuf = &outbuf;
+ }
+ if (!cmd->errfdptr) {
+ cmd->errfdptr = &cmd->errfd;
+ cmd->errbuf = &errbuf;
+ }
+
if (virCommandRunAsync(cmd, NULL) < 0) {
if (cmd->inbuf) {
int tmpfd = infd[0];
@@ -897,26 +920,10 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
if (VIR_CLOSE(infd[1]) < 0)
VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
}
+ cmd->has_error = -1;
return -1;
}
- /* If caller hasn't requested capture of
- * stdout/err, then capture it ourselves
- * so we can log it
- */
- if (!cmd->outbuf &&
- !cmd->outfdptr) {
- cmd->outfd = -1;
- cmd->outfdptr = &cmd->outfd;
- cmd->outbuf = &outbuf;
- }
- if (!cmd->errbuf &&
- !cmd->errfdptr) {
- cmd->errfd = -1;
- cmd->errfdptr = &cmd->errfd;
- cmd->errbuf = &errbuf;
- }
-
if (cmd->inbuf || cmd->outbuf || cmd->errbuf)
ret = virCommandProcessIO(cmd);
@@ -1012,7 +1019,7 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
}
- str = virArgvToString((const char *const *)cmd->args);
+ str = virCommandToString(cmd);
VIR_DEBUG("About to run %s", str ? str : cmd->args[0]);
VIR_FREE(str);
@@ -1090,7 +1097,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
if (exitstatus == NULL) {
if (status != 0) {
virCommandError(VIR_ERR_INTERNAL_ERROR,
- _("Intermediate daemon process exited with
status %d."),
+ _("Child process exited with status %d."),
WEXITSTATUS(status));
return -1;
}
diff --git i/tests/commandtest.c w/tests/commandtest.c
index 46d6ae5..0be8e94 100644
--- i/tests/commandtest.c
+++ w/tests/commandtest.c
@@ -36,7 +36,7 @@
#include "command.h"
#include "files.h"
-#ifndef __linux__
+#ifdef WIN32
static int
mymain(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED)
@@ -625,6 +625,6 @@ mymain(int argc, char **argv)
return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
}
-#endif /* __linux__ */
+#endif /* !WIN32 */
VIRT_TEST_MAIN(mymain)
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org