"Daniel P. Berrange" <berrange@redhat.com> wrote:
The contract for virExec() currently allows the caller to pass in a NULL for stdout/err parameters in which case the child will be connected to /dev/null, or they can pass in a pointer to an int, in which case the child will be connected to a pair of pipes, and the read end of the pipe is returned to the caller.
The LXC driver will require a 3rd option - we want to pass in a existing file handler connected to a logfile. So this patch extends the semantics of these two parameters. If the stderr/out params are non-NULL, but are initialized to -1, then a pipe will be allocated. If they are >= 0, then they are assumed to be existing FDs to dup onto the child's stdout/err.
This change neccessitated updating all existing callers of virExec to make sure they initialize the parameters to -1 to maintain existing behaviour.
ACK No technical problems... so here are some stylistic suggestions
diff -r 100b059a8488 src/openvz_driver.c --- a/src/openvz_driver.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/openvz_driver.c Tue Aug 12 22:12:42 2008 +0100 @@ -736,7 +736,7 @@
static int openvzListDomains(virConnectPtr conn, int *ids, int nids) { int got = 0; - int veid, pid, outfd, errfd; + int veid, pid, outfd = -1, errfd = -1;
I find this formatting a little easier to read/maintain: [e.g., with each declaration on a separate line, independent changes to veid and errfd might not conflict, but they surely would when they're all on the same line. ] int veid; int pid; int outfd = -1; int errfd = -1;
int ret; char buf[32]; char *endptr; @@ -772,7 +772,7 @@ static int openvzListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { int got = 0; - int veid, pid, outfd, errfd, ret; + int veid, pid, outfd = -1, errfd = -1, ret; char vpsname[OPENVZ_NAME_MAX]; char buf[32]; char *endptr; diff -r 100b059a8488 src/qemu_driver.c --- a/src/qemu_driver.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/qemu_driver.c Tue Aug 12 22:12:42 2008 +0100 @@ -948,6 +948,8 @@ if (safewrite(vm->logfile, "\n", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"), errno, strerror(errno)); + + vm->stdout_fd = vm->stderr_fd = -1;
Same here, putting the common bits one above the other makes it easier to see the parts that vary: vm->stdout_fd = -1; vm->stderr_fd = -1;
ret = virExecNonBlock(conn, argv, &vm->pid, vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd); diff -r 100b059a8488 src/util.c --- a/src/util.c Tue Aug 12 22:12:38 2008 +0100 +++ b/src/util.c Tue Aug 12 22:12:42 2008 +0100 @@ -110,6 +110,7 @@ int pid, null, i; int pipeout[2] = {-1,-1}; int pipeerr[2] = {-1,-1}; + int childout = -1, childerr = -1;
And here: int childout = -1; int childerr = -1;
sigset_t oldmask, newmask; struct sigaction sig_action;
@@ -132,39 +133,66 @@ goto cleanup; }
- if ((outfd != NULL && pipe(pipeout) < 0) || - (errfd != NULL && pipe(pipeerr) < 0)) { - ReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("cannot create pipe: %s"), strerror(errno)); - goto cleanup; + if (outfd != NULL) { + if (*outfd == -1) { + if (pipe(pipeout) < 0) { + ReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot create pipe: %s"), strerror(errno));
Maybe s/cannot/Failed to/? The latter is slightly stronger/clearer, and consistent with other messages.
+ goto cleanup; + }