{"thread":{"id":"55202","subject":"[PATCH] wrapper: add workaround for open() returning EINTR","startedAt":"2021-02-24T04:44:15Z","lastAt":"2021-03-04T01:09:30Z","messageCount":15,"participants":["Jeff King","Taylor Blau","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"417657","messageId":"YDXZY8XFRayiM1If@coredump.intra.peff.net","threadId":"55202","inReplyTo":null,"subject":"[PATCH] wrapper: add workaround for open() returning EINTR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-24T04:43:15Z","receivedAt":"2021-02-24T04:44:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On some platforms, open() reportedly returns EINTR when opening regular\nfiles and we receive a signal (usually SIGALRM from our progress meter).\nThis shouldn't happen, as open() should be a restartable syscall, and we\nspecify SA_RESTART when setting up the alarm handler. So it may actually\nbe a kernel or libc bug for this to happen. But it has been reported on\nat least one version of Linux (on a network filesystem):\n\n https://lore.kernel.org/git/c8061cce-71e4-17bd-a56a-a5fed93804da@neanderfunk.de/\n\nas well as on macOS starting with Big Sur even on a regular filesystem.\n\nWe can work around it by retrying open() calls that get EINTR, just as\nwe do for read(), etc. Since we don't ever _want_ to interrupt an open()\ncall, we can get away with just redefining open, rather than insisting\nall callsites use xopen().\n\nWe actually do have an xopen() wrapper already (and it even does this\nretry, though there's no indication of it being an observed problem back\nthen; it seems simply to have been lifted from xread(), etc). But it is\nused hardly anywhere, and isn't suitable for general use because it will\ndie() on error. In theory we could combine the two, but it's awkward to\ndo so because of the variable-args interface of open().\n\nThe workaround here is enabled all the time, without a Makefile knob,\nsince it's a complete noop if open() never returns EINTR. I did push it\ninto its own compat/ source file, though, since it has to #undef our\nmacro redirection. Putting it in a file with other code risks confusion\nif more code is added after that #undef.\n\nReported-by: Aleksey Kliger \nSigned-off-by: Jeff King \n---\nThis was reported to me off-list. Aleksey was kind enough to test the\npatch on a system that was reliably showing the problem during the\ncheckout phase of git-clone (which naturally has both a progress meter\nand is calling open() on a ton of files).\n\nI do still think the OS is doing the wrong thing here. But even if I'm\nright, it's probably prudent to work around it.\n\n git-compat-util.h | 4 ++++\n wrapper.c | 29 +++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 838246289c..2be022c422 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -788,6 +788,10 @@ int git_vsnprintf(char *str, size_t maxsize,\n \t\t const char *format, va_list ap);\n #endif\n \n+#undef open\n+#define open git_open_with_retry\n+int git_open_with_retry(const char *path, int flag, ...);\n+\n #ifdef __GLIBC_PREREQ\n #if __GLIBC_PREREQ(2, 1)\n #define HAVE_STRCHRNUL\ndiff --git a/wrapper.c b/wrapper.c\nindex bcda41e374..130f441064 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -678,3 +678,32 @@ int is_empty_or_missing_file(const char *filename)\n \n \treturn !st.st_size;\n }\n+\n+/*\n+ * Do not add other callers of open() after this. Our redefinition\n+ * below will ensure the compiler catches any.\n+ */\n+#undef open\n+int git_open_with_retry(const char *path, int flags, ...)\n+{\n+\tmode_t mode = 0;\n+\tint ret;\n+\n+\t/*\n+\t * Also O_TMPFILE would take a mode, but it isn't defined everywhere.\n+\t * And anyway, we don't use it in our code base.\n+\t */\n+\tif (flags & O_CREAT) {\n+\t\tva_list ap;\n+\t\tva_start(ap, flags);\n+\t\tmode = va_arg(ap, int);\n+\t\tva_end(ap);\n+\t}\n+\n+\tdo {\n+\t\tret = open(path, flags, mode);\n+\t} while (ret < 0 && errno == EINTR);\n+\n+\treturn ret;\n+}\n+#define open do_not_allow_use_of_bare_open\n-- \n2.30.1.1095.g03347429ea\n"},{"id":"417658","messageId":"YDXaJHnZ5LgCj9NX@coredump.intra.peff.net","threadId":"55202","inReplyTo":"YDXZY8XFRayiM1If@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: add workaround for open() returning EINTR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-24T04:46:28Z","receivedAt":"2021-02-24T04:47:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 23, 2021 at 11:43:16PM -0500, Jeff King wrote:\n\n> The workaround here is enabled all the time, without a Makefile knob,\n> since it's a complete noop if open() never returns EINTR. I did push it\n> into its own compat/ source file, though, since it has to #undef our\n> macro redirection. Putting it in a file with other code risks confusion\n> if more code is added after that #undef.\n\nWhoops, sorry, I had a last-minute change of heart here and just stuck\nit in wrapper.c with a bit of preprocessor magic to guard it. It felt\nweird having compat/open.c, when the rest of compat/ is always\nconditional on Makefile knobs. But I'm happy to go the other way if\nanybody feels strongly.\n\n-Peff\n"},{"id":"417659","messageId":"YDXbDwOwNDJUZa7b@nand.local","threadId":"55202","inReplyTo":"YDXZY8XFRayiM1If@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: add workaround for open() returning EINTR","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-24T04:50:23Z","receivedAt":"2021-02-24T04:51:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 23, 2021 at 11:43:15PM -0500, Jeff King wrote:\n> The workaround here is enabled all the time, without a Makefile knob,\n> since it's a complete noop if open() never returns EINTR. I did push it\n> into its own compat/ source file, though, since it has to #undef our\n> macro redirection. Putting it in a file with other code risks confusion\n> if more code is added after that #undef.\n\nHmm. The patch below defines it in wrapper.c. Intentional?\n\n> I do still think the OS is doing the wrong thing here. But even if I'm\n> right, it's probably prudent to work around it.\n\nRegardless of the above, I agree that if your explanation is true (and I\nhave no reason to believe that it isn't) that the OS is indeed doing the\nwrong thing here.\n\nThat patch below looks quite reasonable, thanks.\n\n\nThanks,\nTaylor\n"},{"id":"417664","messageId":"xmqqzgzuyqli.fsf@gitster.g","threadId":"55202","inReplyTo":"YDXaJHnZ5LgCj9NX@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: add workaround for open() returning EINTR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-24T05:36:25Z","receivedAt":"2021-02-24T05:37:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 23, 2021 at 11:43:16PM -0500, Jeff King wrote:\n>\n>> The workaround here is enabled all the time, without a Makefile knob,\n>> since it's a complete noop if open() never returns EINTR. I did push it\n>> into its own compat/ source file, though, since it has to #undef our\n>> macro redirection. Putting it in a file with other code risks confusion\n>> if more code is added after that #undef.\n>\n> Whoops, sorry, I had a last-minute change of heart here and just stuck\n> it in wrapper.c with a bit of preprocessor magic to guard it. It felt\n> weird having compat/open.c, when the rest of compat/ is always\n> conditional on Makefile knobs. But I'm happy to go the other way if\n> anybody feels strongly.\n\nHmph, just like other workarounds, shouldn't this be \"if your\nplatform screws this up, define this knob to work it around\"? That\nwould make it fit better with the rest of compat/.\n\nI dunno. A no-op wrapper makes the code less transparent, which I\nam somewhat unhappy. But a wrapper that is always used even on\nplatforms that do not need might give us a better chance of noticing\na bug in the wrapper itself, making it less likely for us to leave\nonly the users of minority broken platforms behind. So...\n\n\n"},{"id":"417670","messageId":"acb282ee-fc55-bbe0-8272-087bde180ea2@kdbg.org","threadId":"55202","inReplyTo":"YDXZY8XFRayiM1If@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: add workaround for open() returning EINTR","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-24T07:20:57Z","receivedAt":"2021-02-24T07:21:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.02.21 um 05:43 schrieb Jeff King:\n> The workaround here is enabled all the time, without a Makefile knob,\n> since it's a complete noop if open() never returns EINTR. I did push it\n> into its own compat/ source file, though, since it has to #undef our\n> macro redirection. Putting it in a file with other code risks confusion\n> if more code is added after that #undef.\n\nI'm not so much opposed to \"enable it all the time\" in general, but when\nwe already have an override of open(), like for the Windows case in\ncompat/mingw.h, I find it a bit rough to put another wrapper around it,\neven more so since we won't have the EINTR problem on Windows due to the\nabsence of signals.\n\n-- Hannes\n"},{"id":"417691","messageId":"YDaY/M3Rw+6xwZlf@coredump.intra.peff.net","threadId":"55202","inReplyTo":"xmqqzgzuyqli.fsf@gitster.g","subject":"Re: [PATCH] wrapper: add workaround for open() returning EINTR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-24T18:20:44Z","receivedAt":"2021-02-24T18:21:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 23, 2021 at 09:36:25PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Feb 23, 2021 at 11:43:16PM -0500, Jeff King wrote:\n> >\n> >> The workaround here is enabled all the time, without a Makefile knob,\n> >> since it's a complete noop if open() never returns EINTR. I did push it\n> >> into its own compat/ source file, though, since it has to #undef our\n> >> macro redirection. Putting it in a file with other code risks confusion\n> >> if more code is added after that #undef.\n> >\n> > Whoops, sorry, I had a last-minute change of heart here and just stuck\n> > it in wrapper.c with a bit of preprocessor magic to guard it. It felt\n> > weird having compat/open.c, when the rest of compat/ is always\n> > conditional on Makefile knobs. But I'm happy to go the other way if\n> > anybody feels strongly.\n> \n> Hmph, just like other workarounds, shouldn't this be \"if your\n> platform screws this up, define this knob to work it around\"? That\n> would make it fit better with the rest of compat/.\n\nI actually started that way, but then I got stuck deciding which\nplatforms in config.mak.uname to enable it for. We've seen it on Linux\nand on macOS. I don't know how prevalent it is on those platforms (or\nwhether it is indeed a bug that may even get fixed). Since the check is\nbasically zero-cost at run-time, it seemed like it was worth just being\non guard for it so that nobody ever had to worry about it again.\n\nBut I don't feel that strongly. I'd be happy to do it with a Makefile\nknob if you prefer.\n\n> I dunno. A no-op wrapper makes the code less transparent, which I\n> am somewhat unhappy.\n\nYeah. I think the run-time cost is essentially zero, but it does add a\nbit of complexity if you are debugging. And handling the magic mode\nvararg definitely makes it uglier.\n\nThe other thing that gives me pause is that there may be other syscalls\nwhich need the same treatment. Here's a recent discussion that Big Sur\nmay also return EINTR from the opendir() family:\n\n https://github.com/dotnet/runtime/issues/47584\n\nGit is presumably susceptible to that, too; it's just that we call\nopen() a lot more frequently, so that was the obvious one that came up.\n\nI don't think there is a more general way to address this, though. We'd\nhave to wrap each suspected syscall (but we could presumably at least\ntie them all to a single Makefile knob). I'm not excited at the prospect\nof preemptively adding such wrappers when we aren't even 100% sure that\nthere is a problem.\n\n> But a wrapper that is always used even on\n> platforms that do not need might give us a better chance of noticing\n> a bug in the wrapper itself, making it less likely for us to leave\n> only the users of minority broken platforms behind. So...\n\nThat part I'm less worried about. It sounds like we'd want it on for\nrecent versions of macOS, which would give us pretty wide coverage to\nnotice a bug.\n\n-Peff\n"},{"id":"417692","messageId":"YDaZtqRvdZWkyQVp@coredump.intra.peff.net","threadId":"55202","inReplyTo":"acb282ee-fc55-bbe0-8272-087bde180ea2@kdbg.org","subject":"Re: [PATCH] wrapper: add workaround for open() returning EINTR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-24T18:23:50Z","receivedAt":"2021-02-24T18:24:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 24, 2021 at 08:20:57AM +0100, Johannes Sixt wrote:\n\n> Am 24.02.21 um 05:43 schrieb Jeff King:\n> > The workaround here is enabled all the time, without a Makefile knob,\n> > since it's a complete noop if open() never returns EINTR. I did push it\n> > into its own compat/ source file, though, since it has to #undef our\n> > macro redirection. Putting it in a file with other code risks confusion\n> > if more code is added after that #undef.\n> \n> I'm not so much opposed to \"enable it all the time\" in general, but when\n> we already have an override of open(), like for the Windows case in\n> compat/mingw.h, I find it a bit rough to put another wrapper around it,\n> even more so since we won't have the EINTR problem on Windows due to the\n> absence of signals.\n\nThat's fair. I don't think my new wrapper would interact well with\nmingw_open(). They are both trying to #define open. I think since mine\ncomes later in git-compat-util.h, it is probably overriding mingw_open()\ncompletely on Windows, which is quite bad (we call into my function in\nwrapper.c, but then its \"#undef open\" means we get the original open(),\nnot the previously defined wrapper).\n\n-Peff\n"},{"id":"417870","messageId":"YDiRywyld/0OTT5U@coredump.intra.peff.net","threadId":"55202","inReplyTo":"YDaY/M3Rw+6xwZlf@coredump.intra.peff.net","subject":"[PATCH v2] Makefile: add OPEN_RETURNS_EINTR knob","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-26T06:14:35Z","receivedAt":"2021-02-26T06:15:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 24, 2021 at 01:20:44PM -0500, Jeff King wrote:\n\n> > Hmph, just like other workarounds, shouldn't this be \"if your\n> > platform screws this up, define this knob to work it around\"? That\n> > would make it fit better with the rest of compat/.\n> \n> I actually started that way, but then I got stuck deciding which\n> platforms in config.mak.uname to enable it for. We've seen it on Linux\n> and on macOS. I don't know how prevalent it is on those platforms (or\n> whether it is indeed a bug that may even get fixed). Since the check is\n> basically zero-cost at run-time, it seemed like it was worth just being\n> on guard for it so that nobody ever had to worry about it again.\n> \n> But I don't feel that strongly. I'd be happy to do it with a Makefile\n> knob if you prefer.\n\nSo here's a v2 that turns it into a Makefile knob.\n\nThis was motivated by two things:\n\n - it must not be enabled on Windows, where it overrides the existing\n mingw_open()\n\n - I couldn't replicate it on a Big Sur machine, using the same\n sequence that Aleksey did (basically just cloning and checking out\n dotnet/runtime.git from scratch).\n\n So it's not entirely clear to me how widespread the problem is. A\n conservative approach (at least in terms of change from the status\n quo) is to introduce this knob but not enable it automatically\n (which this patch does). And then if somebody runs into it, they can\n try turning the knob.\n\n It's less conservative in the sense that we won't be preemptively\n protecting people. I dunno. Once we have the knob, it would be\n pretty easy to just flip the default.\n\n-- >8 --\nSubject: [PATCH] Makefile: add OPEN_RETURNS_EINTR knob\n\nOn some platforms, open() reportedly returns EINTR when opening regular\nfiles and we receive a signal (usually SIGALRM from our progress meter).\nThis shouldn't happen, as open() should be a restartable syscall, and we\nspecify SA_RESTART when setting up the alarm handler. So it may actually\nbe a kernel or libc bug for this to happen. But it has been reported on\nat least one version of Linux (on a network filesystem):\n\n https://lore.kernel.org/git/c8061cce-71e4-17bd-a56a-a5fed93804da@neanderfunk.de/\n\nas well as on macOS starting with Big Sur even on a regular filesystem.\n\nWe can work around it by retrying open() calls that get EINTR, just as\nwe do for read(), etc. Since we don't ever _want_ to interrupt an open()\ncall, we can get away with just redefining open, rather than insisting\nall callsites use xopen().\n\nWe actually do have an xopen() wrapper already (and it even does this\nretry, though there's no indication of it being an observed problem back\nthen; it seems simply to have been lifted from xread(), etc). But it is\nused hardly anywhere, and isn't suitable for general use because it will\ndie() on error. In theory we could combine the two, but it's awkward to\ndo so because of the variable-args interface of open().\n\nThis patch adds a Makefile knob for enabling the workaround. It's not\nenabled by default for any platforms in config.mak.uname yet, as we\ndon't have enough data to decide how common this is (I have not been\nable to reproduce on either Linux or Big Sur myself). It may be worth\nenabling preemptively anyway, since the cost is pretty low (if we don't\nsee an EINTR, it's just an extra conditional).\n\nHowever, note that we must not enable this on Windows. It doesn't do\nanything there, and the macro overrides the existing mingw_open()\nredirection. I've added a preemptive #undef here in the mingw header\n(which is processed first) to just quietly disable it (we could also\nmake it an #error, but there is little point in being so aggressive).\n\nReported-by: Aleksey Kliger \nSigned-off-by: Jeff King \n---\n Makefile | 7 +++++++\n compat/mingw.h | 1 +\n compat/open.c | 25 +++++++++++++++++++++++++\n git-compat-util.h | 6 ++++++\n 4 files changed, 39 insertions(+)\n create mode 100644 compat/open.c\n\ndiff --git a/Makefile b/Makefile\nindex 9b1bde2e0e..c4872ca16e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -22,6 +22,9 @@ all::\n # when attempting to read from an fopen'ed directory (or even to fopen\n # it at all).\n #\n+# Define OPEN_RETURNS_EINTR if your open() system call may return EINTR\n+# when a signal is received (as opposed to restarting).\n+#\n # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n #\n # Define USE_LIBPCRE if you have and want to use libpcre. Various\n@@ -1538,6 +1541,10 @@ ifdef FREAD_READS_DIRECTORIES\n \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n \tCOMPAT_OBJS += compat/fopen.o\n endif\n+ifdef OPEN_RETURNS_EINTR\n+\tCOMPAT_CFLAGS += -DOPEN_RETURNS_EINTR\n+\tCOMPAT_OBJS += compat/open.o\n+endif\n ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex af8eddd73e..c9a52ad64a 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -227,6 +227,7 @@ int mingw_rmdir(const char *path);\n \n int mingw_open (const char *filename, int oflags, ...);\n #define open mingw_open\n+#undef OPEN_RETURNS_EINTR\n \n int mingw_fgetc(FILE *stream);\n #define fgetc mingw_fgetc\ndiff --git a/compat/open.c b/compat/open.c\nnew file mode 100644\nindex 0000000000..eb3754a23b\n--- /dev/null\n+++ b/compat/open.c\n@@ -0,0 +1,25 @@\n+#include \"git-compat-util.h\"\n+\n+#undef open\n+int git_open_with_retry(const char *path, int flags, ...)\n+{\n+\tmode_t mode = 0;\n+\tint ret;\n+\n+\t/*\n+\t * Also O_TMPFILE would take a mode, but it isn't defined everywhere.\n+\t * And anyway, we don't use it in our code base.\n+\t */\n+\tif (flags & O_CREAT) {\n+\t\tva_list ap;\n+\t\tva_start(ap, flags);\n+\t\tmode = va_arg(ap, int);\n+\t\tva_end(ap);\n+\t}\n+\n+\tdo {\n+\t\tret = open(path, flags, mode);\n+\t} while (ret < 0 && errno == EINTR);\n+\n+\treturn ret;\n+}\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 838246289c..551cc9f22f 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -788,6 +788,12 @@ int git_vsnprintf(char *str, size_t maxsize,\n \t\t const char *format, va_list ap);\n #endif\n \n+#ifdef OPEN_RETURNS_EINTR\n+#undef open\n+#define open git_open_with_retry\n+int git_open_with_retry(const char *path, int flag, ...);\n+#endif\n+\n #ifdef __GLIBC_PREREQ\n #if __GLIBC_PREREQ(2, 1)\n #define HAVE_STRCHRNUL\n-- \n2.31.0.rc0.520.g3ffb8d01f4\n\n"},{"id":"417871","messageId":"YDiSfXczltBTM+/T@coredump.intra.peff.net","threadId":"55202","inReplyTo":"YDaZtqRvdZWkyQVp@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: add workaround for open() returning EINTR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-26T06:17:33Z","receivedAt":"2021-02-26T06:18:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 24, 2021 at 01:23:51PM -0500, Jeff King wrote:\n\n> > I'm not so much opposed to \"enable it all the time\" in general, but when\n> > we already have an override of open(), like for the Windows case in\n> > compat/mingw.h, I find it a bit rough to put another wrapper around it,\n> > even more so since we won't have the EINTR problem on Windows due to the\n> > absence of signals.\n> \n> That's fair. I don't think my new wrapper would interact well with\n> mingw_open(). They are both trying to #define open. I think since mine\n> comes later in git-compat-util.h, it is probably overriding mingw_open()\n> completely on Windows, which is quite bad (we call into my function in\n> wrapper.c, but then its \"#undef open\" means we get the original open(),\n> not the previously defined wrapper).\n\nThanks again for pointing out mingw_open(). The v2 I posted should avoid\nany bad interactions there, even if we do end up enabling it all the\ntime.\n\nBy the way, I did notice something funny when looking at mingw_open().\nIt unconditionally does:\n\n va_start(args, oflags);\n mode = va_arg(args, int);\n va_end(args);\n\nno matter what we see in oflags. But callers who are not passing O_CREAT\nwill not provide a third argument at all. So probably it is loading\nrandom trash from the stack and passing it around in the \"mode\"\nvariable. This doesn't hurt anything, because ultimately we pass the\ntrash along to the real open function, which will ignore it without\nO_CREAT. But it is definitely undefined behavior to call va_arg() at all\nin this instance.\n\n-Peff\n"},{"id":"417915","messageId":"xmqq8s7ascby.fsf@gitster.g","threadId":"55202","inReplyTo":"YDiRywyld/0OTT5U@coredump.intra.peff.net","subject":"Re: [PATCH v2] Makefile: add OPEN_RETURNS_EINTR knob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-26T22:17:53Z","receivedAt":"2021-02-26T22:18:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> However, note that we must not enable this on Windows. It doesn't do\n> anything there, and the macro overrides the existing mingw_open()\n> redirection. I've added a preemptive #undef here in the mingw header\n> (which is processed first) to just quietly disable it (we could also\n> make it an #error, but there is little point in being so aggressive).\n> ...\n> diff --git a/compat/open.c b/compat/open.c\n> new file mode 100644\n> index 0000000000..eb3754a23b\n> --- /dev/null\n> +++ b/compat/open.c\n> @@ -0,0 +1,25 @@\n> +#include \"git-compat-util.h\"\n> +\n> +#undef open\n> +int git_open_with_retry(const char *path, int flags, ...)\n> +{\n> +\tmode_t mode = 0;\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Also O_TMPFILE would take a mode, but it isn't defined everywhere.\n> +\t * And anyway, we don't use it in our code base.\n> +\t */\n\nThat is being extra careful---I like it very much.\n\n> +\tif (flags & O_CREAT) {\n> +\t\tva_list ap;\n> +\t\tva_start(ap, flags);\n> +\t\tmode = va_arg(ap, int);\n> +\t\tva_end(ap);\n> +\t}\n> +\n> +\tdo {\n> +\t\tret = open(path, flags, mode);\n> +\t} while (ret < 0 && errno == EINTR);\n> +\n> +\treturn ret;\n> +}\n\nThanks.\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 838246289c..551cc9f22f 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -788,6 +788,12 @@ int git_vsnprintf(char *str, size_t maxsize,\n> \t\t const char *format, va_list ap);\n> #endif\n> \n> +#ifdef OPEN_RETURNS_EINTR\n> +#undef open\n> +#define open git_open_with_retry\n> +int git_open_with_retry(const char *path, int flag, ...);\n> +#endif\n> +\n> #ifdef __GLIBC_PREREQ\n> #if __GLIBC_PREREQ(2, 1)\n> #define HAVE_STRCHRNUL\n"},{"id":"418017","messageId":"YDy0C9sRvboGXQ7P@coredump.intra.peff.net","threadId":"55202","inReplyTo":"xmqq8s7ascby.fsf@gitster.g","subject":"[PATCH] config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-01T09:29:47Z","receivedAt":"2021-03-01T09:30:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 26, 2021 at 02:17:53PM -0800, Junio C Hamano wrote:\n\n> > +\t/*\n> > +\t * Also O_TMPFILE would take a mode, but it isn't defined everywhere.\n> > +\t * And anyway, we don't use it in our code base.\n> > +\t */\n> \n> That is being extra careful---I like it very much.\n\nI wondered what would happen if my \"anyway\" above is wrong. We at least\nwould not invoke undefined behavior (because we'd avoid looking at the\nmode parameter even though it exists), but would pass a \"0\" mode to the\nreal open(). Presumably somebody would notice that. :)\n\n> > +\tif (flags & O_CREAT) {\n> > +\t\tva_list ap;\n> > +\t\tva_start(ap, flags);\n> > +\t\tmode = va_arg(ap, int);\n> > +\t\tva_end(ap);\n> > +\t}\n> > +\n> > +\tdo {\n> > +\t\tret = open(path, flags, mode);\n> > +\t} while (ret < 0 && errno == EINTR);\n> > +\n> > +\treturn ret;\n> > +}\n> \n> Thanks.\n\nI got another off-list report of the problem. I think we probably want\nto do this on top:\n\n-- >8 --\nSubject: config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur\n\nWe've had mixed reports on whether the latest release of macOS needs\nthis Makefile knob set. In most reported cases, there's antivirus\nsoftware running (which one might imagine could cause an open() call to\nbe delayed). However, one of the (off-list) reports I've gotten\nindicated that it happened on an otherwise clean install of Big Sur.\n\nSince the symptom is so bad (checkout randomly fails to write several\nfails when the progress meter kicks in), and since the workaround is so\nlightweight (if we don't see EINTR, it's just an extra conditional\ncheck), let's just turn it on by default.\n\nSigned-off-by: Jeff King \n---\nApparently Big Sur jumped from macOS 10.x to 11.x. But our \"uname -r\"\ncheck gives the \"Darwin version\", in which it is 20.x (following 19.x\nfor the previous version). At least according to some sources I found\nonline. :) So that is good, because otherwise all of our uname_R checks\nhere would have been broken. I don't have a Big Sur machine handy to\ntest with, but I believe this should do what we want.\n\n config.mak.uname | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex e22d4b6d67..d204c20a64 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -124,6 +124,9 @@ ifeq ($(uname_S),Darwin)\n \tifeq ($(shell test \"`expr \"$(uname_R)\" : '\\([0-9][0-9]*\\)\\.'`\" -ge 11 && echo 1),1)\n \t\tHAVE_GETDELIM = YesPlease\n \tendif\n+\tifeq ($(shell test \"`expr \"$(uname_R)\" : '\\([0-9][0-9]*\\)\\.'`\" -ge 20 && echo 1),1)\n+\t\tOPEN_RETURNS_EINTR = UnfortunatelyYes\n+\tendif\n \tNO_MEMMEM = YesPlease\n \tUSE_ST_TIMESPEC = YesPlease\n \tHAVE_DEV_TTY = YesPlease\n-- \n2.31.0.rc0.521.g56be7fa5e1\n\n"},{"id":"418047","messageId":"xmqqk0qqssiu.fsf@gitster.c.googlers.com","threadId":"55202","inReplyTo":"YDy0C9sRvboGXQ7P@coredump.intra.peff.net","subject":"Re: [PATCH] config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T17:17:13Z","receivedAt":"2021-03-01T17:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I got another off-list report of the problem. I think we probably want\n> to do this on top:\n>\n> -- >8 --\n> Subject: config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur\n>\n> We've had mixed reports on whether the latest release of macOS needs\n> this Makefile knob set. In most reported cases, there's antivirus\n> software running (which one might imagine could cause an open() call to\n> be delayed). However, one of the (off-list) reports I've gotten\n> indicated that it happened on an otherwise clean install of Big Sur.\n>\n> Since the symptom is so bad (checkout randomly fails to write several\n> fails when the progress meter kicks in), and since the workaround is so\n> lightweight (if we don't see EINTR, it's just an extra conditional\n> check), let's just turn it on by default.\n>\n> Signed-off-by: Jeff King \n> ---\n> Apparently Big Sur jumped from macOS 10.x to 11.x. But our \"uname -r\"\n> check gives the \"Darwin version\", in which it is 20.x (following 19.x\n> for the previous version). At least according to some sources I found\n> online. :) So that is good, because otherwise all of our uname_R checks\n> here would have been broken. I don't have a Big Sur machine handy to\n> test with, but I believe this should do what we want.\n\nThanks.\n\n> config.mak.uname | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/config.mak.uname b/config.mak.uname\n> index e22d4b6d67..d204c20a64 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -124,6 +124,9 @@ ifeq ($(uname_S),Darwin)\n> \tifeq ($(shell test \"`expr \"$(uname_R)\" : '\\([0-9][0-9]*\\)\\.'`\" -ge 11 && echo 1),1)\n> \t\tHAVE_GETDELIM = YesPlease\n> \tendif\n> +\tifeq ($(shell test \"`expr \"$(uname_R)\" : '\\([0-9][0-9]*\\)\\.'`\" -ge 20 && echo 1),1)\n> +\t\tOPEN_RETURNS_EINTR = UnfortunatelyYes\n> +\tendif\n> \tNO_MEMMEM = YesPlease\n> \tUSE_ST_TIMESPEC = YesPlease\n> \tHAVE_DEV_TTY = YesPlease\n"},{"id":"418091","messageId":"xmqqczwimnps.fsf@gitster.c.googlers.com","threadId":"55202","inReplyTo":"YDy0C9sRvboGXQ7P@coredump.intra.peff.net","subject":"Re: [PATCH] config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T23:57:35Z","receivedAt":"2021-03-02T15:16:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I got another off-list report of the problem. I think we probably want\n> to do this on top:\n\nQueued and pushed out.\n\nI wonder if these hits for SA_RESTART in config.mak.uname would be a\ngood way to guide us.\n\n [6c109904 (Port to HP NonStop, 2012-09-19)]\n # Not detected (nor checked for) by './configure'.\n # We don't have SA_RESTART on NonStop, unfortunalety.\n COMPAT_CFLAGS += -DSA_RESTART=0\n\n [40036bed (Port to QNX, 2012-12-18)]\n ifeq ($(uname_S),QNX)\n COMPAT_CFLAGS += -DSA_RESTART=0\n\nOne caveat is that we do not know if their system headers hide the\nreal implementation of open(2) behind a C preprocessor macro, in\nwhcih case OPEN_RETURNS_EINTR wrapper may not work correctly.\n\n> config.mak.uname | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/config.mak.uname b/config.mak.uname\n> index e22d4b6d67..d204c20a64 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -124,6 +124,9 @@ ifeq ($(uname_S),Darwin)\n> \tifeq ($(shell test \"`expr \"$(uname_R)\" : '\\([0-9][0-9]*\\)\\.'`\" -ge 11 && echo 1),1)\n> \t\tHAVE_GETDELIM = YesPlease\n> \tendif\n> +\tifeq ($(shell test \"`expr \"$(uname_R)\" : '\\([0-9][0-9]*\\)\\.'`\" -ge 20 && echo 1),1)\n> +\t\tOPEN_RETURNS_EINTR = UnfortunatelyYes\n> +\tendif\n> \tNO_MEMMEM = YesPlease\n> \tUSE_ST_TIMESPEC = YesPlease\n> \tHAVE_DEV_TTY = YesPlease\n"},{"id":"418157","messageId":"YD+SCtzmtWgFArwW@coredump.intra.peff.net","threadId":"55202","inReplyTo":"xmqqczwimnps.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-03T13:41:30Z","receivedAt":"2021-03-04T00:23:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 01, 2021 at 03:57:35PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I got another off-list report of the problem. I think we probably want\n> > to do this on top:\n> \n> Queued and pushed out.\n\nThanks. I guess we're a bit late to make it into the upcoming release.\nCertainly we have survived for many years without this particular\nbugfix, so in that sense it is not urgent. But I do wonder if we will\nsee more reports as more people start using the new macOS release. So it\nmight be good to keep in mind for maint, if we cut a minor release.\n\nOr alternatively, we could include _just_ the first patch. That's low\nrisk, since you have to enable to knob yourself, but it gives people an\noption if they run into the symptom. But even that is probably not that\nurgent. People can also cherry-pick the patch, after all (and a\ndistributor like homebrew can probably include the patch in their recipe\nif need be).\n\n> I wonder if these hits for SA_RESTART in config.mak.uname would be a\n> good way to guide us.\n> \n> [6c109904 (Port to HP NonStop, 2012-09-19)]\n> # Not detected (nor checked for) by './configure'.\n> # We don't have SA_RESTART on NonStop, unfortunalety.\n> COMPAT_CFLAGS += -DSA_RESTART=0\n> \n> [40036bed (Port to QNX, 2012-12-18)]\n> ifeq ($(uname_S),QNX)\n> COMPAT_CFLAGS += -DSA_RESTART=0\n\nI'm inclined to leave them alone until somebody with access to such a\nsystem can look further into it. After all, if you do not have\nSA_RESTART, you might not even have EINTR in the first place.\n\n> One caveat is that we do not know if their system headers hide the\n> real implementation of open(2) behind a C preprocessor macro, in\n> whcih case OPEN_RETURNS_EINTR wrapper may not work correctly.\n\nYeah. I didn't think about that when I did the original \"just do it\neverywhere\" patch. But that is exactly what caused the problem on\nWindows (not a system macro, but in fact our own!). So I'm glad to have\nbacked it off to a Makefile knob.\n\n-Peff\n"},{"id":"418192","messageId":"xmqqv9a7g2xz.fsf@gitster.c.googlers.com","threadId":"55202","inReplyTo":"YD+SCtzmtWgFArwW@coredump.intra.peff.net","subject":"Re: [PATCH] config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T00:47:20Z","receivedAt":"2021-03-04T01:09:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Thanks. I guess we're a bit late to make it into the upcoming release.\n> Certainly we have survived for many years without this particular\n> bugfix, so in that sense it is not urgent. But I do wonder if we will\n> see more reports as more people start using the new macOS release. So it\n> might be good to keep in mind for maint, if we cut a minor release.\n>\n> Or alternatively, we could include _just_ the first patch. That's low\n> risk, since you have to enable to knob yourself, but it gives people an\n> option if they run into the symptom. But even that is probably not that\n> urgent. People can also cherry-pick the patch, after all (and a\n> distributor like homebrew can probably include the patch in their recipe\n> if need be).\n\nTrue. The topic is forked at somewhere mergeable to 'maint' so\ndistro folks who are interested can merge them down later. I think\nit is small and safe enough to merge them both down to the upcoming\nrelease, though.\n\n"}]}