[PATCH 00/10] first steps towards eliminating TIEHANDLE

[PATCH 00/10] first steps towards eliminating TIEHANDLE

[Eric Wong]

ProcessIO being tied is problematic since perlops like stat
and fcntl don't work directly on the handle, and we need
separate classes to do buffered read and unbuffered sysread.

It's possible to do as common packages (e.g. File::Temp,
IO::Socket::IP, IO::Socket::SSL) do by subclassing IO::Handle
to avoid tie completely.  I also want to avoid the self-tie
thing IO::Socket::SSL to avoid having to rely on weaken().

So, this essentially becomes a war on `close($fh)' an relying
on $fh->close where appropriate.  This loses some usefulness
of autodie, so we keep `close($fh)' for places which aren't
tied at the moment.

One key place is to replace many calls to popen_rd with the new
run_qx to consolidate close calls.  There's also few other
cleanups, code reductions, and safety improvements along the way.

And more changes coming.

Eric Wong (10):
  spawn: croak directly in C pi_fork_exec
  spawnpp: use autodie for syscall failures
  spawn: avoid alloca in C pi_fork_exec
  git: use run_qx to read `git --version'
  git: avoid extra stat(2) for git version
  gcf2: simplify pkg-config and Inline::C setup
  treewide: use run_qx where appropriate
  www_altid: reduce FD pressure in qspawn queues
  xt/eml_check_limits: remove useless import
  lei_ale: use v5.12, autodie, and try_cat

 lib/PublicInbox/Admin.pm           |  7 ++---
 lib/PublicInbox/Config.pm          | 14 ++++-----
 lib/PublicInbox/Fetch.pm           |  7 ++---
 lib/PublicInbox/Gcf2.pm            | 27 +++++-----------
 lib/PublicInbox/Git.pm             | 15 +++++----
 lib/PublicInbox/Import.pm          |  6 ++--
 lib/PublicInbox/LEI.pm             |  7 ++---
 lib/PublicInbox/LeiALE.pm          | 24 +++++++--------
 lib/PublicInbox/LeiBlob.pm         | 12 +++-----
 lib/PublicInbox/LeiMirror.pm       |  7 ++---
 lib/PublicInbox/MultiGit.pm        |  7 ++---
 lib/PublicInbox/Spamcheck/Spamc.pm | 11 ++-----
 lib/PublicInbox/Spawn.pm           | 49 +++++++++++++++---------------
 lib/PublicInbox/SpawnPP.pm         | 10 +++---
 lib/PublicInbox/WwwAltId.pm        |  9 ++----
 lib/PublicInbox/XapHelperCxx.pm    | 11 ++-----
 t/solver_git.t                     | 12 +++-----
 xt/eml_check_limits.t              |  4 +--
 18 files changed, 94 insertions(+), 145 deletions(-)

[PATCH 01/10] spawn: croak directly in C pi_fork_exec

[Eric Wong]

This saves us some Perl code in the wrapper, since the SpawnPP
implementation also dies directly.
---
 lib/PublicInbox/Spawn.pm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index 1fa7a41f..5740ee58 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -89,7 +89,7 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	AV *env = (AV *)SvRV(envref);
 	AV *rlim = (AV *)SvRV(rlimref);
 	const char *filename = SvPV_nolen(file);
-	pid_t pid;
+	pid_t pid = -1;
 	char **argv, **envp;
 	sigset_t set, old;
 	int ret, perrnum;
@@ -100,17 +100,17 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	AV2C_COPY(argv, cmd);
 	AV2C_COPY(envp, env);
 
-	if (sigfillset(&set)) return -1;
-	if (sigdelset(&set, SIGABRT)) return -1;
-	if (sigdelset(&set, SIGBUS)) return -1;
-	if (sigdelset(&set, SIGFPE)) return -1;
-	if (sigdelset(&set, SIGILL)) return -1;
-	if (sigdelset(&set, SIGSEGV)) return -1;
+	if (sigfillset(&set)) goto out;
+	if (sigdelset(&set, SIGABRT)) goto out;
+	if (sigdelset(&set, SIGBUS)) goto out;
+	if (sigdelset(&set, SIGFPE)) goto out;
+	if (sigdelset(&set, SIGILL)) goto out;
+	if (sigdelset(&set, SIGSEGV)) goto out;
 	/* no XCPU/XFSZ here */
-	if (sigprocmask(SIG_SETMASK, &set, &old)) return -1;
+	if (sigprocmask(SIG_SETMASK, &set, &old)) goto out;
 	chld_is_member = sigismember(&old, SIGCHLD);
-	if (chld_is_member < 0) return -1;
-	if (chld_is_member > 0 && sigdelset(&old, SIGCHLD)) return -1;
+	if (chld_is_member < 0) goto out;
+	if (chld_is_member > 0 && sigdelset(&old, SIGCHLD)) goto out;
 
 	pid = vfork();
 	if (pid == 0) {
@@ -171,6 +171,8 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	} else if (perrnum) {
 		errno = perrnum;
 	}
+out:
+	if (pid < 0) croak("E: fork_exec %s: %s\n", filename, strerror(errno));
 	return (int)pid;
 }
 
@@ -369,9 +371,7 @@ sub spawn ($;$$) {
 	}
 	my $cd = $opt->{'-C'} // ''; # undef => NULL mapping doesn't work?
 	my $pgid = $opt->{pgid} // -1;
-	my $pid = pi_fork_exec(\@rdr, $f, $cmd, \@env, $rlim, $cd, $pgid);
-	die "fork_exec @$cmd failed: $!\n" unless $pid > 0;
-	$pid;
+	pi_fork_exec(\@rdr, $f, $cmd, \@env, $rlim, $cd, $pgid);
 }
 
 sub popen_rd {

[PATCH 02/10] spawnpp: use autodie for syscall failures

[Eric Wong]

We lose a little info for fork failures, but I don't think it
matters.
---
 lib/PublicInbox/SpawnPP.pm | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/SpawnPP.pm b/lib/PublicInbox/SpawnPP.pm
index e7174d6f..f89d37d4 100644
--- a/lib/PublicInbox/SpawnPP.pm
+++ b/lib/PublicInbox/SpawnPP.pm
@@ -7,6 +7,7 @@
 package PublicInbox::SpawnPP;
 use v5.12;
 use POSIX qw(dup2 _exit setpgid :signal_h);
+use autodie qw(chdir close fork pipe);
 # this is loaded by PublicInbox::Spawn, so we can't use/require it, here
 
 # Pure Perl implementation for folks that do not use Inline::C
@@ -20,9 +21,8 @@ sub pi_fork_exec ($$$$$$$) {
 		$set->delset($_) or die "delset($_): $!";
 	}
 	sigprocmask(SIG_SETMASK, $set, $old) or die "SIG_SETMASK(set): $!";
-	my $syserr;
-	pipe(my ($r, $w)) or die "pipe: $!";
-	my $pid = fork // die "fork (+exec) @$cmd: $!\n";
+	pipe(my $r, my $w);
+	my $pid = fork;
 	if ($pid == 0) {
 		close $r;
 		$SIG{__DIE__} = sub {
@@ -40,9 +40,7 @@ sub pi_fork_exec ($$$$$$$) {
 			die "setpgid(0, $pgid): $!";
 		}
 		$SIG{$_} = 'DEFAULT' for grep(!/\A__/, keys %SIG);
-		if ($cd ne '') {
-			chdir $cd or die "cd $cd: $!";
-		}
+		chdir($cd) if $cd ne '';
 		while (@$rlim) {
 			my ($r, $soft, $hard) = splice(@$rlim, 0, 3);
 			BSD::Resource::setrlimit($r, $soft, $hard) or

[PATCH 03/10] spawn: avoid alloca in C pi_fork_exec

[Eric Wong]

We don't have thread-safety to worry about, so just leave a few
allocations at process exit at worst.  We'll also update some
comments about usage while we're at it.
---
 lib/PublicInbox/Spawn.pm | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index 5740ee58..8382c4d2 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -6,10 +6,8 @@
 # is explicitly defined in the environment (and writable).
 # Under Linux, vfork can make a big difference in spawning performance
 # as process size increases (fork still needs to mark pages for CoW use).
-# Currently, we only use this for code intended for long running
-# daemons (inside the PSGI code (-httpd) and -nntpd).  The short-lived
-# scripts (-mda, -index, -learn, -init) either use IPC::run or standard
-# Perl routines.
+# None of this is intended to be thread-safe since Perl5 maintainers
+# officially discourage the use of threads.
 #
 # There'll probably be more OS-level C stuff here, down the line.
 # We don't want too many DSOs: https://udrepper.livejournal.com/8790.html
@@ -39,12 +37,6 @@ BEGIN {
 #include <time.h>
 #include <stdio.h>
 #include <string.h>
-
-/* some platforms need alloca.h, but some don't */
-#if defined(__GNUC__) && !defined(alloca)
-#  define alloca(sz) __builtin_alloca(sz)
-#endif
-
 #include <signal.h>
 #include <assert.h>
 
@@ -56,11 +48,17 @@ BEGIN {
  *   This is unlike "sv_len", which returns what you would expect.
  */
 #define AV2C_COPY(dst, src) do { \
+	static size_t dst##__capa; \
 	I32 i; \
 	I32 top_index = av_len(src); \
 	I32 real_len = top_index + 1; \
 	I32 capa = real_len + 1; \
-	dst = alloca(capa * sizeof(char *)); \
+	if (capa > dst##__capa) { \
+		dst##__capa = 0; /* in case Newx croaks */ \
+		Safefree(dst); \
+		Newx(dst, capa, char *); \
+		dst##__capa = capa; \
+	} \
 	for (i = 0; i < real_len; i++) { \
 		SV **sv = av_fetch(src, i, 0); \
 		dst[i] = SvPV_nolen(*sv); \
@@ -90,7 +88,7 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	AV *rlim = (AV *)SvRV(rlimref);
 	const char *filename = SvPV_nolen(file);
 	pid_t pid = -1;
-	char **argv, **envp;
+	static char **argv, **envp;
 	sigset_t set, old;
 	int ret, perrnum;
 	volatile int cerrnum = 0; /* shared due to vfork */
@@ -172,7 +170,8 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 		errno = perrnum;
 	}
 out:
-	if (pid < 0) croak("E: fork_exec %s: %s\n", filename, strerror(errno));
+	if (pid < 0)
+		croak("E: fork_exec %s: %s\n", filename, strerror(errno));
 	return (int)pid;
 }
 

[PATCH 04/10] git: use run_qx to read `git --version'

[Eric Wong]

It exists, now, so save us a few lines of code.
---
 lib/PublicInbox/Git.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index f4a24f2a..b5adc1f4 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -18,7 +18,7 @@ use Errno qw(EINTR EAGAIN);
 use File::Glob qw(bsd_glob GLOB_NOSORT);
 use File::Spec ();
 use Time::HiRes qw(stat);
-use PublicInbox::Spawn qw(spawn popen_rd which);
+use PublicInbox::Spawn qw(spawn popen_rd run_qx which);
 use PublicInbox::ProcessIONBF;
 use PublicInbox::Tmpfile;
 use IO::Poll qw(POLLIN);
@@ -61,9 +61,8 @@ sub check_git_exe () {
 	my @st = stat($GIT_EXE) or die "stat($GIT_EXE): $!";
 	my $st = pack('dd', $st[10], $st[7]);
 	if ($st ne $EXE_ST) {
-		my $rd = popen_rd([ $GIT_EXE, '--version' ]);
-		my $v = readline($rd);
-		CORE::close($rd) or die "$GIT_EXE --version: $?";
+		my $v = run_qx([ $GIT_EXE, '--version' ]);
+		die "$GIT_EXE --version: \$?=$?" if $?;
 		$v =~ /\b([0-9]+(?:\.[0-9]+){2})/ or die
 			"$GIT_EXE --version output: $v # unparseable";
 		$GIT_VER = eval("v$1") // die "BUG: bad vstring: $1 ($v)";

[PATCH 05/10] git: avoid extra stat(2) for git version

[Eric Wong]

No sane installer will update executable files in place due to
ETXTBSY on execve.  So save ourselves a stat(2) call by relying
on the special `CORE::stat(_)' case to reuse the cached result
from the `-x FILE' filetest in which().
---
 lib/PublicInbox/Git.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index b5adc1f4..a1d52118 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -17,7 +17,6 @@ use PublicInbox::Syscall qw(EPOLLIN EPOLLET);
 use Errno qw(EINTR EAGAIN);
 use File::Glob qw(bsd_glob GLOB_NOSORT);
 use File::Spec ();
-use Time::HiRes qw(stat);
 use PublicInbox::Spawn qw(spawn popen_rd run_qx which);
 use PublicInbox::ProcessIONBF;
 use PublicInbox::Tmpfile;
@@ -53,13 +52,13 @@ my %GIT_ESC = (
 );
 my %ESC_GIT = map { $GIT_ESC{$_} => $_ } keys %GIT_ESC;
 
-my $EXE_ST = ''; # pack('dd', st_ctime, st_size);
+my $EXE_ST = ''; # pack('dd', st_dev, st_ino); # no `q' in some 32-bit builds
 my ($GIT_EXE, $GIT_VER);
 
 sub check_git_exe () {
 	$GIT_EXE = which('git') // die "git not found in $ENV{PATH}";
-	my @st = stat($GIT_EXE) or die "stat($GIT_EXE): $!";
-	my $st = pack('dd', $st[10], $st[7]);
+	my @st = stat(_) or die "stat($GIT_EXE): $!"; # can't do HiRes w/ _
+	my $st = pack('dd', $st[0], $st[1]);
 	if ($st ne $EXE_ST) {
 		my $v = run_qx([ $GIT_EXE, '--version' ]);
 		die "$GIT_EXE --version: \$?=$?" if $?;
@@ -117,6 +116,7 @@ sub git_path ($$) {
 sub alternates_changed {
 	my ($self) = @_;
 	my $alt = git_path($self, 'objects/info/alternates');
+	use Time::HiRes qw(stat);
 	my @st = stat($alt) or return 0;
 
 	# can't rely on 'q' on some 32-bit builds, but `d' works

[PATCH 06/10] gcf2: simplify pkg-config and Inline::C setup

[Eric Wong]

We can use run_qx and try_cat to make the build setup simpler.
---
 lib/PublicInbox/Gcf2.pm | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/lib/PublicInbox/Gcf2.pm b/lib/PublicInbox/Gcf2.pm
index 4f163cde..502bf33a 100644
--- a/lib/PublicInbox/Gcf2.pm
+++ b/lib/PublicInbox/Gcf2.pm
@@ -5,11 +5,11 @@
 # other libgit2 stuff may go here, too.
 package PublicInbox::Gcf2;
 use v5.12;
-use PublicInbox::Spawn qw(which popen_rd); # may set PERL_INLINE_DIRECTORY
+use PublicInbox::Spawn qw(which run_qx); # may set PERL_INLINE_DIRECTORY
 use Fcntl qw(SEEK_SET);
 use Time::HiRes qw(clock_gettime CLOCK_MONOTONIC);
 use IO::Handle; # autoflush
-use PublicInbox::Git qw(read_all);
+use PublicInbox::Git;
 use PublicInbox::Lock;
 
 BEGIN {
@@ -27,29 +27,16 @@ BEGIN {
 	my $pc = which($ENV{PKG_CONFIG} // 'pkg-config') //
 		die "pkg-config missing for libgit2";
 	my ($dir) = (__FILE__ =~ m!\A(.+?)/[^/]+\z!);
-	open my $err, '+>', "$inline_dir/.public-inbox.pkg-config.err";
 	my $vals = {};
-	my $rdr = { 2 => $err };
+	my $rdr = { 2 => \(my $err) };
 	my @switches = qw(modversion cflags libs);
 	for my $k (@switches) {
-		my $rd = popen_rd([$pc, "--$k", 'libgit2'], undef, $rdr);
-		chomp(my $val = do { local $/; <$rd> });
-		CORE::close($rd) or last; # checks for error and sets $?
+		chomp(my $val = run_qx([$pc, "--$k", 'libgit2'], undef, $rdr));
+		die "E: libgit2 not installed: $err\n" if $?;
 		$vals->{$k} = $val;
 	}
-	if (!$?) {
-		# note: we name C source files .h to prevent
-		# ExtUtils::MakeMaker from automatically trying to
-		# build them.
-		my $f = "$dir/gcf2_libgit2.h";
-		open my $src, '<', $f;
-		$c_src = read_all($src);
-	}
-	unless ($c_src) {
-		seek($err, 0, SEEK_SET);
-		$err = read_all($err);
-		die "E: libgit2 not installed: $err\n";
-	}
+	my $f = "$dir/gcf2_libgit2.h";
+	$c_src = PublicInbox::Git::try_cat($f) or die "cat $f: $!";
 	# append pkg-config results to the source to ensure Inline::C
 	# can rebuild if there's changes (it doesn't seem to detect
 	# $CFG{CCFLAGSEX} nor $CFG{CPPFLAGS} changes)

[PATCH 07/10] treewide: use run_qx where appropriate

[Eric Wong]

This saves us some code, and is a small step towards getting
ProcessIO working with stat, fcntl and other perlops that don't
work with tied handles.
---
 lib/PublicInbox/Admin.pm           |  7 +++----
 lib/PublicInbox/Config.pm          | 14 ++++++--------
 lib/PublicInbox/Fetch.pm           |  7 +++----
 lib/PublicInbox/Import.pm          |  6 ++----
 lib/PublicInbox/LEI.pm             |  7 +++----
 lib/PublicInbox/LeiBlob.pm         | 12 +++++-------
 lib/PublicInbox/LeiMirror.pm       |  7 +++----
 lib/PublicInbox/MultiGit.pm        |  7 +++----
 lib/PublicInbox/Spamcheck/Spamc.pm | 11 +++--------
 lib/PublicInbox/XapHelperCxx.pm    | 11 +++--------
 t/solver_git.t                     | 12 ++++--------
 11 files changed, 38 insertions(+), 63 deletions(-)

diff --git a/lib/PublicInbox/Admin.pm b/lib/PublicInbox/Admin.pm
index 3140afad..893f4a1b 100644
--- a/lib/PublicInbox/Admin.pm
+++ b/lib/PublicInbox/Admin.pm
@@ -9,7 +9,7 @@ use parent qw(Exporter);
 our @EXPORT_OK = qw(setup_signals);
 use PublicInbox::Config;
 use PublicInbox::Inbox;
-use PublicInbox::Spawn qw(popen_rd);
+use PublicInbox::Spawn qw(run_qx);
 use PublicInbox::Eml;
 *rel2abs_collapsed = \&PublicInbox::Config::rel2abs_collapsed;
 
@@ -67,9 +67,8 @@ sub resolve_git_dir {
 	my ($cd) = @_;
 	# try v1 bare git dirs
 	my $cmd = [ qw(git rev-parse --git-dir) ];
-	my $fh = popen_rd($cmd, undef, {-C => $cd});
-	my $dir = do { local $/; <$fh> };
-	close $fh or die "error in @$cmd (cwd:${\($cd // '.')}): $?\n";
+	my $dir = run_qx($cmd, undef, {-C => $cd});
+	die "error in @$cmd (cwd:${\($cd // '.')}): $?\n" if $?;
 	chomp $dir;
 	# --absolute-git-dir requires git v2.13.0+
 	$dir = rel2abs_collapsed($dir, $cd) if $dir !~ m!\A/!;
diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index d156b2d3..0a572103 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -13,7 +13,7 @@ use v5.10.1;
 use parent qw(Exporter);
 our @EXPORT_OK = qw(glob2re rel2abs_collapsed);
 use PublicInbox::Inbox;
-use PublicInbox::Spawn qw(popen_rd);
+use PublicInbox::Spawn qw(popen_rd run_qx);
 our $LD_PRELOAD = $ENV{LD_PRELOAD}; # only valid at startup
 our $DEDUPE; # set to {} to dedupe or clear cache
 
@@ -576,23 +576,21 @@ sub urlmatch {
 	my (%env, %opt);
 	my $cmd = $self->config_cmd(\%env, \%opt);
 	push @$cmd, @bool, qw(--includes -z --get-urlmatch), $key, $url;
-	my $fh = popen_rd($cmd, \%env, \%opt);
-	local $/ = "\0";
-	my $val = <$fh>;
-	if (!close($fh)) {
+	my $val = run_qx($cmd, \%env, \%opt);
+	if ($?) {
 		undef $val;
 		if (@bool && ($? >> 8) == 128) { # not boolean
 		} elsif (($? >> 8) != 1) {
 			$urlmatch_broken = 1;
 		} elsif ($try_git) { # n.b. this takes cwd into account
-			$fh = popen_rd([qw(git config), @bool,
+			$val = run_qx([qw(git config), @bool,
 					qw(-z --get-urlmatch), $key, $url]);
-			$val = <$fh>;
-			close($fh) or undef($val);
+			undef $val if $?;
 		}
 	}
 	$? = 0; # don't influence lei exit status
 	if (defined($val)) {
+		local $/ = "\0";
 		chomp $val;
 		$val = git_bool($val) if @bool;
 	}
diff --git a/lib/PublicInbox/Fetch.pm b/lib/PublicInbox/Fetch.pm
index e41dd448..b0f1437c 100644
--- a/lib/PublicInbox/Fetch.pm
+++ b/lib/PublicInbox/Fetch.pm
@@ -5,7 +5,7 @@ package PublicInbox::Fetch;
 use v5.12;
 use parent qw(PublicInbox::IPC);
 use URI ();
-use PublicInbox::Spawn qw(popen_rd run_wait);
+use PublicInbox::Spawn qw(popen_rd run_qx run_wait);
 use PublicInbox::Admin;
 use PublicInbox::LEI;
 use PublicInbox::LeiCurl;
@@ -20,9 +20,8 @@ sub remote_url ($$) {
 	my $rn = $lei->{opt}->{'try-remote'} // [ 'origin', '_grokmirror' ];
 	for my $r (@$rn) {
 		my $cmd = [ qw(git config), "remote.$r.url" ];
-		my $fh = popen_rd($cmd, undef, { -C => $dir, 2 => $lei->{2} });
-		my $url = <$fh>;
-		close $fh or next;
+		my $url = run_qx($cmd, undef, { -C => $dir, 2 => $lei->{2} });
+		next if $?;
 		$url =~ s!/*\n!!s;
 		return $url;
 	}
diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index fb70b91b..6eee8774 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -8,7 +8,7 @@
 package PublicInbox::Import;
 use v5.12;
 use parent qw(PublicInbox::Lock);
-use PublicInbox::Spawn qw(run_die popen_rd spawn);
+use PublicInbox::Spawn qw(run_die run_qx spawn);
 use PublicInbox::MID qw(mids mid2path);
 use PublicInbox::Address;
 use PublicInbox::Smsg;
@@ -25,10 +25,8 @@ use PublicInbox::Git qw(read_all);
 
 sub default_branch () {
 	state $default_branch = do {
-		my $r = popen_rd([qw(git config --global init.defaultBranch)],
+		my $h = run_qx([qw(git config --global init.defaultBranch)],
 				 { GIT_CONFIG => undef });
-		chomp(my $h = <$r> // '');
-		CORE::close $r;
 		$h eq '' ? 'refs/heads/master' : "refs/heads/$h";
 	}
 }
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index e060bcbe..0f6f7f6f 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -19,7 +19,7 @@ use IO::Handle ();
 use Fcntl qw(SEEK_SET);
 use PublicInbox::Config;
 use PublicInbox::Syscall qw(EPOLLIN);
-use PublicInbox::Spawn qw(run_wait popen_rd);
+use PublicInbox::Spawn qw(run_wait popen_rd run_qx);
 use PublicInbox::Lock;
 use PublicInbox::Eml;
 use PublicInbox::Import;
@@ -1091,9 +1091,8 @@ sub path_to_fd {
 # caller needs to "-t $self->{1}" to check if tty
 sub start_pager {
 	my ($self, $new_env) = @_;
-	my $fh = popen_rd([qw(git var GIT_PAGER)]);
-	chomp(my $pager = <$fh> // '');
-	close($fh) or warn "`git var PAGER' error: \$?=$?";
+	chomp(my $pager = run_qx([qw(git var GIT_PAGER)]));
+	warn "`git var PAGER' error: \$?=$?" if $?;
 	return if $pager eq 'cat' || $pager eq '';
 	$new_env //= {};
 	$new_env->{LESS} //= 'FRX';
diff --git a/lib/PublicInbox/LeiBlob.pm b/lib/PublicInbox/LeiBlob.pm
index 40f64bd9..648d35b6 100644
--- a/lib/PublicInbox/LeiBlob.pm
+++ b/lib/PublicInbox/LeiBlob.pm
@@ -7,7 +7,7 @@ package PublicInbox::LeiBlob;
 use strict;
 use v5.10.1;
 use parent qw(PublicInbox::IPC);
-use PublicInbox::Spawn qw(run_wait popen_rd which);
+use PublicInbox::Spawn qw(run_wait run_qx which);
 use PublicInbox::DS;
 use PublicInbox::Eml;
 use PublicInbox::Git qw(read_all);
@@ -23,9 +23,8 @@ sub get_git_dir ($$) {
 	} else { # implicit --cwd, quiet errors
 		open $opt->{2}, '>', '/dev/null' or die "open /dev/null: $!";
 	}
-	my $r = popen_rd($cmd, {GIT_DIR => undef}, $opt);
-	chomp(my $gd = do { local $/; <$r> });
-	close($r) ? $gd : undef;
+	chomp(my $git_dir = run_qx($cmd, {GIT_DIR => undef}, $opt));
+	$? ? undef : $git_dir;
 }
 
 sub solver_user_cb { # called by solver when done
@@ -122,9 +121,8 @@ sub lei_blob {
 		my $cmd = [ 'git', '--git-dir='.$lei->ale->git->{git_dir},
 				'cat-file', 'blob', $blob ];
 		if (defined $lei->{-attach_idx}) {
-			my $fh = popen_rd($cmd, $lei->{env}, $rdr);
-			my $buf = do { local $/; <$fh> };
-			return extract_attach($lei, $blob, \$buf) if close($fh);
+			my $buf = run_qx($cmd, $lei->{env}, $rdr);
+			return extract_attach($lei, $blob, \$buf) unless $?;
 		}
 		$rdr->{1} = $lei->{1};
 		my $cerr = run_wait($cmd, $lei->{env}, $rdr) or return;
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 43e59e6c..c2f7cbed 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -7,7 +7,7 @@ use v5.12;
 use parent qw(PublicInbox::IPC);
 use IO::Uncompress::Gunzip qw(gunzip $GunzipError);
 use IO::Compress::Gzip qw(gzip $GzipError);
-use PublicInbox::Spawn qw(popen_rd spawn run_wait run_die);
+use PublicInbox::Spawn qw(spawn run_wait run_die run_qx);
 use File::Path ();
 use File::Temp ();
 use File::Spec ();
@@ -57,9 +57,8 @@ sub try_scrape {
 	my $curl = $self->{curl} //= PublicInbox::LeiCurl->new($lei) or return;
 	my $cmd = $curl->for_uri($lei, $uri, '--compressed');
 	my $opt = { 0 => $lei->{0}, 2 => $lei->{2} };
-	my $fh = popen_rd($cmd, undef, $opt);
-	my $html = do { local $/; <$fh> } // die "read(curl $uri): $!";
-	CORE::close($fh) or return $lei->child_error($?, "@$cmd failed");
+	my $html = run_qx($cmd, undef, $opt);
+	return $lei->child_error($?, "@$cmd failed") if $?;
 
 	# we grep with URL below, we don't want Subject/From headers
 	# making us clone random URLs.  This assumes remote instances
diff --git a/lib/PublicInbox/MultiGit.pm b/lib/PublicInbox/MultiGit.pm
index 35bd0251..9074758a 100644
--- a/lib/PublicInbox/MultiGit.pm
+++ b/lib/PublicInbox/MultiGit.pm
@@ -5,7 +5,7 @@
 package PublicInbox::MultiGit;
 use strict;
 use v5.10.1;
-use PublicInbox::Spawn qw(run_die popen_rd);
+use PublicInbox::Spawn qw(run_die run_qx);
 use PublicInbox::Import;
 use File::Temp 0.19;
 use List::Util qw(max);
@@ -112,9 +112,8 @@ sub epoch_cfg_set {
 	my $f = epoch_dir($self)."/$epoch_nr.git/config";
 	my $v = "../../$self->{all}/config";
 	if (-r $f) {
-		my $rd = popen_rd([qw(git config -f), $f, 'include.path']);
-		chomp(my $have = <$rd> // '');
-		return if $have eq $v;
+		chomp(my $x = run_qx([qw(git config -f), $f, 'include.path']));
+		return if $x eq $v;
 	}
 	run_die([qw(git config -f), $f, 'include.path', $v ]);
 }
diff --git a/lib/PublicInbox/Spamcheck/Spamc.pm b/lib/PublicInbox/Spamcheck/Spamc.pm
index cba33a66..798de218 100644
--- a/lib/PublicInbox/Spamcheck/Spamc.pm
+++ b/lib/PublicInbox/Spamcheck/Spamc.pm
@@ -4,7 +4,7 @@
 # Default spam filter class for wrapping spamc(1)
 package PublicInbox::Spamcheck::Spamc;
 use v5.12;
-use PublicInbox::Spawn qw(popen_rd run_wait);
+use PublicInbox::Spawn qw(run_qx run_wait);
 use IO::Handle;
 use Fcntl qw(SEEK_SET);
 
@@ -20,14 +20,9 @@ sub new {
 sub spamcheck {
 	my ($self, $msg, $out) = @_;
 
+	$out = \(my $buf = '') unless ref($out);
 	my $rdr = { 0 => _msg_to_fh($self, $msg) };
-	my $fh = popen_rd($self->{checkcmd}, undef, $rdr);
-	unless (ref $out) {
-		my $buf = '';
-		$out = \$buf;
-	}
-	$$out = do { local $/; <$fh> };
-	close $fh; # PublicInbox::ProcessIO::CLOSE
+	$$out = run_qx($self->{checkcmd}, undef, $rdr);
 	($? || $$out eq '') ? 0 : 1;
 }
 
diff --git a/lib/PublicInbox/XapHelperCxx.pm b/lib/PublicInbox/XapHelperCxx.pm
index bd616b6f..83503035 100644
--- a/lib/PublicInbox/XapHelperCxx.pm
+++ b/lib/PublicInbox/XapHelperCxx.pm
@@ -7,7 +7,7 @@
 # The resulting executable is not linked to Perl in any way.
 package PublicInbox::XapHelperCxx;
 use v5.12;
-use PublicInbox::Spawn qw(popen_rd which);
+use PublicInbox::Spawn qw(run_qx which);
 use PublicInbox::Git qw(read_all);
 use PublicInbox::Search;
 use Fcntl qw(SEEK_SET);
@@ -29,14 +29,9 @@ my $xflags = ($ENV{CXXFLAGS} // '-Wall -ggdb3 -O0') . ' ' .
 my $xap_modversion;
 
 sub xap_cfg (@) {
-	use autodie qw(open seek);
-	open my $err, '+>', undef;
 	my $cmd = [ $ENV{PKG_CONFIG} // 'pkg-config', @_, 'xapian-core' ];
-	my $rd = popen_rd($cmd, undef, { 2 => $err });
-	chomp(my $ret = do { local $/; <$rd> });
-	return $ret if close($rd);
-	seek($err, 0, SEEK_SET);
-	$err = read_all($err);
+	chomp(my $ret = run_qx($cmd, undef, { 2 => \(my $err) }));
+	return $ret if !$?;
 	die <<EOM;
 @$cmd failed: Xapian development files missing? (\$?=$?)
 $err
diff --git a/t/solver_git.t b/t/solver_git.t
index 4f09e05b..ab8aba15 100644
--- a/t/solver_git.t
+++ b/t/solver_git.t
@@ -6,7 +6,7 @@ use PublicInbox::TestCommon;
 use Cwd qw(abs_path);
 require_git v2.6;
 use PublicInbox::ContentHash qw(git_sha);
-use PublicInbox::Spawn qw(popen_rd);
+use PublicInbox::Spawn qw(run_qx);
 require_mods(qw(DBD::SQLite Xapian URI::Escape));
 require PublicInbox::SolverGit;
 my $rdr = { 2 => \(my $null) };
@@ -234,13 +234,9 @@ SKIP: {
 		my $cmd = [ qw(git hash-object -w --stdin) ];
 		my $env = { GIT_DIR => $binfoo };
 		while (my ($label, $size) = each %bin) {
-			pipe(my ($rin, $win)) or BAIL_OUT;
-			my $rout = popen_rd($cmd , $env, { 0 => $rin });
-			$rin = undef;
-			print { $win } ("\0" x $size) or BAIL_OUT;
-			close $win or BAIL_OUT;
-			chomp(my $x = <$rout>);
-			close $rout or BAIL_OUT "$?";
+			my $rdr = { 0 => \("\0" x $size) };
+			chomp(my $x = run_qx($cmd , $env, $rdr));
+			xbail "@$cmd: \$?=$?" if $?;
 			$oid{$label} = $x;
 		}
 

[PATCH 08/10] www_altid: reduce FD pressure in qspawn queues

[Eric Wong]

We can use the built-in stdin redirection functionality of
spawn() instead of creating a pipe that sits idle in the queue
on busy servers.
---
 lib/PublicInbox/WwwAltId.pm | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/WwwAltId.pm b/lib/PublicInbox/WwwAltId.pm
index 48520142..31d9b607 100644
--- a/lib/PublicInbox/WwwAltId.pm
+++ b/lib/PublicInbox/WwwAltId.pm
@@ -61,14 +61,9 @@ The administrator needs to install the sqlite3(1) binary
 to support gzipped sqlite3 dumps.</pre>
 EOF
 
-	# setup stdin, POSIX requires writes <= 512 bytes to succeed so
-	# we can close the pipe right away.
-	pipe(my ($r, $w)) or die "pipe: $!";
-	syswrite($w, ".dump\n") == 6 or die "write: $!";
-	close($w) or die "close: $!";
-
 	# TODO: use -readonly if available with newer sqlite3(1)
-	my $qsp = PublicInbox::Qspawn->new([$sqlite3, $fn], undef, { 0 => $r });
+	my $qsp = PublicInbox::Qspawn->new([$sqlite3, $fn], undef,
+							{ 0 => \".dump\n" });
 	$ctx->{altid_pfx} = $altid_pfx;
 	$env->{'qspawn.filter'} = PublicInbox::GzipFilter->new;
 	$qsp->psgi_yield($env, undef, \&check_output, $ctx);

[PATCH 09/10] xt/eml_check_limits: remove useless import

[Eric Wong]

Found while hunting for popen_rd calls.
---
 xt/eml_check_limits.t | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/xt/eml_check_limits.t b/xt/eml_check_limits.t
index a6d010af..1f89c6d4 100644
--- a/xt/eml_check_limits.t
+++ b/xt/eml_check_limits.t
@@ -1,15 +1,13 @@
 #!perl -w
-# Copyright (C) 2020-2021 all contributors <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 use strict;
 use v5.10.1;
-use Test::More;
 use PublicInbox::TestCommon;
 use PublicInbox::Eml;
 use PublicInbox::Inbox;
 use List::Util qw(max);
 use Benchmark qw(:all :hireswallclock);
-use PublicInbox::Spawn qw(popen_rd);
 use Carp ();
 require_git(2.19); # for --unordered
 require_mods(qw(BSD::Resource));

[PATCH 10/10] lei_ale: use v5.12, autodie, and try_cat

[Eric Wong]

Just things I spotted while looking for other problems
throughout our codebase.
---
 lib/PublicInbox/LeiALE.pm | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/LeiALE.pm b/lib/PublicInbox/LeiALE.pm
index b198af1c..674d897e 100644
--- a/lib/PublicInbox/LeiALE.pm
+++ b/lib/PublicInbox/LeiALE.pm
@@ -1,4 +1,4 @@
-# Copyright (C) 2021 all contributors <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 
 # All Locals Ever: track lei/store + externals ever used as
@@ -6,10 +6,10 @@
 # and --only targets that haven't been through "lei add-external".
 # Typically: ~/.cache/lei/all_locals_ever.git
 package PublicInbox::LeiALE;
-use strict;
-use v5.10.1;
+use v5.12;
 use parent qw(PublicInbox::LeiSearch PublicInbox::Lock);
 use PublicInbox::Git qw(read_all);
+use autodie qw(close open rename seek truncate);
 use PublicInbox::Import;
 use PublicInbox::LeiXSearch;
 use Fcntl qw(SEEK_SET);
@@ -77,18 +77,16 @@ sub refresh_externals {
 	if ($new ne '' || $gone) {
 		$self->{lockfh}->autoflush(1);
 		if ($gone) {
-			seek($self->{lockfh}, 0, SEEK_SET) or die "seek: $!";
-			truncate($self->{lockfh}, 0) or die "truncate: $!";
+			seek($self->{lockfh}, 0, SEEK_SET);
+			truncate($self->{lockfh}, 0);
 		} else {
 			$old = '';
 		}
 		print { $self->{lockfh} } $old, $new or die "print: $!";
 	}
-	$new = $old = '';
+	$new = '';
 	my $f = $self->git->{git_dir}.'/objects/info/alternates';
-	if (open my $fh, '<', $f) {
-		read_all($fh, -s $fh, \$old);
-	}
+	$old = PublicInbox::Git::try_cat($f);
 	for my $x (@ibxish) {
 		$new .= $lei->canonpath_harder($x->git->{git_dir})."/objects\n";
 	}
@@ -98,10 +96,10 @@ sub refresh_externals {
 	# this needs to be atomic since child processes may start
 	# git-cat-file at any time
 	my $tmp = "$f.$$.tmp";
-	open my $fh, '>', $tmp or die "open($tmp): $!";
-	print $fh $new or die "print($tmp): $!";
-	close $fh or die "close($tmp): $!";
-	rename($tmp, $f) or die "rename($tmp, $f): $!";
+	open my $fh, '>', $tmp;
+	print $fh $new;
+	close $fh;
+	rename($tmp, $f)
 }
 
 1;