Re: open-process and related functions for MinGW Guile

[Eli Zaretskii <eliz@gnu.org>]

> From: ludo@gnu.org (Ludovic Courtès)
> Cc: Mark H Weaver <mhw@netris.org>,  guile-devel@gnu.org
> Date: Sun, 29 Jun 2014 22:21:28 +0200
> 
> > +#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.

Most of the magic is gone in this version.  I will add a comment about
0xC0000000.

> > +# 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.

Gnulib already has such a module, but its design and implementation is
based on wrong premises.  We've been through that with Mark back in
February.

And my experience with Gnulib responsiveness hasn't changed much since
then: 2 tiny patches I submitted were accepted, but a larger patch to
nl_langinfo, which is very important for Guile, was left without a
comment for the past 3 weeks.

> > -#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.

Which ones?

> >    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.)

It fixes a compiler warning.

> > -      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?

Ignore this, I wasn't aware a change was made there.

> > --- /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”

I was just following the example of win32-uname.c.

Thanks for the other feedback.