Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 23, 2024, 17:30 UTC
Message-ID: <ZxkysCb/d3+PJjW4@nand.local>
In-Reply-To: <bc829d84-6696-4e16-9687-8ba611af29ff@app.fastmail.com>

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

Show 7 quoted lines

> On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:
> > 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.

Show 18 quoted lines

> > 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.

Show 18 quoted lines

> > +	};
> > +	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