Re: [PATCH] wrapper: add workaround for open() returning EINTR

[Jeff King <peff@peff.net>]

On Tue, Feb 23, 2021 at 09:36:25PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Feb 23, 2021 at 11:43:16PM -0500, Jeff King wrote:
> >
> >> The workaround here is enabled all the time, without a Makefile knob,
> >> since it's a complete noop if open() never returns EINTR. I did push it
> >> into its own compat/ source file, though, since it has to #undef our
> >> macro redirection. Putting it in a file with other code risks confusion
> >> if more code is added after that #undef.
> >
> > Whoops, sorry, I had a last-minute change of heart here and just stuck
> > it in wrapper.c with a bit of preprocessor magic to guard it. It felt
> > weird having compat/open.c, when the rest of compat/ is always
> > conditional on Makefile knobs. But I'm happy to go the other way if
> > anybody feels strongly.
> 
> Hmph, just like other workarounds, shouldn't this be "if your
> platform screws this up, define this knob to work it around"?  That
> would make it fit better with the rest of compat/.

I actually started that way, but then I got stuck deciding which
platforms in config.mak.uname to enable it for. We've seen it on Linux
and on macOS. I don't know how prevalent it is on those platforms (or
whether it is indeed a bug that may even get fixed). Since the check is
basically zero-cost at run-time, it seemed like it was worth just being
on guard for it so that nobody ever had to worry about it again.

But I don't feel that strongly. I'd be happy to do it with a Makefile
knob if you prefer.

> I dunno.  A no-op wrapper makes the code less transparent, which I
> am somewhat unhappy.

Yeah. I think the run-time cost is essentially zero, but it does add a
bit of complexity if you are debugging. And handling the magic mode
vararg definitely makes it uglier.

The other thing that gives me pause is that there may be other syscalls
which need the same treatment. Here's a recent discussion that Big Sur
may also return EINTR from the opendir() family:

  https://github.com/dotnet/runtime/issues/47584

Git is presumably susceptible to that, too; it's just that we call
open() a lot more frequently, so that was the obvious one that came up.

I don't think there is a more general way to address this, though. We'd
have to wrap each suspected syscall (but we could presumably at least
tie them all to a single Makefile knob). I'm not excited at the prospect
of preemptively adding such wrappers when we aren't even 100% sure that
there is a problem.

> But a wrapper that is always used even on
> platforms that do not need might give us a better chance of noticing
> a bug in the wrapper itself, making it less likely for us to leave
> only the users of minority broken platforms behind.  So...

That part I'm less worried about. It sounds like we'd want it on for
recent versions of macOS, which would give us pretty wide coverage to
notice a bug.

-Peff