[libvirt] PATCH: 3/5: Allow FD for stdout/err to be passed in

[Jim Meyering]

"Daniel P. Berrange" <berrange at 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;
> +            }