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

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> +/* We have to and actually can handle cancelable system().  The big
> +   problem: we have to kill the child process if necessary.  To do
> +   this a cleanup handler has to be registered and it has to be able
> +   to find the PID of the child.  The main problem is to reliable have
> +   the PID when needed.  It is not necessary for the parent thread to
> +   return.  It might still be in the kernel when the cancellation
> +   request comes.  Therefore we have to use the clone() calls ability
> +   to have the kernel write the PID into the user-level variable.  */

This comment does not look relevant to me anymore.

> -#ifdef _LIBC_REENTRANT
> -static struct sigaction intr, quit;
> -static int sa_refcntr;
> -__libc_lock_define_initialized (static, lock);
> +#ifdef SIGCANCEL
> +
> +struct cancel_handler_args
> +{
> +  struct sigaction *quit;
> +  struct sigaction *intr;
> +  pid_t pid;

Trailing whitespace on the last line.

> +# define CLEANUP_HANDLER(q, i, p) \
> +  __libc_cleanup_region_start (1, cancel_handler,		\
> +			       &((struct cancel_handler_args) {	\
> +			         .quit = &(q),			\
> +				 .intr = &(i),			\
> +				 .pid = (p) }))
> +# define CLEANUP_RESET() \
> +  __libc_cleanup_region_end (0)

I don't see the value in those macros, they just hide what is going on.
You should also use a named local variable instead of the compound
literal.  I think it is acceptable to duplicate the actual __waitpid
call for the cancellation and non-cancellation cases.

Rest looks okay to me.

I think we should seriously consider not messing with signals if
__libc_multiple_threads, but that is a separate change.

Thanks,
Florian