On Sun, Oct 27, 2024 at 02:17:36PM +0100, Johannes Sixt wrote:
> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index e326c6fcd2d..6c75fe36b15 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -532,6 +532,62 @@ 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;
>
> I would have made this variable DWORD or unsigned instead of plain int,
> because it receives a bit pattern and would match the parameter type of
> CreateFileW() better; but no big deal.
I have to reroll this series anyway, so I'll just fix this while at it.
> > + int fd;
> > +
> > + /* We only support basic flags. */
> > + if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
> > + 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;
>
> O_RDWR, O_WRONLY and O_RDONLY are not flags, but values occupying two
> bits of oflags. This must be:
>
> if ((oflags & O_ACCMODE) == O_RDWR)
> access = GENERIC_READ | GENERIC_WRITE;
> else if ((oflags & O_ACCMODE) == O_WRONLY)
> access = GENERIC_WRITE;
> else
> access = GENERIC_READ;
>
> or similar.
Ah, that makes sense indeed. Will fix.
> > +
> > + 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;
>
> First I wondered what this is about, but then noticed that it is just
> copied from the mingw_open_append() implementation catering to some
> bogus network filesystems. Good.
I'll add a comment.
Patrick
