Johannes Sixt <j6t@xxxxxxxx> writes:
> The Windows CRT implements O_APPEND "manually": on write() calls, the
> file pointer is set to EOF before the data is written. Clearly, this is
> not atomic. And in fact, this is the root cause of failures observed in
> t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where
> different processes write to the same trace file simultanously; it also
> occurred in t5400-send-pack.sh, but there it was worked around in
> 71406ed4d6 ("t5400: avoid concurrent writes into a trace file",
> 2017-05-18).
>
> Fortunately, Windows does support atomic O_APPEND semantics using the
> file access mode FILE_APPEND_DATA. Provide an implementation that does.
>
> This implementation is minimal in such a way that it only implements
> the open modes that are actually used in the Git code base. Emulation
> for other modes can be added as necessary later. To become aware of
> the necessity early, the unusal error ENOSYS is reported if an
> unsupported mode is encountered.
>
> Diagnosed-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
> Helped-by: Jeff Hostetler <git@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Johannes Sixt <j6t@xxxxxxxx>
> ---
> compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 39 insertions(+), 2 deletions(-)
Nice.
I wonder how much more expensive using this implementation is
compared with the original "race susceptible" open(), when raciness
is known not to be an issue (e.g. there is higher level lock that
protects the appending).
If it turns out to be quite more costly, I do not think we'd mind
introducing a thin wrapper
#ifndef race_safe_append_open
#ifndef race_safe_append_open(fn) open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666)
#endif
in git-compat-util.h after it includes "compat/mingw.h" and replace
the call to open(... O_APPEND ...) in trace.c::get_trace_fd() with a
call to that wrapper. That way, other codepaths that use O_APPEND
(namely, reflog and todo-list writers) can avoid the additional
cost, if any.
Some may find it beneficial from code readability POV because that
approach marks the codepath that needs to have non-racy fd more
explicitly.
I am assuming that in this case other users of O_APPEND are not
performance critical, so hopefully the following is only theoretical
and not necessary.
git-compat-util.h | 5 +++++
trace.c | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 9a64998b24..a981c50800 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -218,6 +218,11 @@
#else
#include <stdint.h>
#endif
+
+#ifndef race_safe_append_open
+#define race_safe_append_open(fn) open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666)
+#endif
+
#ifdef NO_INTPTR_T
/*
* On I16LP32, ILP32 and LP64 "long" is the save bet, however
diff --git a/trace.c b/trace.c
index fc623e91fd..bef9cee1da 100644
--- a/trace.c
+++ b/trace.c
@@ -47,7 +47,7 @@ static int get_trace_fd(struct trace_key *key)
else if (strlen(trace) == 1 && isdigit(*trace))
key->fd = atoi(trace);
else if (is_absolute_path(trace)) {
- int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
+ int fd = race_safe_append_open(trace);
if (fd == -1) {
warning("could not open '%s' for tracing: %s",
trace, strerror(errno));
