On Wed, Oct 23, 2024 at 06:17:15PM +0200, Kristoffer Haugsbakk wrote:

On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:

quoted

On Windows, we emulate open(3p) via `mingw_open()`. This function
implements handling of some platform- specific quirks that are required

s/platform- specific/platform-specific/

Linewrapping artifact?

Looks like it. Thanks for spotting.

quoted

to make it behave as closely as possible like open(3p) would, but for
most cases we just call the Windows-specific `_wopen()` function.

This function has a major downside though: it does not allow us to
specify the sharing mode. While there is `_wsopen()` that allows us to
pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`
flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows
concurrent read- and write-access, but does not allow for concurrent
deletions. Unfortunately though, we have to allow concurrent deletions
if we want to have POSIX-style atomic renames on top of an existing file
that has open file handles.

Implement a new function that emulates open(3p) for existing files via
`CreateFileW()` such that we can set the required sharing flags.

While we have the same issue when calling open(3p) with `O_CREAT`,

s/O_CREAT/O_CREATE/ ?

No, O_CREAT is the flag name.

quoted

+	};
+	HANDLE handle;
+	int access;
+	int fd;
+
+	/* We only support basic flags. */
+	if (oflags & ~(O_ACCMODE | O_NOINHERIT))
+		return errno = ENOSYS, -1;

This use of the comma operator is maybe an idiom to save space and avoid
a brace around the `if`?  This pattern is already in use in
`mingw_open_append`.  I see in `mingw.h` that it uses:

static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)
{ errno = ENOSYS; return -1; }

Yeah. What Patrick wrote here is not technically wrong, but I would not
consider it in the style of the rest of the project. Perhaps
compat/mingw-stuff is a bit of a wild west, but I think it would be
match the rest of the project's conventions here.

I actually think from grepping around that mingw_open_append is the only
other function in the tree that uses the "return errno = XXX, -1;"
trick. It might be nice to keep it that way, and/or rewrite that portion
like so:

--- 8< ---
diff --git a/compat/mingw.c b/compat/mingw.c
index df78f61f7f..c36147549a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -494,8 +494,10 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	DWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;

 	/* only these flags are supported */
-	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
-		return errno = ENOSYS, -1;
+	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {
+		errno = ENOSYS;
+		return -1;
+	}

 	/*
 	 * FILE_SHARE_WRITE is required to permit child processes
--- >8 ---

Thanks,
Taylor