Re: open-process and related functions for MinGW Guile

[Ludovic Courtès]

Hello, Eli,

Eli Zaretskii <address@hidden> skribis:

> This is a sequel to the thread that started here:
>
>   http://lists.gnu.org/archive/html/guile-devel/2014-02/msg00047.html
>
> As agreed with Mark at the end of that thread, please find below
> patches that enable open-process and friends in the MinGW build of
> Guile.  The main changes since the patches I presented in February
> are:
>
>  . Guile's standard handles are not redirected before running the
>    child process.
>
>  . The code which runs the child process supports both a Unixy shell
>    (if available), which is useful for the test suite, the stock
>    Windows shell cmd.exe, and other programs.  This support includes
>    correct handling of quoted command-line arguments.
>
>  . Translation of signals to exit status and back is based on a
>    mapping that produces signal values identical to the ones in
>    signal.h, as opposed to some convention private to Guile.
>
>  . waitpid emulation supports WNOHANG (required to pass the
>    rnrs-libraries test).

Nice!

Preliminary comments below.

> --- libguile/posix.c~ 2014-06-22 19:08:35.862625000 +0300
> +++ libguile/posix.c  2014-06-29 18:36:02.000000000 +0300
> @@ -84,6 +84,32 @@
>  #if HAVE_SYS_WAIT_H
>  # include <sys/wait.h>
>  #endif
> +#ifdef __MINGW32__
> +
> +#include <c-strcase.h>
> +
> +# define WEXITSTATUS(stat_val) ((stat_val) & 255)
> +# define WIFEXITED(stat_val)   (((stat_val) & 0xC0000000) == 0)
> +# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
> +# define WTERMSIG(stat_val)    win32_status_to_termsig (stat_val)
> +/* The funny conditional avoids a compiler warning in status:stop_sig.  */
> +# define WIFSTOPPED(stat_val)  ((stat_val) == (stat_val) ? 0 : 0)
> +# define WSTOPSIG(stat_var)    (0)

I think this was raised in the previous discussion: it looks a bit like
black magic, so there should be a comment explaining why this is needed,
how the constants were chosen, etc.

In addition...

> +# include <process.h>
> +# define HAVE_WAITPID 1
> +  static int win32_status_to_termsig (DWORD);
> +  static int win32_signal_to_status (int);
> +# define getuid()              (500) /* Local Administrator */
> +# define getgid()              (513) /* None */
> +# define setuid(u)             (0)
> +# define setgid(g)             (0)
> +# define WIN32_LEAN_AND_MEAN
> +# include <windows.h>
> +# define WNOHANG               1
> +  int waitpid (intptr_t, int *, int);
> +# include "win32-proc.c"

... what would you think of putting all this in a Gnulib module?  It
would benefit all GNU packages and probably get more testing.

> -#ifdef HAVE_SETEGID
>  SCM_DEFINE (scm_setegid, "setegid", 1, 0, 0,
>              (SCM id),
>           "Sets the effective group ID to the integer @var{id}, provided the 
> process\n"

This should be a separate change, and it’s dubious since there could be
platforms without setegid.

>    exec_argv = scm_i_allocate_string_pointers (args);
>  
> -  execv (exec_file, exec_argv);
> +  execv (exec_file, (char const * const *)exec_argv);

This should be a separate change (if at all needed.)

> -      if (reading)
> +      if (reading) 
>          {
>            close (c2p[1]);
> -          read_port = scm_fdes_to_port (c2p[0], "r0", sym_read_pipe);
> +          read_port = scm_fdes_to_port (c2p[0], "r", sym_read_pipe);
> +          scm_setvbuf (read_port, scm_from_int (_IONBF), SCM_UNDEFINED);
>          }
>        if (writing)
>          {
>            close (p2c[0]);
> -          write_port = scm_fdes_to_port (p2c[1], "w0", sym_write_pipe);
> +          write_port = scm_fdes_to_port (p2c[1], "w", sym_write_pipe);
> +          scm_setvbuf (write_port, scm_from_int (_IONBF), SCM_UNDEFINED);

This reverts a43fa1b.  Could you explain why it’s needed, and make it a
separate patch?

> --- /dev/null 1970-01-01 02:00:00 +0200
> +++ libguile/win32-proc.c     2014-06-29 11:26:08 +0300

Please call it “w32-proc.c” or “woe32-proc.c”, and add the LGPLv3+
license header.

> +/* Translate abnormal exit status of Windows programs into the signal
> +   that terminated the program.  This is required to support scm_kill
> +   and WTERMSIG.  */
> +
> +struct signal_and_status {
> +  int sig;
> +  DWORD status;
> +};
> +
> +static const struct signal_and_status sigtbl[] = {
> +  {SIGSEGV, 0xC0000005},     /* access to invalid address */

Opening braces on a line of their own; spaces around braces.

Thank you,
Ludo’.