David,
* David Steele (david@pgmasters.net) wrote:
> The GUC shows the current mode of the data directory, while the
> variables in file_perm.c store the mode that should be used to create
> new dirs/files. One is certainly based on the other but I thought it
> best to split them for clarity.
Agreed. I did have some other comments about the patches tho-
> > You also forgot a call to SetDataDirectoryCreatePerm or
> > pg_dir_create_mode remains to its default.
>
> Are saying *if* a call is forgotten?
>
> Yes, the file/dir create modes will use the default restrictive
> permissions unless set otherwise. I see this as a good thing. Worst
> case, we miss some areas where group access should be enabled and we
> find those over the next few months.
I tend to agree with this, though the space of concern is quite limited-
basically between the top of PostmasterMain() and when it calls
checkDataDir(), and we really shouldn't be creating any files before
we've check that the data dir looks sane.
> > The interface of file_perm.h that you are introducing is not confusing
> > anymore though..
>
> Yes, that was the idea. I think it makes it clearer what we are trying
> to do and centralizes variables related to create modes in one place.
>
> I've attached new patches that exclude Windows from permissions tests
> and deal with bootstrap permissions in a better way. PostgresMain() and
> AuxiliaryProcessMain() now use checkDataDir() to set permissions.
Alright, changes I've made, since I got impatient and it didn't seem to
make sense to bounce these back to David instead of just making them (I
did discuss them with him on the phone today tho, just to be clear).
- MakeDirectoryDefaultPerm() was quite a mouthful, so I've simplified
that down to MakePGDirectory(), which I hope is clear in that it's for
making directories related to PG (as it isn't a general mkdir() call
or just some platform-agnostic mkdir(), which MakeDirectory() might
imply, but is specific for working with PG data directories).
- The PG_FILE_MODE_DEFAULT, PG_DIR_MODE_DEFAULT, et al, were confusing.
Those constants are used for the default file/dir mode, sure, but what
that actually *are* is the 'owner'-only style mode, so they've been
changed to PG_FILE_MODE_OWNER, PG_DIR_MODE_OWNER, etc.
- Where we're intentionally ignoring the MakePGDirectory() result,
use (void).
- Various comment improvements.
- Improved the commit messages a bit.
Things that still need doing:
- Go back over with pgindent and make sure it's all happy.
- Improved documentation
- Likely more comments (I don't think I can ever have enough)
- Further discussion in the commit messages
- Perhaps a bit more review to try to minimize the risk that something
breaks on Windows... Looked ok to me, but probably still some risk
that the Windows buildfarm members fall over, though I suppose that's
also what they're there for.
Updated patch attached.
David, if you could look through this again and make sure I didn't break
anything with the changes made, and perhaps make improvements to the
docs/comments/commit messages, that'd be helpful for getting this over
the line.
Thanks!
Stephen