8000 Activate perlcritic InputOutput::RequireCheckedSyscalls and fix resul… · postgrespro/postgres@d56cb42 · GitHub
[go: up one dir, main page]
Skip to content

Commit d56cb42

Browse files
peterepetere
committed
Activate perlcritic InputOutput::RequireCheckedSyscalls and fix resulting warnings
This checks that certain I/O-related Perl functions properly check their return value. Some parts of the PostgreSQL code had been a bit sloppy about that. The new perlcritic warnings are fixed here. I didn't design any beautiful error messages, mostly just used "or die $!", which mostly matches existing code, and also this is developer-level code, so having the system error plus source code reference should be ok. Initially, we only activate this check for a subset of what the perlcritic check would warn about. The effective list is chmod flock open read rename seek symlink system The initial set of functions is picked because most existing code already checked the return value of those, so any omissions are probably unintended, or because it seems important for test correctness. The actual perlcritic configuration is written as an exclude list. That seems better so that we are clear on what we are currently not checking. Maybe future patches want to investigate checking some of the other functions. (In principle, we might eventually want to check all of them, but since this is test and build support code, not production code, there are probably some reasonable compromises to be made.) Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/flat/88b7d4f2-46d9-4cc7-b1f7-613c90f9a76a%40eisentraut.org
1 parent bb5604b commit d56cb42

File tree

15 files changed

+52
-43
lines changed

15 files changed

+52
-43
lines changed

‎src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl

Lines changed: 1 addition & 1 deletion
57AE
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ sub create_files
3636
{
3737
foreach my $fn (map { $_->{name} } @_)
3838
{
39-
open my $file, '>', "$tempdir/$fn";
39+
open my $file, '>', "$tempdir/$fn" or die $!;
4040

4141
print $file 'CONTENT';
4242
close $file;

‎src/bin/pg_basebackup/t/010_pg_basebackup.pl

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@
7777
ok(-d "$tempdir/backup", 'backup directory was created and left behind');
7878
rmtree("$tempdir/backup");
7979

80-
open my $conf, '>>', "$pgdata/postgresql.conf";
80+
open my $conf, '>>', "$pgdata/postgresql.conf" or die $!;
8181
print $conf "max_replication_slots = 10\n";
8282
print $conf "max_wal_senders = 10\n";
8383
print $conf "wal_level = replica\n";
@@ -175,7 +175,7 @@
175175
qw(backup_label tablespace_map postgresql.auto.conf.tmp
176176
current_logfiles.tmp global/pg_internal.init.123))
177177
{
178-
open my $file, '>>', "$pgdata/$filename";
178+
open my $file, '>>', "$pgdata/$filename" or die $!;
179179
print $file "DONOTCOPY";
180180
close $file;
181181
}
@@ -185,7 +185,7 @@
185185
# unintended side effects.
186186
if ($Config{osname} ne 'darwin')
187187
{
188-
open my $file, '>>', "$pgdata/.DS_Store";
188+
open my $file, '>>', "$pgdata/.DS_Store" or die $!;
189189
print $file "DONOTCOPY";
190190
close $file;
191191
}
@@ -423,7 +423,7 @@
423423
my $tblspcoid = $1;
424424
my $escapedRepTsDir = $realRepTsDir;
425425
$escapedRepTsDir =~ s/\\/\\\\/g;
426-
open my $mapfile, '>', $node2->data_dir . '/tablespace_map';
426+
open my $mapfile, '>', $node2->data_dir . '/tablespace_map' or die $!;
427427
print $mapfile "$tblspcoid $escapedRepTsDir\n";
428428
close $mapfile;
429429

‎src/bin/pg_ctl/t/001_start_stop.pl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
command_ok([ $ENV{PG_REGRESS}, '--config-auth', "$tempdir/data" ],
2424
'configure authentication');
2525
my $node_port = PostgreSQL::Test::Cluster::get_free_port();
26-
open my $conf, '>>', "$tempdir/data/postgresql.conf";
26+
open my $conf, '>>', "$tempdir/data/postgresql.conf" or die $!;
2727
print $conf "fsync = off\n";
2828
print $conf "port = $node_port\n";
2929
print $conf PostgreSQL::Test::Utils::slurp_file($ENV{TEMP_CONFIG})

‎src/bin/pg_resetwal/t/002_corrupted.pl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
my $data;
2222
open my $fh, '<', $pg_control or BAIL_OUT($!);
2323
binmode $fh;
24-
read $fh, $data, 16;
24+
read $fh, $data, 16 or die $!;
2525
close $fh;
2626

2727
# Fill pg_control with zeros

‎src/bin/pg_rewind/t/009_growing_files.pl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@
6969
# Extract the last line from the verbose output as that should have the error
7070
# message for the unexpected file size
7171
my $last;
72-
open my $f, '<', "$standby_pgdata/tst_both_dir/file1";
72+
open my $f, '<', "$standby_pgdata/tst_both_dir/file1" or die $!;
7373
$last = $_ while (<$f>);
7474
close $f;
7575
like($last, qr/error: size of source file/, "Check error message");

‎src/bin/pg_rewind/t/RewindTest.pm

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -311,8 +311,8 @@ sub run_pg_rewind
311311
# Make sure that directories have the right umask as this is
312312
# required by a follow-up check on permissions, and better
313313
# safe than sorry.
314-
chmod(0700, $node_primary->archive_dir);
315-
chmod(0700, $node_primary->data_dir . "/pg_wal");
314+
chmod(0700, $node_primary->archive_dir) or die $!;
315+
chmod(0700, $node_primary->data_dir . "/pg_wal") or die $!;
316316

317317
# Add appropriate restore_command to the target cluster
318318
$node_primary->enable_restoring($node_primary, 0);

‎src/pl/plperl/text2macro.pl

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,11 +88,11 @@ sub selftest
8888
close $fh;
8989

9090
system("perl $0 --name=X $tmp.pl > $tmp.c") == 0 or die;
91-
open $fh, '>>', "$tmp.c";
91+
open $fh, '>>', "$tmp.c" or die;
9292
print $fh "#include <stdio.h>\n";
9393
print $fh "int main() { puts(X); return 0; }\n";
9494
close $fh;
95-
system("cat -n $tmp.c");
95+
system("cat -n $tmp.c") == 0 or die;
9696

9797
system("make $tmp") == 0 or die;
9898
open $fh, '<', "./$tmp |" or die;

‎src/test/kerberos/t/001_auth.pl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@
111111
# Construct a pgpass file to make sure we don't use it
112112
append_to_file($pgpass, '*:*:*:*:abc123');
113113

114-
chmod 0600, $pgpass;
114+
chmod 0600, $pgpass or die $!;
115115

116116
# Build the krb5.conf to use.
117117
#

‎src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
# install certificate and protected key
3434
copy("server.crt", $ddir);
3535
copy("server.key", $ddir);
36-
chmod 0600, "$ddir/server.key";
36+
chmod 0600, "$ddir/server.key" or die $!;
3737

3838
$node->start;
3939

‎src/test/perl/PostgreSQL/Test/Cluster.pm

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@ sub set_replication_conf
467467
$self->host eq $test_pghost
468468
or croak "set_replication_conf only works with the default host";
469469

470-
open my $hba, '>>', "$pgdata/pg_hba.conf";
470+
open my $hba, '>>', "$pgdata/pg_hba.conf" or die $!;
471471
print $hba
472472
"\n# Allow replication (set up by PostgreSQL::Test::Cluster.pm)\n";
473473
if ($PostgreSQL::Test::Utils::windows_os
@@ -580,7 +580,7 @@ sub init
580580
PostgreSQL::Test::Utils::system_or_bail($ENV{PG_REGRESS},
581581
'--config-auth', $pgdata, @{ $params{auth_extra} });
582582

583-
open my $conf, '>>', "$pgdata/postgresql.conf";
583+
open my $conf, '>>', "$pgdata/postgresql.conf" or die $!;
584584
print $conf "\n# Added by PostgreSQL::Test::Cluster.pm\n";
585585
print $conf "fsync = off\n";
586586
print $conf "restart_after_crash = off\n";
@@ -862,7 +862,7 @@ sub init_from_backup
862862
rmdir($data_path);
863863
PostgreSQL::Test::RecursiveCopy::copypath($backup_path, $data_path);
864864
}
865-
chmod(0700, $data_path);
865+
chmod(0700, $data_path) or die $!;
866866

867867
# Base configuration for this node
868868
$self->append_conf(
@@ -1688,16 +1688,16 @@ sub _reserve_port
16881688
if (kill 0, $pid)
16891689
{
16901690
# process exists and is owned by us, so we can't reserve this port
1691-
flock($portfile, LOCK_UN);
1691+
flock($portfile, LOCK_UN) || die $!;
16921692
close($portfile);
16931693
return 0;
16941694
}
16951695
}
16961696
# All good, go ahead and reserve the port
1697-
seek($portfile, 0, SEEK_SET);
1697+
seek($portfile, 0, SEEK_SET) || die $!;
16981698
# print the pid with a fixed width so we don't leave any trailing junk
16991699
print $portfile sprintf("%10d\n", $$);
1700-
flock($portfile, LOCK_UN);
1700+
flock($portfile, LOCK_UN) || die $!;
17011701
close($portfile);
17021702
push(@port_reservation_files, $filename);
17031703
return 1;

0 commit comments

Comments
 (0)
0