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?
> 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/ ?
> implementing that mode would be more complex due to the required
> permission handling. Furthermore, atomic updates via renames typically
> write to exclusive lockfile and then perform the rename, and thus we
> don't have to handle the case where the locked path has been created
> with `O_CREATE`. So while it would be nice to have proper POSIX
> semantics in all paths, we instead aim for a minimum viable fix here.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index e326c6fcd2d..ce95f3a5968 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -532,6 +532,59 @@ static int mingw_open_append(wchar_t const
> *wfilename, int oflags, ...)
> return fd;
> }
>
> +/*
> + * Ideally, we'd use `_wopen()` to implement this functionality so that we
> + * don't have to reimplement it, but unfortunately we do not have tight control
> + * over the share mode there. And while `_wsopen()` and friends exist that give
> + * us _some_ control over the share mode, this family of functions doesn't give
> + * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict
> + * requirement for us though so that we can unlink or rename over files that
> + * are held open by another process.
> + *
> + * We are thus forced to implement our own emulation of `open()`. To make our
> + * life simpler we only implement basic support for this, namely opening
> + * existing files for reading and/or writing. This means that newly created
> + * files won't have their sharing mode set up correctly, but for now I couldn't
> + * find any case where this matters. We may have to revisit that in the future
> + * though based on our needs.
> + */
This is above my technical level but the comment reads nicely.
> +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
> +{
> + SECURITY_ATTRIBUTES security_attributes = {
> + .nLength = sizeof(security_attributes),
> + .bInheritHandle = !(oflags & O_NOINHERIT),
`clang-format` thinks that these two lines should be indented with tabs
instead (so two tabs in total).
(Ubuntu clang-format version 14.0.0-1ubuntu1.1)
> + };
> + 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; }
```
> + if (oflags & O_RDWR)
> + access = GENERIC_READ | GENERIC_WRITE;
> + else if (oflags & O_WRONLY)
> + access = GENERIC_WRITE;
> + else
> + access = GENERIC_READ;
> +
> + handle = CreateFileW(filename, access,
> + FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> + &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
`clang-format` says that these two lines are too long.
> + if (handle == INVALID_HANDLE_VALUE) {
> + DWORD err = GetLastError();
> + if (err == ERROR_INVALID_PARAMETER)
> + err = ERROR_PATH_NOT_FOUND;
> + errno = err_win_to_posix(err);
> + return -1;
Here you don’t use the comma operator, presumably because it wouldn’t
turn the `if` into a one-statement block.
> + }
> +
> + fd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);
> + if (fd < 0)
> + CloseHandle(handle);
> + return fd;
> +}
> +
> /*
> * Does the pathname map to the local named pipe filesystem?
> * That is, does it have a "//./pipe/" prefix?
> @@ -567,6 +620,8 @@ int mingw_open (const char *filename, int oflags, ...)
>
> if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
> open_fn = mingw_open_append;
> + else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
> + open_fn = mingw_open_existing;
> else
> open_fn = _wopen;
>
> --
> 2.47.0.118.gfd3785337b.dirty
