All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Aleksey Kliger <alklig@microsoft.com>
Subject: [PATCH] config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur
Date: Mon, 1 Mar 2021 04:29:47 -0500	[thread overview]
Message-ID: <YDy0C9sRvboGXQ7P@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq8s7ascby.fsf@gitster.g>

On Fri, Feb 26, 2021 at 02:17:53PM -0800, Junio C Hamano wrote:

> > +	/*
> > +	 * Also O_TMPFILE would take a mode, but it isn't defined everywhere.
> > +	 * And anyway, we don't use it in our code base.
> > +	 */
> 
> That is being extra careful---I like it very much.

I wondered what would happen if my "anyway" above is wrong. We at least
would not invoke undefined behavior (because we'd avoid looking at the
mode parameter even though it exists), but would pass a "0" mode to the
real open(). Presumably somebody would notice that. :)

> > +	if (flags & O_CREAT) {
> > +		va_list ap;
> > +		va_start(ap, flags);
> > +		mode = va_arg(ap, int);
> > +		va_end(ap);
> > +	}
> > +
> > +	do {
> > +		ret = open(path, flags, mode);
> > +	} while (ret < 0 && errno == EINTR);
> > +
> > +	return ret;
> > +}
> 
> Thanks.

I got another off-list report of the problem. I think we probably want
to do this on top:

-- >8 --
Subject: config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur

We've had mixed reports on whether the latest release of macOS needs
this Makefile knob set. In most reported cases, there's antivirus
software running (which one might imagine could cause an open() call to
be delayed). However, one of the (off-list) reports I've gotten
indicated that it happened on an otherwise clean install of Big Sur.

Since the symptom is so bad (checkout randomly fails to write several
fails when the progress meter kicks in), and since the workaround is so
lightweight (if we don't see EINTR, it's just an extra conditional
check), let's just turn it on by default.

Signed-off-by: Jeff King <peff@peff.net>
---
Apparently Big Sur jumped from macOS 10.x to 11.x. But our "uname -r"
check gives the "Darwin version", in which it is 20.x (following 19.x
for the previous version). At least according to some sources I found
online. :) So that is good, because otherwise all of our uname_R checks
here would have been broken. I don't have a Big Sur machine handy to
test with, but I believe this should do what we want.

 config.mak.uname | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index e22d4b6d67..d204c20a64 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -124,6 +124,9 @@ ifeq ($(uname_S),Darwin)
 	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 && echo 1),1)
 		HAVE_GETDELIM = YesPlease
 	endif
+	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 20 && echo 1),1)
+		OPEN_RETURNS_EINTR = UnfortunatelyYes
+	endif
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease
-- 
2.31.0.rc0.521.g56be7fa5e1


  reply	other threads:[~2021-03-01  9:30 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-24  4:43 [PATCH] wrapper: add workaround for open() returning EINTR Jeff King
2021-02-24  4:46 ` Jeff King
2021-02-24  5:36   ` Junio C Hamano
2021-02-24 18:20     ` Jeff King
2021-02-26  6:14       ` [PATCH v2] Makefile: add OPEN_RETURNS_EINTR knob Jeff King
2021-02-26 22:17         ` Junio C Hamano
2021-03-01  9:29           ` Jeff King [this message]
2021-03-01 17:17             ` [PATCH] config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur Junio C Hamano
2021-03-01 23:57             ` Junio C Hamano
2021-03-03 13:41               ` Jeff King
2021-03-04  0:47                 ` Junio C Hamano
2021-02-24  4:50 ` [PATCH] wrapper: add workaround for open() returning EINTR Taylor Blau
2021-02-24  7:20 ` Johannes Sixt
2021-02-24 18:23   ` Jeff King
2021-02-26  6:17     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YDy0C9sRvboGXQ7P@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=alklig@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.