bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Andrew Whatson]

Hello Josselin & Ludo,

Thanks for your work on this, posix_spawn is a nice improvement!

My comments on the changes in the wip-posix-spawn branch follow.

In doc/ref/posix.texi:

> @quotation Note
> If you are only looking to fork+exec with some pipes set up, using 
> pipes
> or the more @code{spawn} procedure described below will be more robust
> (in particular in multi-threaded contexts), more portable, and usually
> more efficient.
> @end quotation

Should be "... the more primitive @code{spawn} procedure".

> @deffn {Scheme Procedure} spawn @var{program} @var{arguments} @
>        [#:environment=(environ)] @
>        [#:input=(current-input-port)] @
>        [#:output=(current-output-port)] @
>        [#:error=(current-error-port)] @
>        [#:search-path?=#t]
> Spawn a new child process executing @var{program} with the
> given @var{arguments}, a list of one or more strings, and
> return its PID.  Raise a @code{system-error} exception if
> @var{program} could not be found or could not be executed.

The description of arguments as "a list of one or more strings" is
surprising, I think some users might expect to be able to pass zero
arguments, but we don't explain what the single argument should be.

An earlier version of the docs for this function clarified this with
"(which must include the name of the executable as a first element)".
The manual for exec(3) says "the first argument, by convention, should
point to the filename associated with the file being executed."

Maybe we could clarify in a second paragraph: "By convention, the first
element of @var{arguments} should be the name of the program being
executed.  If calling a C program, for example, this will be passed as
the value for @code{argv[0]}."

In libguile/posix.c, piped_process, around line 1565:

> if (*pid == -1)
>   switch (errno_save)
>     {
>       /* Errors that seemingly come from fork.  */
>     case EAGAIN:
>     case ENOMEM:
>     case ENOSYS:
>       errno = err;
>       free (exec_file);
>       SCM_SYSERROR;
>       break;
>
>     default:    /* ENOENT, etc. */
>       /* Create a dummy process that exits with value 127.  */
>       dprintf (err, "In execvp of %s: %s\n", exec_file,
>                strerror (errno_save));
>     }

The "dummy process" comment here is misleading, this function simply
returns the error code.  Its callers 'scm_piped_process' and
'scm_system_star' are responsible for simulating exit code 127, and are
already well commented.  Probably just remove this redundant comment.

Otherwise, looks good to me :)

Thanks again!

Cheers,
Andrew