[libvirt] [PATCHv2 1/2] build: avoid close, system

[Matthias Bolte]

2011/1/29 Eric Blake <eblake at redhat.com>:
> * src/fdstream.c (virFDStreamOpenFile, virFDStreamCreateFile):
> Use VIR_FORCE_CLOSE instead of close.
> * tests/commandtest.c (mymain): Likewise.
> * tools/virsh.c (editFile): Use virCommand instead of system.
> * src/util/util.c (__virExec): Special case preservation of std
> file descriptors to child.
> ---
>
> v2: make virCommand behave more like system.  So I didn't do
> any signal handling like system, but at least now you can
> actually edit interactively.  virCommandRun doesn't like
> interactive sessions, so I have to use virCommandRunAsync
> followed by virCommandWait.  I also had to fix virExec.
>
>  src/fdstream.c      |    6 ++--
>  src/util/util.c     |   12 +++++----
>  tests/commandtest.c |   12 ++++++---
>  tools/virsh.c       |   65 ++++++++++++++++++++++++++------------------------
>  4 files changed, 52 insertions(+), 43 deletions(-)

> diff --git a/src/util/util.c b/src/util/util.c
> index f412a83..af14b2e 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -593,14 +593,16 @@ __virExec(const char *const*argv,
>         goto fork_error;
>     }
>
> -    VIR_FORCE_CLOSE(infd);
> +    if (infd != STDIN_FILENO)
> +        VIR_FORCE_CLOSE(infd);
>     VIR_FORCE_CLOSE(null);
> -    tmpfd = childout;   /* preserve childout value */
> -    VIR_FORCE_CLOSE(tmpfd);
> -    if (childerr > 0 &&
> +    if (childout != STDOUT_FILENO) {
> +        tmpfd = childout;   /* preserve childout value */
> +        VIR_FORCE_CLOSE(tmpfd);

Took me a moment to understand this. I think in case you pass parent's
std{in,out,err} to child's std{in,out,err} this works. But when you
exchange stdout and stderr like this: parent std{in,err,out} -> child
std{in,out,err}, then childout != STDOUT_FILENO is wrong and should be
childout > STDERR_FILENO, otherwise you could close the parent's
stderr here.

> +    }
> +    if (childerr > STDERR_FILENO &&
>         childerr != childout) {
>         VIR_FORCE_CLOSE(childerr);
> -        childout = -1;

Technically it's okay to remove this like as childout isn't accessed
afterwards anymore. But by using the tmpfd variable we tricked
VIR_FORCE_CLOSE and should finish it's job here.

ACK, with that comments addressed.

Mathias