Re: PATCH: Configurable file mode mask

Hi Michael,

On 4/5/18 2:55 AM, Michael Paquier wrote:
> On Wed, Apr 04, 2018 at 08:03:54PM -0400, David Steele wrote:
>
>> Instead I have created variables in file_perm.c
>> that hold the current file create mode, dir create mode, and mode mask.
>> All call sites use those variables (e.g. pg_dir_create_mode), which I
>> think are much clear.
>
> Hm. I personally find that even more confusing, especially with
> SetDataDirectoryCreatePerm which basically is the same as
> SetConfigOption for the backend.

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.

> In your case pg_dir_create_mode is not
> aimed at remaining a constant as you may change it depending on if
> grouping is enabled or not... And all the other iterations of such
> variables in src/common/ are constants (see NumScanKeywords or
> forkNames[] for example), so I cannot see with a good eye this change.

The idea is to have a variable that give the fir dir/file create mode
without having to trust that umask() is set correctly or do mode masking
on chmod(). This makes a number of call sites simpler and, I hope,
easier to read.

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

What we don't want is a regression in the current, default behavior.
This is design is intended to avoid that outcome.

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

Thanks!
--
-David
david(at)pgmasters(dot)net