On 8/29/17 12:15, David Steele wrote: > While working on the patch to allow group reads of $PGDATA I refactored > the various OpenFile() functions to use default/global permissions > rather than permissions defined in each call. > > I think the patch stands on its own regardless of whether we accept the > patch to allow group permissions (which won't make this CF). We had a > couple different ways of defining permissions (e.g. 0600 vs S_IRUSR | > S_IWUSR) and in any case it represented quite a bit of duplication. > This way seems simpler and less error-prone.
Yeah, it would be good to clean some of that up. I wonder whether we even need that much flexibility. We already set a global umask, so we could just open all files with 0666 and let the umask sort it out. Then we don't need all the *Perm() variants. I don't like the function-like macros in fd.h. We can use proper functions. I also wonder whether the umask save-and-restore code in copy.c and be-fsstubs.c is sound. If the AllocateFile() call in between errors out, then there is nothing that restores the original umask. This might need a TRY/CATCH block, or maybe just a chmod() afterwards. The mkdir() calls could perhaps use some further refactoring so you don't have to specify the mode everywhere. This kind of code: - if (stat_buf.st_mode & (S_IRWXG | S_IRWXO)) + if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT) ereport(FATAL, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("data directory \"%s\" has group or world access", DataDir), - errdetail("Permissions should be u=rwx (0700)."))); + errdetail("Permissions should be (%04o).", PG_DIR_MODE_DEFAULT))); can be problematic, because we are hoping that PG_MODE_MASK_DEFAULT, PG_DIR_MODE_DEFAULT, and the wording in the error message can stay consistent. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers