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

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 24, 2024, 06:30 UTC
Message-ID: <ZxnpnLbETN2-DZsq@pks.im>
In-Reply-To: <bc829d84-6696-4e16-9687-8ba611af29ff@app.fastmail.com>

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`.

Show 10 quoted lines

> > +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.

Show 17 quoted lines

> > +	};
> > +	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.

Show 12 quoted lines

> > +	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