Re: PATCH: Configurable file mode mask

David,

* David Steele (david@pgmasters.net) wrote:
> On 1/8/18 8:58 PM, Peter Eisentraut wrote:
> > On 1/3/18 08:11, Robert Haas wrote:
> >> On Tue, Jan 2, 2018 at 11:43 AM, David Steele <david@pgmasters.net> wrote:
> >>>>> I think MakeDirectory() is a good wrapper, but isn't
> >>>> MakeDirectoryPerm() sort of silly?
> >>>
> >>> There's one place in the backend (storage/ipc/ipc.c) that sets non-default
> >>> directory permissions.  This function is intended to support that and any
> >>> extensions that need to set custom perms.
> >>
> >> Yeah, but all it does is call mkdir(), which could just as well be
> >> called directly.  I think there's a pointer to a wrapper when it does
> >> something for you -- supply an argument, log something, handle
> >> portability concerns -- but this wrapper does exactly nothing.
> > 
> > Yeah, I didn't like this aspect when this patch was originally
> > submitted.  We want to keep the code legible for future new
> > contributors.  Having these generic-sounding but specific-in-purpose
> > wrapper functions can be pretty confusing.  Let's use mkdir() when it's
> > the appropriate function, and let's figure out a different name for
> > "make a data directory subdirectory in a secure and robust way".
> 
> I think there is value to keeping the function names symmetric to the
> FileOpen()/FileOpenPerm() variants but I'm clearly in the minority so
> there's no point in pursuing it.
> 
> How about MakeDirectoryDefaultPerm()?  That's what I'll go with if I
> don't hear any other ideas.  The single call to MakeDirectoryPerm() will
> be reverted to mkdir() and I'll remove the function.

I would be sure to include a good comment/explanation about why mkdir()
is being used there and not MakeDirectoryDefaultPerm() (unless one
exists already), just to avoid later hackers thinking that mkdir() is
the way to go in general when, in most cases, MakeDirectoryDefaultPerm()
should be used.

Thanks!

Stephen