Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files

[Patrick Steinhardt <ps@pks.im>]

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