On Wed, Oct 23, 2024 at 06:17:15PM +0200, Kristoffer Haugsbakk wrote:
> 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.
> > 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.
> > + };
> > + 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
