Re: PATCH: Configurable file mode mask

On Tue, Mar 06, 2018 at 01:32:49PM -0500, David Steele wrote:
> On 3/5/18 10:46 PM, Michael Paquier wrote:
> Ah, I see what you mean now. Fixed using follow_fast. This variant
> claims to be faster and it doesn't matter if files are occasionally
> checked twice.

Fine for me. I can see this option present in perl 5.8.8 as well, so we
should be good.

>> Those two are separate issues. Could you begin a new thread on the
>> matter? This will attract more attention.
>
> OK, I'll move it back for now, and make a note to raise the position as
> a separate issue. I'd rather not do it in this fest, though.

Seems like you forgot to re-add the chmod calls in initdb.c.

>> - if (chmod(location, S_IRWXU) != 0)
>> + current_umask = umask(0);
>> + umask(current_umask);
>> [...]
>> - if (chmod(pg_data, S_IRWXU) != 0)
>> + if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
>> Such modifications should be part of patch 3, not 2, where the group
>> features really apply.
>
> The goal of patch 2 is to refactor the way permissions are set by
> replacing locally hard-coded values with centralized values, so I think
> this makes sense here.

Hm. I would have left that out in this first version, now I am fine to
defer that to a committer's opinion as well.

>> my $test_mode = shift;
>>
>> + umask(0077);
>
> Added. (Ensure that all files are created with default data dir
> permissions).

+ # Ensure that all files are created with default data dir permissions
+ umask(0077);
I have stared at this comment two minutes to finally understand that
this refers to the extra files which are part of this test. It may be
better to be a bit more precise here. I thought first that this
referred as well to setup_cluster calls...

>> Patch 2 begins to look fine for me.
>
> I also made chmod_recursive() generic.

Likely this name is not worth worrying with conflicts in CPAN stuff ;)

+# Ensure all permissions in the pg_data directory are
correct. Permissions
+# should be dir = 0700, file = 0600.
+sub check_pg_data_perm
+{
check_permission_recursive() would be a more adapted name. Stuff in
TestLib.pm is not necessarily linked to data folders.

sub clean_rewind_test
{
+ ok (check_pg_data_perm($node_master->data_dir(), 0));
+
$node_master->teardown_node if defined $node_master;
I have also a pending patch for pg_rewind which adds read-only files in
the data folders of a new test, so this would cause this part to blow
up. Testing that for all the test sets does not bring additional value
as well, and doing it at cleanup phase is also confusing. So could you
move that check into only one test's script? You could remove the umask
call in 003_extrafiles.pl as well this way, and reduce the patch diffs.

+ if ($file_mode != 0600)
+ {
+ print(*STDERR, "$File::Find::name mode must be 0600\n");
+
+ $result = 0;
+ return
+ }
0600 should be replaced by $expected_file_perm, and isn't a ';' missing
for each return "clause" (how does this even work..)?

Patch 2 is getting close for a committer lookup I think, still need to
look at patch 3 in details.. And from patch 3:
# Expected permission
- my $expected_file_perm = 0600;
- my $expected_dir_perm = 0700;
+ my $expected_file_perm = $allow_group ? 0640 : 0600;
+ my $expected_dir_perm = $allow_group ? 0750 : 0700;
Or $expected_file_perm and $expected_dir_perm could just be used as
arguments.
--
Michael