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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 27, 2024, 23:48 UTC
Message-ID: <Zx7RU6sgIJjNSutE@nand.local>
In-Reply-To: <Zx5eipv_Un3x1j-4@pks.im>

On Sun, Oct 27, 2024 at 04:38:50PM +0100, Patrick Steinhardt wrote:

Show 28 quoted lines

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

It may be nice to write this as a switch statement, since we're always comparing the value of oflags & O_ACCMODE, like so:

    switch (oflags & O_ACCMODE) {
    case O_RDWR:
        access = GENERIC_READ | GENERIC_WRITE;
        break;
    case O_WRONLY:
        access = GENERIC_WRITE;
        break;
    default:
        access = GENERIC_READ;
        break;
    }

, but it is a minor point and I certainly do not have very strong feelings here.

Thanks, Taylor