Re: [PATCH 1/2] compat/mingw: handle O_CLOEXEC in `mingw_open_existing()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 13, 2025, 17:52 UTC
- Message-ID
- <xmqqo6y4om26.fsf@gitster.g>
- In-Reply-To
- <20250313-b4-pks-mingw-lockfile-flake-v1-1-bc5d3e70f516@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 23 quoted lines
> Our MinGW emulation of the open(3p) syscall uses one of three different > code paths depending on the flags passed by the caller. Ideally, we > would just use `_wopen()` for all of these directly and instead rely on > the Windows SDK to implement the logic for us. But unfortunately, this > interface does not allow us to set the `FILE_SHARING_*` flags, which we > need to have control over to implement POSIX semantics. > > One of the code paths is for opening existing files, where we end up > calling `mingw_open_existing()`. While this code path is executed when > the user passes `O_NOINHERIT`, we don't know to handle `O_CLOEXEC` yet, > which causes a couple of code paths that use the flag to not use the > emulation. The consequence is that those code paths do not support POSIX > semantics because we don't know to set the sharing mode correctly. > > Supporting `O_CLOEXEC` is quite trivial: we don't have to do anything, > as Windows already closes the file handle by default when exec'ing into > another process. This is further supported by the fact that we indeed > define `O_CLOEXEC` as `O_NOINHERIT` in case the former isn't defined in > "compat/mingw.h". > > Adapt the code so that we know to handle `O_CLOEXEC` in case it has a > different definition than `O_NOINHERIT` to improve our POSIX semantics > handling.
This one looks quite sensible and straight-forward even to a non-Windows person like me.
Show 27 quoted lines
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> compat/mingw.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index f524c54d06d..101e380c5a3 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -560,7 +560,7 @@ static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
> int fd;
>
> /* We only support basic flags. */
> - if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
> + if (oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)) {
> errno = ENOSYS;
> return -1;
> }
> @@ -632,7 +632,7 @@ 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)))
> + else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT | O_CLOEXEC)))
> open_fn = mingw_open_existing;
> else
> open_fn = _wopen;