[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: open-process and related functions for MinGW Guile
From: |
Ludovic Courtès |
Subject: |
Re: open-process and related functions for MinGW Guile |
Date: |
Sun, 29 Jun 2014 22:21:28 +0200 |
User-agent: |
Gnus/5.130009 (Ma Gnus v0.9) Emacs/24.3 (gnu/linux) |
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’.