* [PATCH 01/10] spawn: croak directly in C pi_fork_exec
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 02/10] spawnpp: use autodie for syscall failures Eric Wong
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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 {
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 02/10] spawnpp: use autodie for syscall failures
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
2023-10-27 22:21 ` [PATCH 01/10] spawn: croak directly in C pi_fork_exec Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 03/10] spawn: avoid alloca in C pi_fork_exec Eric Wong
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 03/10] spawn: avoid alloca in C pi_fork_exec
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
2023-10-27 22:21 ` [PATCH 01/10] spawn: croak directly in C pi_fork_exec Eric Wong
2023-10-27 22:21 ` [PATCH 02/10] spawnpp: use autodie for syscall failures Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 04/10] git: use run_qx to read `git --version' Eric Wong
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 04/10] git: use run_qx to read `git --version'
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
` (2 preceding siblings ...)
2023-10-27 22:21 ` [PATCH 03/10] spawn: avoid alloca in C pi_fork_exec Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 05/10] git: avoid extra stat(2) for git version Eric Wong
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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)";
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 05/10] git: avoid extra stat(2) for git version
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
` (3 preceding siblings ...)
2023-10-27 22:21 ` [PATCH 04/10] git: use run_qx to read `git --version' Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 06/10] gcf2: simplify pkg-config and Inline::C setup Eric Wong
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 06/10] gcf2: simplify pkg-config and Inline::C setup
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
` (4 preceding siblings ...)
2023-10-27 22:21 ` [PATCH 05/10] git: avoid extra stat(2) for git version Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 07/10] treewide: use run_qx where appropriate Eric Wong
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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)
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 07/10] treewide: use run_qx where appropriate
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
` (5 preceding siblings ...)
2023-10-27 22:21 ` [PATCH 06/10] gcf2: simplify pkg-config and Inline::C setup Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 08/10] www_altid: reduce FD pressure in qspawn queues Eric Wong
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 08/10] www_altid: reduce FD pressure in qspawn queues
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
` (6 preceding siblings ...)
2023-10-27 22:21 ` [PATCH 07/10] treewide: use run_qx where appropriate Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 09/10] xt/eml_check_limits: remove useless import Eric Wong
2023-10-27 22:21 ` [PATCH 10/10] lei_ale: use v5.12, autodie, and try_cat Eric Wong
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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);
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 09/10] xt/eml_check_limits: remove useless import
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
` (7 preceding siblings ...)
2023-10-27 22:21 ` [PATCH 08/10] www_altid: reduce FD pressure in qspawn queues Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
2023-10-27 22:21 ` [PATCH 10/10] lei_ale: use v5.12, autodie, and try_cat Eric Wong
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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));
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 10/10] lei_ale: use v5.12, autodie, and try_cat
2023-10-27 22:21 [PATCH 00/10] first steps towards eliminating TIEHANDLE Eric Wong
` (8 preceding siblings ...)
2023-10-27 22:21 ` [PATCH 09/10] xt/eml_check_limits: remove useless import Eric Wong
@ 2023-10-27 22:21 ` Eric Wong
9 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2023-10-27 22:21 UTC (permalink / raw)
To: meta
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;
^ permalink raw reply related [flat|nested] 11+ messages in thread