Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Di., 29. Dez. 2020 um 17:29 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> > > > Btw, regarding use of posix_spawn, I'd expect a discussion before we
> > > > make such a change.  AFAIU it is not a trivial decision, as
> > > > posix_spawn has its down sides, and therefore is not necessarily the
> > > > best API for running sub-processes on every supported platform, even
> > > > if you consider only the Posix ones.  We should consider the
> > > > advantages and disadvantages before we make the decision.
> > >
> > > Sure, I'm happy to have that discussion. I briefly reviewed the
> > > posix_spawn implementation of GNU libc and Gnulib, and found that it
> > > uses vfork/clone + execve like our hand-rolled code, so I wouldn't
> > > expect any significant change. The primary advantage is to offload
> > > complexity into a library that can properly deal with system-specific
> > > issues and can improve over time. For example, on Linux, posix_spawn
> > > can use clone instead of vfork.
> >
> > See Savannah bug #59093 for one subtle issue:
> >
> >   https://savannah.gnu.org/bugs/?59093
> >
> > Since Emacs also sets its stack limit in some cases, this could be
> > directly relevant to us.  (But I didn't look into it close enough to
> > tell whether it actually is relevant.)
>
> I think that specific problem isn't relevant (we don't change the
> stack size between fork and exec), but the fix to
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869 (ironically
> reported by me) is.
> As indicated in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869#8,
> it might be better to solve the underlying problem in a different way
> that doesn't involve changing resource limits (changing a
> process-global setting is somewhat fishy anyway, especially with
> modules). Essentially we just need to make sure to not add file
> descriptors larger than FD_SETSIZE to an fd_set.

I've now done this with commit
8bc85d46cc9214a531f2d2ecb3f5fb48af8105a6. While the setrlimit approach
is nominally cleaner, it can be subverted because soft rlimits can be
changed arbitrarily, so I'd propose we revert the commits that
introduced the setrlimit calls
(b6d9613df83813609ef80da45975e70954d1fb6d,
a5509099484e0762842bc2c9e914779397b91469).