Re: [PATCH v3 1/2] posix: Use posix_spawn on popen

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

>   * BZ#22834: now that proc_file_chain is not copied on another process, it
>     just require to access is through the proc_file_chain_lock.

Sorry, what does that mean? (“copied on another process” in particular.)

> +* The popen and system do not run atfork handlers anymore (BZ#17490).
> +  Although it is a possible POSIX violation, the POSIX rationale in
> +  pthread_atfork documentation regarding atfork handlers is to handle
> +  incosistent mutex state after fork call in multithread environment.
> +  In both popen and system there is no direct access to user-defined mutexes.
> +
>  Deprecated and removed features, and other changes affecting compatibility:
>  
>  * The glibc.tune tunable namespace has been renamed to glibc.cpu and the
> diff --git a/libio/iopopen.c b/libio/iopopen.c
> index 2eff45b4c8..da24e60eef 100644
> --- a/libio/iopopen.c
> +++ b/libio/iopopen.c
> @@ -34,7 +34,8 @@
>  #include <not-cancel.h>
>  #include <sys/types.h>
>  #include <sys/wait.h>
> -#include <kernel-features.h>
> +#include <spawn.h>
> +#include <paths.h>
>  
>  struct _IO_proc_file
>  {
> @@ -64,8 +65,8 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>  {
>    int read_or_write;
>    int parent_end, child_end;
> +  int child_std_end;

Should this be child_pipe_fd or something more descriptive?  Perhaps
also add a comment to the previous line that these are indexes into
pipe_fds below?

> +    if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[child_end],
> +	child_std_end) != 0)
> +      goto spawn_failure;
> +
> +    /* POSIX.2: "popen() shall ensure that any streams from previous popen()
> +       calls that remain open in the parent process are closed in the new
> +       child process." */
> +    bool addclose_failure = false;
> +#ifdef _IO_MTSAFE_IO
> +    _IO_cleanup_region_start_noarg (unlock);
> +    _IO_lock_lock (proc_file_chain_lock);
> +#endif
> +    for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
> +      {
> +	int fd = _IO_fileno ((FILE *) p);
> +
> +	/* If any stream from previous popen() calls has fileno
> +	   child_send, it has been already closed by the dup2 syscall
> +	   above.  */

Typo: child_send.  But I'm not sure how this can happen.  Shouldn't the
descriptor still be open if the stream is linked into the chain?

If not, that doesn't really fix the race.

Rest looks okay to me.

Thanks,
Florian