* [PATCH 0/3] another round of minor updates
@ 2016-02-28 11:28 Eric Wong
2016-02-28 11:28 ` [PATCH 1/3] t/: remove unnecessary Dumper use Eric Wong
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Eric Wong @ 2016-02-28 11:28 UTC (permalink / raw)
To: meta
Another round of tiny tweaks to clean up our code and
possibly reduce bugs/errors. There's possibly a Xapian
corruption bug from running t/search.t related to
closing the wrong descriptor or forking,
but I haven't been able to verify it.
This is with libsearch-xapian-perl on Debian jessie x86-64,
(1.2.19.0-1), not my usual wheezy installs which I'm
slowly upgrading...
lib/PublicInbox/Config.pm | 1 -
lib/PublicInbox/Feed.pm | 2 --
lib/PublicInbox/Git.pm | 10 ++--------
lib/PublicInbox/ProcessPipe.pm | 5 ++---
lib/PublicInbox/SearchIdx.pm | 4 +---
lib/PublicInbox/Spawn.pm | 1 -
t/nntpd.t | 1 -
t/search.t | 1 -
8 files changed, 5 insertions(+), 20 deletions(-)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/3] t/: remove unnecessary Dumper use
2016-02-28 11:28 [PATCH 0/3] another round of minor updates Eric Wong
@ 2016-02-28 11:28 ` Eric Wong
2016-02-28 11:28 ` [PATCH 2/3] searchidx: use defined for checking EOF behavior Eric Wong
2016-02-28 11:28 ` [PATCH 3/3] reduce calls to close unless error checks are needed Eric Wong
2 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2016-02-28 11:28 UTC (permalink / raw)
To: meta
No point in loading Data::Dumper if we do not use it
in the tests.
---
t/nntpd.t | 1 -
t/search.t | 1 -
2 files changed, 2 deletions(-)
diff --git a/t/nntpd.t b/t/nntpd.t
index f6f71a2..d8c2e7b 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -17,7 +17,6 @@ use Socket qw(SO_KEEPALIVE IPPROTO_TCP TCP_NODELAY);
use File::Temp qw/tempdir/;
use Net::NNTP;
use IPC::Run qw(run);
-use Data::Dumper;
my $tmpdir = tempdir(CLEANUP => 1);
my $home = "$tmpdir/pi-home";
diff --git a/t/search.t b/t/search.t
index cd7048f..3ec3f30 100644
--- a/t/search.t
+++ b/t/search.t
@@ -7,7 +7,6 @@ eval { require PublicInbox::SearchIdx; };
plan skip_all => "Xapian missing for search" if $@;
use File::Temp qw/tempdir/;
use Email::MIME;
-use Data::Dumper;
my $tmpdir = tempdir(CLEANUP => 1);
my $git_dir = "$tmpdir/a.git";
my ($root_id, $last_id);
--
EW
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/3] searchidx: use defined for checking EOF behavior
2016-02-28 11:28 [PATCH 0/3] another round of minor updates Eric Wong
2016-02-28 11:28 ` [PATCH 1/3] t/: remove unnecessary Dumper use Eric Wong
@ 2016-02-28 11:28 ` Eric Wong
2016-02-28 11:28 ` [PATCH 3/3] reduce calls to close unless error checks are needed Eric Wong
2 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2016-02-28 11:28 UTC (permalink / raw)
To: meta
While empty or "0" should never appear, this allows the
reviewer to think and know less about the context in which
this check is done.
---
lib/PublicInbox/SearchIdx.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index 6727299..1d0d926 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -337,7 +337,7 @@ sub rlog {
--raw -r --no-abbrev/, $range);
my $latest;
my $bytes;
- while (my $line = <$log>) {
+ while (defined(my $line = <$log>)) {
if ($line =~ /$addmsg/o) {
my $mime = do_cat_mail($git, $1, \$bytes) or next;
$add_cb->($self, $git, $mime, $bytes);
--
EW
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 3/3] reduce calls to close unless error checks are needed
2016-02-28 11:28 [PATCH 0/3] another round of minor updates Eric Wong
2016-02-28 11:28 ` [PATCH 1/3] t/: remove unnecessary Dumper use Eric Wong
2016-02-28 11:28 ` [PATCH 2/3] searchidx: use defined for checking EOF behavior Eric Wong
@ 2016-02-28 11:28 ` Eric Wong
2 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2016-02-28 11:28 UTC (permalink / raw)
To: meta
We can rely on timely auto-destruction based on reference
counting; reducing the chance of redundant close(2) calls
which may hit the wront FD.
We do care about certain close calls (e.g. writing to a buffered
IO handle) if we require error-checking for write-integrity. In
other cases, let things go out-of-scope so it can be freed
automatically after use.
---
lib/PublicInbox/Config.pm | 1 -
lib/PublicInbox/Feed.pm | 2 --
lib/PublicInbox/Git.pm | 10 ++--------
lib/PublicInbox/ProcessPipe.pm | 5 ++---
lib/PublicInbox/SearchIdx.pm | 2 --
lib/PublicInbox/Spawn.pm | 1 -
6 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 844f666..b511638 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -101,7 +101,6 @@ sub try_cat {
if (open(my $fh, '<', $path)) {
local $/;
$rv = <$fh>;
- close $fh;
}
$rv;
}
diff --git a/lib/PublicInbox/Feed.pm b/lib/PublicInbox/Feed.pm
index a5828a8..54cbf23 100644
--- a/lib/PublicInbox/Feed.pm
+++ b/lib/PublicInbox/Feed.pm
@@ -255,7 +255,6 @@ sub each_recent_blob {
}
}
- close $log; # we may EPIPE here
# for pagination
($first_commit, $last_commit);
}
@@ -269,7 +268,6 @@ sub get_feedopts {
my %rv;
if (open my $fh, '<', "$ctx->{git_dir}/description") {
chomp($rv{description} = <$fh>);
- close $fh;
} else {
$rv{description} = '($GIT_DIR/description missing)';
}
diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 57d17d3..0f92dd9 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -29,8 +29,6 @@ sub _bidi_pipe {
my @cmd = ('git', "--git-dir=$self->{git_dir}", qw(cat-file), $batch);
my $redir = { 0 => fileno($out_r), 1 => fileno($in_w) };
$self->{$pid} = spawn(\@cmd, undef, $redir);
- close $out_r or fail($self, "close failed: $!");
- close $in_w or fail($self, "close failed: $!");
$out_w->autoflush(1);
$self->{$out} = $out_w;
$self->{$in} = $in_r;
@@ -100,13 +98,9 @@ sub check {
sub _destroy {
my ($self, $in, $out, $pid) = @_;
- my $p = $self->{$pid} or return;
- $self->{$pid} = undef;
+ my $p = delete $self->{$pid} or return;
foreach my $f ($in, $out) {
- my $fh = $self->{$f};
- defined $fh or next;
- close $fh;
- $self->{$f} = undef;
+ delete $self->{$f};
}
waitpid $p, 0;
}
diff --git a/lib/PublicInbox/ProcessPipe.pm b/lib/PublicInbox/ProcessPipe.pm
index eade524..e088c10 100644
--- a/lib/PublicInbox/ProcessPipe.pm
+++ b/lib/PublicInbox/ProcessPipe.pm
@@ -15,13 +15,12 @@ sub READ { sysread($_[0]->{fh}, $_[1], $_[2], $_[3] || 0) }
sub READLINE { readline($_[0]->{fh}) }
-sub CLOSE { close($_[0]->{fh}) }
+sub CLOSE { delete($_[0]->{fh}) }
sub FILENO { fileno($_[0]->{fh}) }
sub DESTROY {
- my $fh = delete($_[0]->{fh});
- close $fh if $fh;
+ delete($_[0]->{fh});
waitpid($_[0]->{pid}, 0);
}
diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index 1d0d926..415decd 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -348,7 +348,6 @@ sub rlog {
$latest = $1;
}
}
- close $log;
$latest;
}
@@ -446,7 +445,6 @@ sub _read_git_config_perm {
my @cmd = qw(config core.sharedRepository);
my $fh = PublicInbox::Git->new($self->{git_dir})->popen(@cmd);
my $perm = <$fh>;
- close $fh;
chomp $perm if defined $perm;
$perm;
}
diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index 51ad269..02c5446 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -171,7 +171,6 @@ sub popen_rd {
$r->blocking($blocking) if defined $blocking;
$opts->{1} = fileno($w);
my $pid = spawn($cmd, $env, $opts);
- close $w;
return ($r, $pid) if wantarray;
my $ret = gensym;
tie *$ret, 'PublicInbox::ProcessPipe', $pid, $r;
--
EW
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-28 11:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-28 11:28 [PATCH 0/3] another round of minor updates Eric Wong
2016-02-28 11:28 ` [PATCH 1/3] t/: remove unnecessary Dumper use Eric Wong
2016-02-28 11:28 ` [PATCH 2/3] searchidx: use defined for checking EOF behavior Eric Wong
2016-02-28 11:28 ` [PATCH 3/3] reduce calls to close unless error checks are needed Eric Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).