Re: [PATCH] Implement open-process and related functions on MinGW

[Eli Zaretskii]

> From: Mark H Weaver <address@hidden>
> Cc: address@hidden (Ludovic Courtès),  address@hidden
> Date: Sat, 22 Feb 2014 10:54:16 -0500
> 
> > diff --git a/libguile/posix.c b/libguile/posix.c
> > index 0443f95..69652a3 100644
> > --- a/libguile/posix.c
> > +++ b/libguile/posix.c
> > @@ -85,6 +85,27 @@
> >  #if HAVE_SYS_WAIT_H
> >  # include <sys/wait.h>
> >  #endif
> > +#ifdef __MINGW32__
> > +# define WEXITSTATUS(stat_val) ((stat_val) & 255)
> > +/* Windows reports exit status from fatal exceptions as 0xC0000nnn
> > +   codes, see winbase.h.  We usurp codes above 0xC0000200 for SIGxxx
> > +   signals.  */
> > +# define WIFEXITED(stat_val)   (((stat_val) & 0xC0000000) == 0)
> > +# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) != 0)
> > +# define WTERMSIG(stat_val)    ((stat_val > 0xC0000200) ? stat_val - 
> > 0xC0000200 : stat_val)
> > +# define WIFSTOPPED(stat_val)  (0)
> > +# define WSTOPSIG(stat_var)    (0)
> 
> Definitions for these on MinGW are available in Gnulib's 'sys_wait'
> module.  I'll import it soon.

Please don't: that gnulib module is dead wrong for Windows.  If we use
it, we will be unable to report even the simplest error exits, let
alone programs that crashed due to a fatal signal.

> > +# include <process.h>
> > +# define waitpid(p,s,o)        _cwait(s, p, WAIT_CHILD)
> > +# define HAVE_WAITPID 1
> 
> Gnulib has a 'waitpid' module that implements it on MinGW.  Can we just
> use that, and then assume it is there instead of setting HAVE_WAITPID?

The gnulib waitpid module does exactly what I did above.  If you are
more comfortable with including it than with a single trivial macro,
then go ahead.  But in general, due to the known problems with getting
Windows-specific patches to gnulib approved and admitted, I'd advise
to stay away of gnulib as much as possible, when Windows is
concerned.  E.g., what if tomorrow we would need to make the waitpid
emulation more sophisticated?

> I'll add it to my list of modules to import, along with the others
> mentioned in this message.
> 
> > +# define getuid()              (500) /* Local Administrator */
> > +# define getgid()              (513) /* None */
> > +# define setuid(u)             (0)
> > +# define setgid(g)             (0)
> 
> Hmm.  I'm not sure about these.  If we can't do better than this, I
> think we should arrange to not bind these functions in MinGW, and not
> call them in our code.  What do you think?

Which functions are you talking about, specifically?

In general, where the Windows equivalent is to do nothing, my advice
would be to have a no-op implementation, rather than an unbound
function.  The former approach makes it much easier to write portable
Guile scripts, instead of spreading boundness checks all over the
place.  FWIW, Emacs takes the former approach with very good results.

> > +# define pipe(f)               _pipe(f, 0, O_NOINHERIT)
> 
> Gnulib has a 'pipe' module that implements it on MinGW.

Same response as for waitpid.

> > +#ifdef __MINGW32__
> > +  else
> > +    {
> > +      HANDLE ph = OpenProcess (PROCESS_TERMINATE, 0, scm_to_int (pid));
> > +      int s = scm_to_int (sig);
> > +
> > +      if (!ph)
> > +   {
> > +     errno = EPERM;
> > +     goto err;
> > +   }
> > +      if (!TerminateProcess (ph, 0xC0000200 + s))
> > +   {
> > +     errno = EINVAL;
> > +     goto err;
> > +   }
> > +      CloseHandle (ph);
> > +    }
> > +#endif     /* __MINGW32__ */
> >  #endif
> >    return SCM_UNSPECIFIED;
> >  }
> 
> This change is not mentioned in the commit log.

Sorry, I sent a wrong commit log.  Here's the correct one:

        * posix.c (WEXITSTATUS, WIFEXITED, WIFSIGNALED, WTERMSIG)
        (WIFSTOPPED, WSTOPSIG) [__MINGW32__]: Define for MinGW.
        (waitpid, getuid, getgid, setuid, setgid, pipe) [__MINGW32__]:
        Define replacements for MinGW.
        (scm_kill) [__MINGW32__]: Implement killing a process on
        MS-Windows.
        (scm_waitpid) [__MINGW32__]: Ignore the options argument.
        (scm_status_exit_val, scm_getuid): No longer ifdef'ed away for MinGW.
        (scm_open_process) [__MINGW32__]: Implement starting a process
        on MS-Windows.  Fix a typo in an error message (execlp instead
        of execvp).
        (scm_init_popen): No longer ifdef'ed away when HAVE_FORK is not
        defined.
        (scm_init_posix): The scm_init_popen feature is now available
        even if HAVE_FORK is not defined.

> Can you use the GNU coding conventions for placement of brackets?

Sorry, I don't follow: which part is not according to the GNU coding
conventions?

> What is the meaning of 0xC0000200?  It would be good to add a comment
> explaining what that means, or better yet use preprocessor defines
> (if they are available in a header).

It's explained where WEXIT* macros are defined.  I will add another
comment here pointing there.

> Ideally this should be implemented in Gnulib, but I'm okay with
> including this in Guile for now.

Thanks.  As I wrote above, gnulib doesn't have a functional
implementation of these macros and the related functionality.

> > @@ -720,6 +760,7 @@ SCM_DEFINE (scm_waitpid, "waitpid", 1, 1, 0,
> >  {
> >    int i;
> >    int status;
> > +#ifndef __MINGW32__
> >    int ioptions;
> >    if (SCM_UNBNDP (options))
> >      ioptions = 0;
> > @@ -728,6 +769,7 @@ SCM_DEFINE (scm_waitpid, "waitpid", 1, 1, 0,
> >        /* Flags are interned in scm_init_posix.  */
> >        ioptions = scm_to_int (options);
> >      }
> > +#endif
> >    SCM_SYSCALL (i = waitpid (scm_to_int (pid), &status, ioptions));
> >    if (i == -1)
> >      SCM_SYSERROR;
> 
> Gnulib has a 'waitpid' module that apparently implements it on MinGW.
> Can we use that?

See above.

> Also, this change is not mentioned in the commit log.

See above.

> Thanks for working on this, but in a multithreaded program, it's no good
> to change the file descriptors in the main program temporarily before
> spawning, and then restore them afterwards.  We'll have to find another
> way of doing this.

Threads don't work in the MinGW build anyway, and no one was able to
help me understand why (see the discussions last August).  As long as
this limitation exists, this is a non-issue, and certainly better than
not having this functionality available at all.

The only other way I know of is to bypass Posix-like functions like
'open', 'close', and 'dup', and go to the level of Win32 API, where a
function that creates a child subprocess can accept handles for the
child's standard I/O.  If you will be comfortable with a lot more
Widows specific stuff, I can work on that.

Thanks.