[PATCH 0/2] more spawn improvements

[PATCH 0/2] more spawn improvements

[Eric Wong]

Less code overall and improved git compatibility.  Maybe one day
posix_spawn(3) can get rlimit support and we can just use that:

http://austingroupbugs.net/view.php?id=603

Eric Wong (2):
  spawn: support chdir via -C option
  spawn: better error handling

 lib/PublicInbox/Admin.pm     | 31 +++++++----------------
 lib/PublicInbox/SolverGit.pm |  6 ++---
 lib/PublicInbox/Spawn.pm     | 48 ++++++++++++++++--------------------
 lib/PublicInbox/SpawnPP.pm   |  7 ++++--
 4 files changed, 38 insertions(+), 54 deletions(-)

[PATCH 1/2] spawn: support chdir via -C option

[Eric Wong]

This simplifies our admin module a bit and allows solver to be
used with v1 inboxes using git versions prior to v1.8.5 (but
still >= git v1.8.0).
---
 lib/PublicInbox/Admin.pm     | 31 +++++++++----------------------
 lib/PublicInbox/SolverGit.pm |  6 +++---
 lib/PublicInbox/Spawn.pm     |  8 ++++++--
 lib/PublicInbox/SpawnPP.pm   |  7 +++++--
 4 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/lib/PublicInbox/Admin.pm b/lib/PublicInbox/Admin.pm
index 5a3554cf..44b44b6e 100644
--- a/lib/PublicInbox/Admin.pm
+++ b/lib/PublicInbox/Admin.pm
@@ -10,6 +10,7 @@ use Cwd 'abs_path';
 use base qw(Exporter);
 our @EXPORT_OK = qw(resolve_repo_dir);
 require PublicInbox::Config;
+use PublicInbox::Spawn qw(popen_rd);
 
 sub resolve_repo_dir {
 	my ($cd, $ver) = @_;
@@ -18,28 +19,14 @@ sub resolve_repo_dir {
 		$$ver = 2 if $ver;
 		return abs_path($prefix);
 	}
-
-	my @cmd = qw(git rev-parse --git-dir);
-	my $cmd = join(' ', @cmd);
-	my $pid = open my $fh, '-|';
-	defined $pid or die "forking $cmd failed: $!\n";
-	if ($pid == 0) {
-		if (defined $cd) {
-			chdir $cd or die "chdir $cd failed: $!\n";
-		}
-		exec @cmd;
-		die "Failed to exec $cmd: $!\n";
-	} else {
-		my $dir = eval {
-			local $/;
-			<$fh>;
-		};
-		close $fh or die "error in $cmd (cwd:$cd): $!\n";
-		chomp $dir;
-		$$ver = 1 if $ver;
-		return abs_path($cd) if ($dir eq '.' && defined $cd);
-		abs_path($dir);
-	}
+	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 ".join(' ', @$cmd)." (cwd:$cd): $!\n";
+	chomp $dir;
+	$$ver = 1 if $ver;
+	return abs_path($cd) if ($dir eq '.' && defined $cd);
+	abs_path($dir);
 }
 
 # for unconfigured inboxes
diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 03666646..c57fb4c6 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -472,7 +472,7 @@ sub do_git_apply ($) {
 	my $patches = $self->{patches};
 
 	# we need --ignore-whitespace because some patches are CRLF
-	my @cmd = (qw(git -C), $dn, qw(apply --cached --ignore-whitespace
+	my @cmd = (qw(git apply --cached --ignore-whitespace
 			--unidiff-zero --whitespace=warn --verbose));
 	my $len = length(join(' ', @cmd));
 	my $total = $self->{tot};
@@ -491,8 +491,8 @@ sub do_git_apply ($) {
 	} while (@$patches && $len < $ARG_SIZE_MAX &&
 		 !oids_same_ish($patches->[0]->{oid_b}, $prv_oid_b));
 
-	my $rdr = { 2 => 1 };
-	my $qsp = PublicInbox::Qspawn->new(\@cmd, $self->{git_env}, $rdr);
+	my $opt = { 2 => 1, -C => $dn };
+	my $qsp = PublicInbox::Qspawn->new(\@cmd, $self->{git_env}, $opt);
 	$self->{-cur_di} = $di;
 	$self->{-qsp} = $qsp;
 	$qsp->psgi_qx($self->{psgi_env}, undef, \&apply_result, $self);
diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index 6d42d5bc..d624c521 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -84,7 +84,8 @@ static void xerr(const char *msg)
  * whatever we'll need in the future.
  * Be sure to update PublicInbox::SpawnPP if this changes
  */
-int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref)
+int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
+		 const char *cd)
 {
 	AV *redir = (AV *)SvRV(redirref);
 	AV *cmd = (AV *)SvRV(cmdref);
@@ -118,6 +119,8 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref)
 		}
 		for (sig = 1; sig < NSIG; sig++)
 			signal(sig, SIG_DFL); /* ignore errors on signals */
+		if (*cd && chdir(cd) < 0)
+			xerr("chdir");
 
 		max = av_len(rlim);
 		for (i = 0; i < max; i += 3) {
@@ -216,7 +219,8 @@ sub spawn ($;$$) {
 		}
 		push @$rlim, $r, @$v;
 	}
-	my $pid = pi_fork_exec($redir, $f, $cmd, \@env, $rlim);
+	my $cd = $opts->{'-C'} // ''; # undef => NULL mapping doesn't work?
+	my $pid = pi_fork_exec($redir, $f, $cmd, \@env, $rlim, $cd);
 	$pid < 0 ? undef : $pid;
 }
 
diff --git a/lib/PublicInbox/SpawnPP.pm b/lib/PublicInbox/SpawnPP.pm
index 2ac02c56..cd682a6b 100644
--- a/lib/PublicInbox/SpawnPP.pm
+++ b/lib/PublicInbox/SpawnPP.pm
@@ -9,8 +9,8 @@ use warnings;
 use POSIX qw(dup2 :signal_h);
 
 # Pure Perl implementation for folks that do not use Inline::C
-sub pi_fork_exec ($$$$$) {
-	my ($redir, $f, $cmd, $env, $rlim) = @_;
+sub pi_fork_exec ($$$$$$) {
+	my ($redir, $f, $cmd, $env, $rlim, $cd) = @_;
 	my $old = POSIX::SigSet->new();
 	my $set = POSIX::SigSet->new();
 	$set->fillset or die "fillset failed: $!";
@@ -33,6 +33,9 @@ sub pi_fork_exec ($$$$$) {
 			dup2($parent_fd, $child_fd) or
 				die "dup2($parent_fd, $child_fd): $!\n";
 		}
+		if ($cd ne '') {
+			chdir $cd or die "chdir $cd: $!";
+		}
 		if ($ENV{MOD_PERL}) {
 			exec which('env'), '-i', @$env, @$cmd;
 			die "exec env -i ... $cmd->[0] failed: $!\n";

[PATCH 2/2] spawn: better error handling

[Eric Wong]

Since vfork always shares memory between the child and parent,
we can propagate errors to the parent errno using shared memory
instead of just dumping to stderr and hoping somebody sees it.
---
 lib/PublicInbox/Spawn.pm | 42 +++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index d624c521..6eea2b9c 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -56,26 +56,10 @@ my $vfork_spawn = <<'VFORK_SPAWN';
 	dst[real_len] = 0; \
 } while (0)
 
-static void *deconst(const char *s)
-{
-	union { const char *in; void *out; } u;
-	u.in = s;
-	return u.out;
-}
-
 /* needs to be safe inside a vfork'ed process */
-static void xerr(const char *msg)
+static void exit_err(int *cerrnum)
 {
-	struct iovec iov[3];
-	const char *err = strerror(errno); /* should be safe in practice */
-
-	iov[0].iov_base = deconst(msg);
-	iov[0].iov_len = strlen(msg);
-	iov[1].iov_base = deconst(err);
-	iov[1].iov_len = strlen(err);
-	iov[2].iov_base = deconst("\n");
-	iov[2].iov_len = 1;
-	writev(2, iov, 3);
+	*cerrnum = errno;
 	_exit(1);
 }
 
@@ -95,7 +79,7 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	pid_t pid;
 	char **argv, **envp;
 	sigset_t set, old;
-	int ret, errnum;
+	int ret, perrnum, cerrnum = 0;
 
 	AV2C_COPY(argv, cmd);
 	AV2C_COPY(envp, env);
@@ -115,12 +99,12 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 			if (parent_fd == child_fd)
 				continue;
 			if (dup2(parent_fd, child_fd) < 0)
-				xerr("dup2");
+				exit_err(&cerrnum);
 		}
 		for (sig = 1; sig < NSIG; sig++)
 			signal(sig, SIG_DFL); /* ignore errors on signals */
 		if (*cd && chdir(cd) < 0)
-			xerr("chdir");
+			exit_err(&cerrnum);
 
 		max = av_len(rlim);
 		for (i = 0; i < max; i += 3) {
@@ -132,7 +116,7 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 			rl.rlim_cur = SvIV(*soft);
 			rl.rlim_max = SvIV(*hard);
 			if (setrlimit(SvIV(*res), &rl) < 0)
-				xerr("sertlimit");
+				exit_err(&cerrnum);
 		}
 
 		/*
@@ -140,13 +124,19 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 		 * to the group taking out a subprocess
 		 */
 		execve(filename, argv, envp);
-		xerr("execve failed");
+		exit_err(&cerrnum);
 	}
-	errnum = errno;
+	perrnum = errno;
 	ret = sigprocmask(SIG_SETMASK, &old, NULL);
 	assert(ret == 0 && "BUG calling sigprocmask to restore");
-	errno = errnum;
-
+	if (cerrnum) {
+		if (pid > 0)
+			waitpid(pid, NULL, 0);
+		pid = -1;
+		errno = cerrnum;
+	} else if (perrnum) {
+		errno = perrnum;
+	}
 	return (int)pid;
 }
 VFORK_SPAWN