| Message ID | 3552feddb332b31744c2ab723b112b9b08926436.1729695349.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | compat/mingw: implement POSIX-style atomic renames | expand |
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@pks.im> > --- > 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
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
On Wed, Oct 23, 2024 at 05:05:00PM +0200, Patrick Steinhardt wrote: > compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) Very well reasoned and explained. I am certainly not a Windows expert, but from what I read the changes look reasonable to me. Thanks, Taylor
On Wed, Oct 23, 2024 at 06:17:15PM +0200, Kristoffer Haugsbakk wrote: > On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote: > > While we have the same issue when calling open(3p) with `O_CREAT`, > > s/O_CREAT/O_CREATE/ ? Yeah, you'd think that, but no, it's really `O_CREAT`. > > +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) And they indeed should be. > > + }; > > + 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; } > ``` I basically copied this from other code, but don't care strongly about it either way. > > + 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. Might be. But the `FILE_SHARE_*` line cannot really be any shorter without hurting readability. And the second line just follows the first line then. Patrick
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. + */ +static int mingw_open_existing(const wchar_t *filename, int oflags, ...) +{ + SECURITY_ATTRIBUTES security_attributes = { + .nLength = sizeof(security_attributes), + .bInheritHandle = !(oflags & O_NOINHERIT), + }; + HANDLE handle; + int access; + int fd; + + /* We only support basic flags. */ + if (oflags & ~(O_ACCMODE | O_NOINHERIT)) + return errno = ENOSYS, -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); + 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; + } + + 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;
On Windows, we emulate open(3p) via `mingw_open()`. This function implements handling of some platform- specific quirks that are required 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`, 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@pks.im> --- compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)