git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH 1/4] Introduce a function to lock/unlock file descriptors when appending
Date: Thu, 09 Aug 2018 10:35:25 -0700 (PDT)	[thread overview]
Message-ID: <e449ed75fe3705692175017f98438815aeccf0fb.1533836122.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.17.git.gitgitgadget@gmail.com>

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This function will be used to make write accesses in trace_write() a bit
safer.

Note: this patch takes a very different approach for cross-platform
support than Git is historically taking: the original approach is to
first implement everything on Linux, using the functions available on
Linux, and then trying to emulate these functions on platforms that do
not have those functions such as Windows.

This leads to all kinds of undesirable quirks in Git's source code (and
performance characteristics) because of the lack of a proper abstraction
layer: instead of declaring functions for the functionality we
*actually* need, we abuse POSIX functions to say what we need, even if
those functions serve much broader purposes (and do not make at all
clear what the caller wants of them).

For example, when Git wants to determine whether a path refers to a
symbolic link, it calls lstat(). That POSIX function has to be emulated
on Windows, painstakingly filling all the information lstat() would,
only for the caller to throw away everything except that one bit of
information, and all of the time figuring out the mtime/atime/ctime and
file size and whatnot was spent in vain.

If we were to follow that approach, we would extend the fcntl()
emulation in compat/mingw.c after this commit, to support the function
added in this commit.

But fcntl() allows a lot more versatile file region locking that we
actually need, to by necessity the fcntl() emulation would be quite
complex: To support the `l_whence = SEEK_CUR` (which we would use, if it
did not require so much book-keeping due to our writing between locking
and unlocking the file), we would have to call `SetFilePointerEx()`
(after obtaining the `HANDLE` on which all Win32 API functions work
instead of the integer file descriptors used by all POSIX functions).
Multiplying the number of Win32 API round-trips. Of course, we could
implement an incomplete emulation of fcntl()'s F_WRLCK, but that would
be unsatisfying.

An alternative approach would be to use the `flock()` function, whose
semantics are much closer to existing Win32 API. But `flock()` is not
even a POSIX standard, so we would have to implement emulation of
`flock()` for *other* platforms. And it would again be the wrong thing
to do, as we would once again fail to have an abstraction that clearly
says what *exact*functionality we want to use.

To set a precedent for a better approach, let's introduce a proper
abstraction: a function that says in its name precisely what Git
wants it to do (as opposed to *how* it does it on Linux):
lock_or_unlock_fd_for_appending().

The next commit will provide a Windows-specific implementation of this
function/functionality.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

squash! Introduce a function to lock/unlock file descriptors when appending
---
 git-compat-util.h |  2 ++
 wrapper.c         | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 9a64998b2..13b83bade 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define getc_unlocked(fh) getc(fh)
 #endif
 
+extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);
+
 /*
  * Our code often opens a path to an optional file, to work on its
  * contents when we can successfully open it.  We can ignore a failure
diff --git a/wrapper.c b/wrapper.c
index e4fa9d84c..6c2116272 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -690,3 +690,17 @@ int xgethostname(char *buf, size_t len)
 		buf[len - 1] = 0;
 	return ret;
 }
+
+#ifndef GIT_WINDOWS_NATIVE
+int lock_or_unlock_fd_for_appending(int fd, int lock_it)
+{
+	struct flock flock;
+
+	flock.l_type = lock_it ? F_WRLCK : F_UNLCK;
+	flock.l_whence = SEEK_SET;
+	flock.l_start = 0;
+	flock.l_len = 0xffffffff; /* arbitrary number of bytes */
+
+	return fcntl(fd, F_SETLKW, &flock);
+}
+#endif
-- 
gitgitgadget


  reply	other threads:[~2018-08-09 17:35 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-09 17:35 [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE Johannes Schindelin via GitGitGadget
2018-08-09 17:35 ` Johannes Schindelin via GitGitGadget [this message]
2018-08-09 19:01   ` [PATCH 1/4] Introduce a function to lock/unlock file descriptors when appending Junio C Hamano
2018-08-10 18:32     ` Johannes Schindelin
2018-08-09 17:35 ` [PATCH 2/4] mingw: implement lock_or_unlock_fd_for_appending() Johannes Schindelin via GitGitGadget
2018-08-09 17:35 ` [PATCH 3/4] trace: lock the trace file to avoid racy trace_write() calls Johannes Schindelin via GitGitGadget
2018-08-09 17:35 ` [PATCH 4/4] trace: verify that locking works Johannes Schindelin via GitGitGadget
2018-08-09 19:47 ` [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE Jeff King
2018-08-09 20:49   ` Junio C Hamano
2018-08-09 21:08     ` Junio C Hamano
2018-08-09 21:32       ` Jeff King
2018-08-10 14:09     ` Jeff King
2018-08-10 15:58       ` Junio C Hamano
2018-08-10 15:58       ` Junio C Hamano
2018-08-10 16:43       ` Johannes Schindelin
2018-08-10 17:15         ` Jeff King
2018-08-10 18:40           ` Junio C Hamano
2018-08-10 19:34           ` Johannes Schindelin
2018-08-10 16:15 ` Johannes Sixt
2018-08-10 16:51   ` Jeff Hostetler
2018-08-10 16:57     ` Jeff Hostetler
2018-08-10 17:08     ` Johannes Sixt
2018-08-10 18:34   ` Junio C Hamano
2018-08-10 18:56     ` Jeff King
2018-08-13 19:02     ` [PATCH] mingw: enable atomic O_APPEND Johannes Sixt
2018-08-13 20:20       ` Junio C Hamano
2018-08-13 21:05         ` Johannes Sixt
2018-08-13 21:22           ` Ævar Arnfjörð Bjarmason
2018-08-13 21:55             ` Junio C Hamano
2018-08-13 22:37             ` Jeff King
2018-08-14 13:47               ` Ævar Arnfjörð Bjarmason
2018-08-14 14:53                 ` Jeff King
2018-08-14 18:29               ` Johannes Sixt
2018-08-14 19:17                 ` Jeff King
2018-08-14 13:01       ` Jeff Hostetler
2018-08-14 14:38         ` Junio C Hamano
2018-08-10 19:47 ` [PATCH v2 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE Johannes Schindelin via GitGitGadget
2018-08-10 19:47   ` [PATCH v2 1/4] Introduce a function to lock/unlock file descriptors when appending Johannes Schindelin via GitGitGadget
2018-08-10 20:05     ` Junio C Hamano
2018-08-10 21:31       ` Johannes Schindelin
2018-08-10 19:47   ` [PATCH v2 2/4] mingw: implement lock_or_unlock_fd_for_appending() Johannes Schindelin via GitGitGadget
2018-08-10 19:47   ` [PATCH v2 3/4] trace: lock the trace file to avoid racy trace_write() calls Johannes Schindelin via GitGitGadget
2018-08-10 19:47   ` [PATCH v2 4/4] trace: verify that locking works Johannes Schindelin via GitGitGadget

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=e449ed75fe3705692175017f98438815aeccf0fb.1533836122.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).