[PATCH] D78896: [Support] Add file lock/unlock functions

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked 5 inline comments as done.
sepavloff added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1140-1141
+/// @param File    The descriptor representing the file to lock.
+/// @param Timeout Time in milliseconds that the process should wait before
+///                reporting lock failure.
+/// @returns errc::success if lock is successfully obtained,
----------------
labath wrote:
> this would be nicer as a std::chrono type.
Agree.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1145
+/// error_code otherwise.
+std::error_code lockFile(int FD, unsigned Timeout = 0);
+
----------------
labath wrote:
> Should this be called `tryLockFile`, or maybe even `tryLockFileFor` (to mirror e.g. [[ https://en.cppreference.com/w/cpp/thread/timed_mutex/try_lock_for | std::timed_mutex::try_lock_for ]]) ?
Make sense. Changed to `tryLockFile`.


================
Comment at: llvm/unittests/Support/Path.cpp:1938-1939
+#ifdef _WIN32
+// Windows refuses lock request if file region is already locked by the same
+// process. POSIX system in this case updates the existing lock.
+TEST_F(FileSystemTest, lockFile) {
----------------
labath wrote:
> Have you considered using flock(2) instead of F_SETLK? That might give you semantics which are a bit saner and a bit closer to what happens on windows (though file locking is always weird on posix systems)...
IIUC, `flock` is not a POSIX call. GLIBC implements it on top of `fcntl`. The implementation also contains vague statement that it represents different mechanism on 4BSD: https://github.com/bminor/glibc/blob/92954ffa5a5662fbfde14febd7e5dcc358c85470/sysdeps/posix/flock.c#L18 . So I would refrain from using it, as the code must work on Linux, MacOS and *BSD. POSIX calls looks more portable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78896/new/

https://reviews.llvm.org/D78896