[PATCH 0/7] various build fixes + OpenBSD compat

[PATCH 0/7] various build fixes + OpenBSD compat

[Eric Wong]

7/7 had me shaking my head for a bit, but I'd rather pay
that price than introduce weirdness with C++ templates.

Eric Wong (7):
  treewide: drop MSG_EOR with AF_UNIX+SOCK_SEQPACKET
  Makefile.PL: fix syntax for ASan and valgrind targets
  Makefile.PL: depend on autodie, at least for tests
  t/kqnotify: improve test reliability on OpenBSD
  xap_helper.h: don't compress debug sections on OpenBSD
  xap_helper.h: limit stderr assignment to glibc+FreeBSD
  xap_helper.h: fix double-free on OpenBSD hdestroy

 INSTALL                          |  1 +
 Makefile.PL                      | 10 +++--
 lib/PublicInbox/CodeSearchIdx.pm |  8 ++--
 lib/PublicInbox/IPC.pm           | 10 ++---
 lib/PublicInbox/LEI.pm           | 28 ++++++-------
 lib/PublicInbox/PktOp.pm         |  4 +-
 lib/PublicInbox/WQBlocked.pm     |  3 +-
 lib/PublicInbox/XapClient.pm     |  4 +-
 lib/PublicInbox/XapHelperCxx.pm  |  4 +-
 lib/PublicInbox/xap_helper.h     | 68 ++++++++++++++++++++++++++++----
 script/lei                       | 10 ++---
 t/check-www-inbox.perl           |  4 +-
 t/cmd_ipc.t                      |  8 ++--
 t/kqnotify.t                     | 55 +++++++++++++++++++-------
 t/lei-daemon.t                   |  4 +-
 t/xap_helper.t                   | 11 +++---
 16 files changed, 158 insertions(+), 74 deletions(-)

[PATCH 1/7] treewide: drop MSG_EOR with AF_UNIX+SOCK_SEQPACKET

[Eric Wong]

It's apparently not needed for AF_UNIX + SOCK_SEQPACKET as our
receivers never check for MSG_EOR in "struct msghdr".msg_flags
anyways.  I don't believe POSIX is clear on the exact semantics
of MSG_EOR on this socket type.  This works around truncation
problems on OpenBSD recvmsg when MSG_EOR is used by the sender.

Link: https://marc.info/?i=20230826020759.M335788@dcvr
---
 lib/PublicInbox/CodeSearchIdx.pm |  8 ++++----
 lib/PublicInbox/IPC.pm           | 10 +++++-----
 lib/PublicInbox/LEI.pm           | 28 ++++++++++++++--------------
 lib/PublicInbox/PktOp.pm         |  4 ++--
 lib/PublicInbox/WQBlocked.pm     |  3 +--
 lib/PublicInbox/XapClient.pm     |  4 ++--
 script/lei                       | 10 +++++-----
 t/check-www-inbox.perl           |  4 ++--
 t/cmd_ipc.t                      |  8 ++++----
 t/lei-daemon.t                   |  4 ++--
 t/xap_helper.t                   |  4 ++--
 11 files changed, 43 insertions(+), 44 deletions(-)

diff --git a/lib/PublicInbox/CodeSearchIdx.pm b/lib/PublicInbox/CodeSearchIdx.pm
index f9251be5..2e46e30a 100644
--- a/lib/PublicInbox/CodeSearchIdx.pm
+++ b/lib/PublicInbox/CodeSearchIdx.pm
@@ -55,7 +55,7 @@ use PublicInbox::CidxLogP;
 use PublicInbox::CidxComm;
 use PublicInbox::Git qw(%OFMT2HEXLEN);
 use PublicInbox::Compat qw(uniqstr);
-use Socket qw(MSG_EOR AF_UNIX SOCK_SEQPACKET);
+use Socket qw(AF_UNIX SOCK_SEQPACKET);
 use Carp ();
 our (
 	$LIVE, # pid => cmd
@@ -253,7 +253,7 @@ sub cidx_reap_log { # awaitpid cb
 	my ($pid, $self, $op_p) = @_;
 	if (!$? || ($DO_QUIT && (($? & 127) == $DO_QUIT ||
 				($? & 127) == POSIX::SIGPIPE))) {
-		send($op_p, "shard_done $self->{shard}", MSG_EOR);
+		send($op_p, "shard_done $self->{shard}", 0);
 	} else {
 		warn "E: git @LOG_STDIN: \$?=$?\n";
 		$self->{xdb}->cancel_transaction;
@@ -501,7 +501,7 @@ sub shard_commit { # via wq_io_do
 	my ($self) = @_;
 	my $op_p = delete($self->{0}) // die 'BUG: no {0} op_p';
 	$self->commit_txn_lazy;
-	send($op_p, "shard_done $self->{shard}", MSG_EOR);
+	send($op_p, "shard_done $self->{shard}", 0);
 }
 
 sub assoc_max_init ($) {
@@ -782,7 +782,7 @@ sub prune_commit { # via wq_io_do in IDX_SHARDS
 	my $prune_op_p = delete $self->{0} // die 'BUG: no {0} op_p';
 	my $nr = delete $self->{nr_prune} // die 'BUG: nr_prune undef';
 	cidx_ckpoint($self, "prune [$self->{shard}] $nr done") if $nr;
-	send($prune_op_p, "prune_done $self->{shard}", MSG_EOR);
+	send($prune_op_p, "prune_done $self->{shard}", 0);
 }
 
 sub shards_active { # post_loop_do
diff --git a/lib/PublicInbox/IPC.pm b/lib/PublicInbox/IPC.pm
index 84765748..766c377f 100644
--- a/lib/PublicInbox/IPC.pm
+++ b/lib/PublicInbox/IPC.pm
@@ -16,7 +16,7 @@ use PublicInbox::DS qw(awaitpid);
 use PublicInbox::Spawn;
 use PublicInbox::OnDestroy;
 use PublicInbox::WQWorker;
-use Socket qw(AF_UNIX MSG_EOR SOCK_STREAM);
+use Socket qw(AF_UNIX SOCK_STREAM);
 my $MY_MAX_ARG_STRLEN = 4096 * 33; # extra 4K for serialization
 my $SEQPACKET = eval { Socket::SOCK_SEQPACKET() }; # portable enough?
 our @EXPORT_OK = qw(ipc_freeze ipc_thaw nproc_shards);
@@ -279,7 +279,7 @@ sub wq_broadcast {
 		my $buf = ipc_freeze([$sub, @args]);
 		for my $bcast1 (values %$wkr) {
 			my $sock = $bcast1 // $self->{-wq_s1} // next;
-			send($sock, $buf, MSG_EOR) // croak "send: $!";
+			send($sock, $buf, 0) // croak "send: $!";
 			# XXX shouldn't have to deal with EMSGSIZE here...
 		}
 	} else {
@@ -294,7 +294,7 @@ sub stream_in_full ($$$) {
 		croak "socketpair: $!";
 	my $n = send_cmd($s1, [ fileno($r) ],
 			ipc_freeze(['do_sock_stream', length($buf)]),
-			MSG_EOR) // croak "sendmsg: $!";
+			0) // croak "sendmsg: $!";
 	undef $r;
 	$n = send_cmd($w, $fds, $buf, 0) // croak "sendmsg: $!";
 	while ($n < length($buf)) {
@@ -316,7 +316,7 @@ sub wq_io_do { # always async
 		if (length($buf) > $MY_MAX_ARG_STRLEN) {
 			stream_in_full($s1, $fds, $buf);
 		} else {
-			my $n = send_cmd $s1, $fds, $buf, MSG_EOR;
+			my $n = send_cmd $s1, $fds, $buf, 0;
 			return if defined($n); # likely
 			$!{ETOOMANYREFS} and
 				croak "sendmsg: $! (check RLIMIT_NOFILE)";
@@ -365,7 +365,7 @@ sub wq_nonblock_do { # always async
 	if ($self->{wqb}) { # saturated once, assume saturated forever
 		$self->{wqb}->flush_send($buf);
 	} else {
-		$send_cmd->($self->{-wq_s1}, [], $buf, MSG_EOR) //
+		$send_cmd->($self->{-wq_s1}, [], $buf, 0) //
 			($!{EAGAIN} ? PublicInbox::WQBlocked->new($self, $buf)
 					: croak("sendmsg: $!"));
 	}
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 578686e2..5fbb1211 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -10,7 +10,7 @@ use v5.12;
 use parent qw(PublicInbox::DS PublicInbox::LeiExternal
 	PublicInbox::LeiQuery);
 use Getopt::Long ();
-use Socket qw(AF_UNIX SOCK_SEQPACKET MSG_EOR pack_sockaddr_un);
+use Socket qw(AF_UNIX SOCK_SEQPACKET pack_sockaddr_un);
 use Errno qw(EPIPE EAGAIN ECONNREFUSED ENOENT ECONNRESET);
 use Cwd qw(getcwd);
 use POSIX qw(strftime);
@@ -481,7 +481,7 @@ sub x_it ($$) {
 	if ($self->{pkt_op_p}) { # worker => lei-daemon
 		$self->{pkt_op_p}->pkt_do('x_it', $code);
 	} elsif ($self->{sock}) { # lei->daemon => lei(1) client
-		send($self->{sock}, "x_it $code", MSG_EOR);
+		send($self->{sock}, "x_it $code", 0);
 	} elsif ($quit == \&CORE::exit) { # an admin (one-shot) command
 		exit($code >> 8);
 	} # else ignore if client disconnected
@@ -548,7 +548,7 @@ sub child_error { # passes non-fatal curl exit codes to user
 	if ($self->{pkt_op_p}) { # to top lei-daemon
 		$self->{pkt_op_p}->pkt_do('child_error', $child_error);
 	} elsif ($self->{sock}) { # to lei(1) client
-		send($self->{sock}, "child_error $child_error", MSG_EOR);
+		send($self->{sock}, "child_error $child_error", 0);
 	} # else noop if client disconnected
 }
 
@@ -1017,7 +1017,7 @@ sub send_exec_cmd { # tell script/lei to execute a command
 	PublicInbox::IPC::send_cmd(
 			$self->{sock} // die('lei client gone'),
 			[ map { fileno($_) } @$io ],
-			exec_buf($cmd, $env), MSG_EOR) //
+			exec_buf($cmd, $env), 0) //
 		Carp::croak("sendmsg: $!");
 }
 
@@ -1028,7 +1028,7 @@ sub poke_mua { # forces terminal MUAs to wake up and hopefully notice new mail
 	while (my $op = shift(@$alerts)) {
 		if ($op eq ':WINCH') {
 			# hit the process group that started the MUA
-			send($sock, '-WINCH', MSG_EOR) if $sock;
+			send($sock, '-WINCH', 0) if $sock;
 		} elsif ($op eq ':bell') {
 			out($self, "\a");
 		} elsif ($op =~ /(?<!\\),/) { # bare ',' (not ',,')
@@ -1037,7 +1037,7 @@ sub poke_mua { # forces terminal MUAs to wake up and hopefully notice new mail
 			my $cmd = $1; # run an arbitrary command
 			require Text::ParseWords;
 			$cmd = [ Text::ParseWords::shellwords($cmd) ];
-			send($sock, exec_buf($cmd, {}), MSG_EOR) if $sock;
+			send($sock, exec_buf($cmd, {}), 0) if $sock;
 		} else {
 			warn("W: unsupported --alert=$op\n"); # non-fatal
 		}
@@ -1093,7 +1093,7 @@ sub pgr_err {
 	print { $self->{2} } @msg;
 	$self->{2}->autoflush(1);
 	stop_pager($self);
-	send($self->{sock}, 'wait', MSG_EOR); # wait for user to quit pager
+	send($self->{sock}, 'wait', 0); # wait for user to quit pager
 }
 
 sub stop_pager {
@@ -1110,25 +1110,25 @@ sub accept_dispatch { # Listener {post_accept} callback
 	my $self = bless { sock => $sock }, __PACKAGE__;
 	vec(my $rvec = '', fileno($sock), 1) = 1;
 	select($rvec, undef, undef, 60) or
-		return send($sock, 'timed out waiting to recv FDs', MSG_EOR);
+		return send($sock, 'timed out waiting to recv FDs', 0);
 	# (4096 * 33) >MAX_ARG_STRLEN
 	my @fds = PublicInbox::IPC::recv_cmd($sock, my $buf, 4096 * 33) or
 		return; # EOF
 	if (!defined($fds[0])) {
 		warn(my $msg = "recv_cmd failed: $!");
-		return send($sock, $msg, MSG_EOR);
+		return send($sock, $msg, 0);
 	} else {
 		my $i = 0;
 		for my $fd (@fds) {
 			open($self->{$i++}, '+<&=', $fd) and next;
-			send($sock, "open(+<&=$fd) (FD=$i): $!", MSG_EOR);
+			send($sock, "open(+<&=$fd) (FD=$i): $!", 0);
 		}
-		$i == 4 or return send($sock, 'not enough FDs='.($i-1), MSG_EOR)
+		$i == 4 or return send($sock, 'not enough FDs='.($i-1), 0)
 	}
 	# $ENV_STR = join('', map { "\0$_=$ENV{$_}" } keys %ENV);
 	# $buf = "$argc\0".join("\0", @ARGV).$ENV_STR."\0\0";
 	substr($buf, -2, 2, '') eq "\0\0" or  # s/\0\0\z//
-		return send($sock, 'request command truncated', MSG_EOR);
+		return send($sock, 'request command truncated', 0);
 	my ($argc, @argv) = split(/\0/, $buf, -1);
 	undef $buf;
 	my %env = map { split(/=/, $_, 2) } splice(@argv, $argc);
@@ -1174,7 +1174,7 @@ sub event_step {
 			die "unrecognized client signal: $buf";
 		}
 		my $s = $self->{-socks} // []; # lei up --all
-		@$s = grep { send($_, $buf, MSG_EOR) } @$s;
+		@$s = grep { send($_, $buf, 0) } @$s;
 	};
 	if (my $err = $@) {
 		eval { $self->fail($err) };
@@ -1534,7 +1534,7 @@ sub cfg_dump ($$) {
 sub request_umask {
 	my ($lei) = @_;
 	my $s = $lei->{sock} // return;
-	send($s, 'umask', MSG_EOR) // die "send: $!";
+	send($s, 'umask', 0) // die "send: $!";
 	vec(my $rvec = '', fileno($s), 1) = 1;
 	select($rvec, undef, undef, 2) or die 'timeout waiting for umask';
 	recv($s, my $v, 5, 0) // die "recv: $!";
diff --git a/lib/PublicInbox/PktOp.pm b/lib/PublicInbox/PktOp.pm
index 4c434566..dc432307 100644
--- a/lib/PublicInbox/PktOp.pm
+++ b/lib/PublicInbox/PktOp.pm
@@ -11,7 +11,7 @@ use v5.10.1;
 use parent qw(PublicInbox::DS);
 use Errno qw(EAGAIN ECONNRESET);
 use PublicInbox::Syscall qw(EPOLLIN);
-use Socket qw(AF_UNIX MSG_EOR SOCK_SEQPACKET);
+use Socket qw(AF_UNIX SOCK_SEQPACKET);
 use PublicInbox::IPC qw(ipc_freeze ipc_thaw);
 use Scalar::Util qw(blessed);
 
@@ -32,7 +32,7 @@ sub pair {
 
 sub pkt_do { # for the producer to trigger event_step in consumer
 	my ($self, $cmd, @args) = @_;
-	send($self->{op_p}, @args ? "$cmd\0".ipc_freeze(\@args) : $cmd, MSG_EOR)
+	send($self->{op_p}, @args ? "$cmd\0".ipc_freeze(\@args) : $cmd, 0)
 }
 
 sub event_step {
diff --git a/lib/PublicInbox/WQBlocked.pm b/lib/PublicInbox/WQBlocked.pm
index fbb43600..8d931fa9 100644
--- a/lib/PublicInbox/WQBlocked.pm
+++ b/lib/PublicInbox/WQBlocked.pm
@@ -8,7 +8,6 @@ use parent qw(PublicInbox::DS);
 use PublicInbox::Syscall qw(EPOLLOUT EPOLLONESHOT);
 use PublicInbox::IPC;
 use Carp ();
-use Socket qw(MSG_EOR);
 
 sub new {
 	my ($cls, $wq, $buf) = @_;
@@ -25,7 +24,7 @@ sub flush_send {
 		} else {
 			my $wq_s1 = $self->{sock};
 			my $n = $PublicInbox::IPC::send_cmd->($wq_s1, [], $buf,
-								MSG_EOR);
+								0);
 			next if defined($n);
 			Carp::croak("sendmsg: $!") unless $!{EAGAIN};
 			PublicInbox::DS::epwait($wq_s1, EPOLLOUT|EPOLLONESHOT);
diff --git a/lib/PublicInbox/XapClient.pm b/lib/PublicInbox/XapClient.pm
index 56e3c3b4..f6c09c3b 100644
--- a/lib/PublicInbox/XapClient.pm
+++ b/lib/PublicInbox/XapClient.pm
@@ -9,7 +9,7 @@
 package PublicInbox::XapClient;
 use v5.12;
 use PublicInbox::Spawn qw(spawn);
-use Socket qw(AF_UNIX SOCK_SEQPACKET MSG_EOR);
+use Socket qw(AF_UNIX SOCK_SEQPACKET);
 use PublicInbox::IPC;
 
 sub mkreq {
@@ -21,7 +21,7 @@ sub mkreq {
 	}
 	my @fds = map fileno($_), @$ios;
 	my $buf = join("\0", @arg, '');
-	$n = PublicInbox::IPC::send_cmd($self->{io}, \@fds, $buf, MSG_EOR) //
+	$n = PublicInbox::IPC::send_cmd($self->{io}, \@fds, $buf, 0) //
 		die "send_cmd: $!";
 	$n == length($buf) or die "send_cmd: $n != ".length($buf);
 	$r;
diff --git a/script/lei b/script/lei
index 5feb7751..a77ea880 100755
--- a/script/lei
+++ b/script/lei
@@ -2,7 +2,7 @@
 # Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 use v5.12;
-use Socket qw(AF_UNIX SOCK_SEQPACKET MSG_EOR pack_sockaddr_un);
+use Socket qw(AF_UNIX SOCK_SEQPACKET pack_sockaddr_un);
 use PublicInbox::CmdIPC4;
 my $narg = 5;
 my $sock;
@@ -109,9 +109,9 @@ open my $dh, '<', '.' or die "open(.) $!";
 my $buf = join("\0", scalar(@ARGV), @ARGV);
 while (my ($k, $v) = each %ENV) { $buf .= "\0$k=$v" }
 $buf .= "\0\0";
-$send_cmd->($sock, [0, 1, 2, fileno($dh)], $buf, MSG_EOR) or die "sendmsg: $!";
-$SIG{TSTP} = sub { send($sock, 'STOP', MSG_EOR); kill 'STOP', $$ };
-$SIG{CONT} = sub { send($sock, 'CONT', MSG_EOR) };
+$send_cmd->($sock, [0, 1, 2, fileno($dh)], $buf, 0) or die "sendmsg: $!";
+$SIG{TSTP} = sub { send($sock, 'STOP', 0); kill 'STOP', $$ };
+$SIG{CONT} = sub { send($sock, 'CONT', 0) };
 
 my $x_it_code = 0;
 while (1) {
@@ -126,7 +126,7 @@ while (1) {
 	} elsif ($buf eq '-WINCH') {
 		kill($buf, @parent); # for MUA
 	} elsif ($buf eq 'umask') {
-		send($sock, 'u'.pack('V', umask), MSG_EOR) or die "send: $!"
+		send($sock, 'u'.pack('V', umask), 0) or die "send: $!"
 	} elsif ($buf =~ /\Ax_it ([0-9]+)\z/) {
 		$x_it_code ||= $1 + 0;
 		last;
diff --git a/t/check-www-inbox.perl b/t/check-www-inbox.perl
index 033b90d1..46f9ce1e 100644
--- a/t/check-www-inbox.perl
+++ b/t/check-www-inbox.perl
@@ -123,7 +123,7 @@ while (keys %workers) { # reacts to SIGCHLD
 		}
 	}
 	while ($u = shift @queue) {
-		my $s = $todo[1]->send($u, MSG_EOR);
+		my $s = $todo[1]->send($u, 0);
 		if ($!{EAGAIN}) {
 			unshift @queue, $u;
 			last;
@@ -177,7 +177,7 @@ sub worker_loop {
 		foreach my $l (@links, "DONE\t$u") {
 			next if $l eq '' || $l =~ /\.mbox(?:\.gz)\z/;
 			do {
-				$s = $done_wr->send($l, MSG_EOR);
+				$s = $done_wr->send($l, 0);
 			} while (!defined $s && $!{EINTR});
 			die "$$ send $!\n" unless defined $s;
 			my $n = length($l);
diff --git a/t/cmd_ipc.t b/t/cmd_ipc.t
index cd76d5e8..403d0eed 100644
--- a/t/cmd_ipc.t
+++ b/t/cmd_ipc.t
@@ -5,7 +5,7 @@ use strict;
 use v5.10.1;
 use Test::More;
 use PublicInbox::TestCommon;
-use Socket qw(AF_UNIX SOCK_STREAM MSG_EOR);
+use Socket qw(AF_UNIX SOCK_STREAM);
 pipe(my ($r, $w)) or BAIL_OUT;
 my ($send, $recv);
 require_ok 'PublicInbox::Spawn';
@@ -122,7 +122,7 @@ SKIP: {
 	$send = $send_ic;
 	$recv = $recv_ic;
 	$do_test->(SOCK_STREAM, 0, 'Inline::C stream');
-	$do_test->($SOCK_SEQPACKET, MSG_EOR, 'Inline::C seqpacket');
+	$do_test->($SOCK_SEQPACKET, 0, 'Inline::C seqpacket');
 }
 
 SKIP: {
@@ -131,7 +131,7 @@ SKIP: {
 	$send = PublicInbox::CmdIPC4->can('send_cmd4');
 	$recv = PublicInbox::CmdIPC4->can('recv_cmd4');
 	$do_test->(SOCK_STREAM, 0, 'MsgHdr stream');
-	$do_test->($SOCK_SEQPACKET, MSG_EOR, 'MsgHdr seqpacket');
+	$do_test->($SOCK_SEQPACKET, 0, 'MsgHdr seqpacket');
 	SKIP: {
 		($send_ic && $recv_ic) or
 			skip 'Inline::C not installed/enabled', 12;
@@ -149,7 +149,7 @@ SKIP: {
 	$recv = PublicInbox::Syscall->can('recv_cmd4') or
 		skip 'recv_cmd4 not defined for arch';
 	$do_test->(SOCK_STREAM, 0, 'PP Linux stream');
-	$do_test->($SOCK_SEQPACKET, MSG_EOR, 'PP Linux seqpacket');
+	$do_test->($SOCK_SEQPACKET, 0, 'PP Linux seqpacket');
 }
 
 done_testing;
diff --git a/t/lei-daemon.t b/t/lei-daemon.t
index e11105bc..78ed265e 100644
--- a/t/lei-daemon.t
+++ b/t/lei-daemon.t
@@ -2,7 +2,7 @@
 # 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 PublicInbox::TestCommon;
-use Socket qw(AF_UNIX SOCK_SEQPACKET MSG_EOR pack_sockaddr_un);
+use Socket qw(AF_UNIX SOCK_SEQPACKET pack_sockaddr_un);
 
 test_lei({ daemon_only => 1 }, sub {
 	my $send_cmd = PublicInbox::Spawn->can('send_cmd4') // do {
@@ -40,7 +40,7 @@ test_lei({ daemon_only => 1 }, sub {
 			socket(my $c, AF_UNIX, SOCK_SEQPACKET, 0) or
 							BAIL_OUT "socket: $!";
 			connect($c, $addr) or BAIL_OUT "connect: $!";
-			$send_cmd->($c, \@fds, 'hi',  MSG_EOR);
+			$send_cmd->($c, \@fds, 'hi',  0);
 		}
 		lei_ok('daemon-pid');
 		chomp($pid = $lei_out);
diff --git a/t/xap_helper.t b/t/xap_helper.t
index 3646cf97..73c1c849 100644
--- a/t/xap_helper.t
+++ b/t/xap_helper.t
@@ -7,7 +7,7 @@ require_mods(qw(DBD::SQLite Search::Xapian));
 my $msg = no_scm_rights;
 plan(skip_all => $msg) if $msg; # TODO: FIFO support?
 use PublicInbox::Spawn qw(spawn);
-use Socket qw(AF_UNIX SOCK_SEQPACKET SOCK_STREAM MSG_EOR);
+use Socket qw(AF_UNIX SOCK_SEQPACKET SOCK_STREAM);
 require PublicInbox::AutoReap;
 require PublicInbox::IPC;
 require PublicInbox::XapClient;
@@ -54,7 +54,7 @@ my $doreq = sub {
 	my $buf = join("\0", @arg, '');
 	my @fds = fileno($y);
 	push @fds, fileno($err) if $err;
-	my $n = PublicInbox::IPC::send_cmd($s, \@fds, $buf, MSG_EOR);
+	my $n = PublicInbox::IPC::send_cmd($s, \@fds, $buf, 0);
 	$n // xbail "send: $!";
 	my $arg = "@arg";
 	$arg =~ s/\Q$tmp\E/\$TMP/gs;

[PATCH 2/7] Makefile.PL: fix syntax for ASan and valgrind targets

[Eric Wong]

Mixing various quoting and escaping rules between shell, make,
and Perl got confusing in Makefile.PL :x  This hopefully sorts
out my confusion.

We'll also fix and use TEST_XH_CXX_ONLY=1 to avoid needlessly
running the tests on the XS||SWIG implementation when we're
checking for memory errors.

Fixes: 2312ca26023fcbe3 (makefile: add targets for ASan and valgrind)
---
 Makefile.PL    | 9 +++++----
 t/xap_helper.t | 7 +++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Makefile.PL b/Makefile.PL
index 5865a252..d2309336 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -264,12 +264,13 @@ lib/PublicInbox.pm : FORCE
 	VERSION=\$(VERSION) \$(PERL) -w ./version-gen.perl
 
 test-asan : pure_all
-	CXXFLAGS='-O0 -Wall -ggdb3 -fsanitize=address' \
+	TEST_XH_CXX_ONLY=1 CXXFLAGS='-O0 -Wall -ggdb3 -fsanitize=address' \\
 		prove -bvw t/xap_helper.t
 
-VG_OPT = '-v --trace-children=yes --track-fds=yes'
-VG_OPT += ' --leak-check=yes --track-origins=yes'
+VG_OPT = -v --trace-children=yes --track-fds=yes
+VG_OPT += --leak-check=yes --track-origins=yes
 test-valgrind : pure_all
-	VALGRIND="valgrind \$\$(VG_OPT)" prove -bvw t/xap_helper.t
+	TEST_XH_CXX_ONLY=1 VALGRIND="valgrind \$(VG_OPT)" \\
+		prove -bvw t/xap_helper.t
 EOF
 }
diff --git a/t/xap_helper.t b/t/xap_helper.t
index 73c1c849..b68f2773 100644
--- a/t/xap_helper.t
+++ b/t/xap_helper.t
@@ -107,13 +107,12 @@ my $test = sub {
 };
 my $ar;
 
-my @NO_CXX;
-if (!$ENV{TEST_XH_CXX_ONLY}) {
+my @NO_CXX = (1);
+unless ($ENV{TEST_XH_CXX_ONLY}) {
 	$ar = $test->(qw[-MPublicInbox::XapHelper -e
 			PublicInbox::XapHelper::start('-j0')]);
 	$ar = $test->(qw[-MPublicInbox::XapHelper -e
 			PublicInbox::XapHelper::start('-j1')]);
-	push @NO_CXX, 0;
 }
 SKIP: {
 	eval {
@@ -121,8 +120,8 @@ SKIP: {
 		PublicInbox::XapHelperCxx::check_build();
 	};
 	skip "XapHelperCxx build: $@", 1 if $@;
-	push @NO_CXX, 1;
 
+	@NO_CXX = $ENV{TEST_XH_CXX_ONLY} ? (0) : (0, 1);
 	$ar = $test->(qw[-MPublicInbox::XapHelperCxx -e
 			PublicInbox::XapHelperCxx::start('-j0')]);
 	$ar = $test->(qw[-MPublicInbox::XapHelperCxx -e

[PATCH 3/7] Makefile.PL: depend on autodie, at least for tests

[Eric Wong]

While using autodie everywhere is not appropriate[*], many of
our tests and FS access code can be easier-to-write and more
readable using autodie as we've started doing in XapHelperCxx.pm
and xap_helper.t

[*] - EAGAIN on non-blocking I/O shouldn't die, nor should
      certain cases of opening maybe-missing files for reading
---
 INSTALL     | 1 +
 Makefile.PL | 1 +
 2 files changed, 2 insertions(+)

diff --git a/INSTALL b/INSTALL
index f5e14ebe..5f080f28 100644
--- a/INSTALL
+++ b/INSTALL
@@ -201,6 +201,7 @@ library.  Debian-based distros put them in "libperl5.$MINOR" or
 "perl-modules-5.$MINOR"; and FreeBSD puts them in "perl5".
 RPM-based distros split them out into separate packages:
 
+* autodie                          rpm: perl-autodie
 * Digest::SHA                      rpm: perl-Digest-SHA
 * Data::Dumper                     rpm: perl-Data-Dumper
 * Encode                           rpm: perl-Encode
diff --git a/Makefile.PL b/Makefile.PL
index d2309336..5a5628ba 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -150,6 +150,7 @@ WriteMakefile(
 
 		# perl-modules-5.xx or libperl5.xx in Debian-based
 		# part of "perl5" on FreeBSD
+		'autodie' => 0,
 		'Compress::Raw::Zlib' => 0,
 		'Compress::Zlib' => 0,
 		'Data::Dumper' => 0,

[PATCH 4/7] t/kqnotify: improve test reliability on OpenBSD

[Eric Wong]

Unlike FreeBSD, OpenBSD (tested 7.3) kevent doesn't document
EVFILT_VNODE behavior when directories are being watched.

Regardless, FreeBSD semantics appear to be mostly (if not
unreliably) supported.  Detecting rename(2) isn't reliable
at all and events seem to get lost and the test needs to
retry the rename(2) to succeed.  Fortunately, rename(2)
isn't recommended for Maildirs anyways since it can clobber
existing files.

link(2) detection appears to be merely delayed on OpenBSD,
so the test merely needs an occasional delay.
---
 t/kqnotify.t | 55 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/t/kqnotify.t b/t/kqnotify.t
index 902ce0f1..21381806 100644
--- a/t/kqnotify.t
+++ b/t/kqnotify.t
@@ -1,36 +1,63 @@
 #!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>
 #
 # Ensure KQNotify can pick up rename(2) and link(2) operations
 # used by Maildir writing tools
-use strict;
-use Test::More;
+use v5.12;
 use PublicInbox::TestCommon;
+use autodie;
 plan skip_all => 'KQNotify is only for *BSD systems' if $^O !~ /bsd/;
 require_mods('IO::KQueue');
 use_ok 'PublicInbox::KQNotify';
 my ($tmpdir, $for_destroy) = tmpdir();
-mkdir "$tmpdir/new" or BAIL_OUT "mkdir: $!";
-open my $fh, '>', "$tmpdir/tst" or BAIL_OUT "open: $!";
-close $fh or BAIL_OUT "close: $!";
+mkdir "$tmpdir/new";
 
 my $kqn = PublicInbox::KQNotify->new;
 my $mask = PublicInbox::KQNotify::MOVED_TO_OR_CREATE();
 my $w = $kqn->watch("$tmpdir/new", $mask);
 
-rename("$tmpdir/tst", "$tmpdir/new/tst") or BAIL_OUT "rename: $!";
+# Unlike FreeBSD, OpenBSD (tested 7.3) kevent NOTE_EXTEND doesn't detect
+# renames into directories reliably.  It's kevent(3) manpage doesn't
+# document this behavior (unlike FreeBSD), but it sometimes works...
+open my $fh, '>', "$tmpdir/tst";
+close $fh;
+rename("$tmpdir/tst", "$tmpdir/new/tst");
 my $hit = [ map { $_->fullname } $kqn->read ];
-is_deeply($hit, ["$tmpdir/new/tst"], 'rename(2) detected (via NOTE_EXTEND)');
+my $try = 0;
+while (!@$hit && $^O eq 'openbsd' && $try++ < 30) {
+	diag "retrying NOTE_EXTEND detection for $^O (#$try)";
+	# kevent can totally ignore the event, so delaying hasn't worked;
+	# keep doing the same thing until kevent notices one of them
+	open $fh, '>', "$tmpdir/tst";
+	close $fh;
+	rename("$tmpdir/tst", "$tmpdir/new/tst");
+	$hit = [ map { $_->fullname } $kqn->read ]
+}
+is_deeply($hit, ["$tmpdir/new/tst"],
+		'rename(2) detected (via NOTE_EXTEND)')
+		or diag explain($hit);
 
-open $fh, '>', "$tmpdir/tst" or BAIL_OUT "open: $!";
-close $fh or BAIL_OUT "close: $!";
-link("$tmpdir/tst", "$tmpdir/new/link") or BAIL_OUT "link: $!";
-$hit = [ grep m!/link$!, map { $_->fullname } $kqn->read ];
-is_deeply($hit, ["$tmpdir/new/link"], 'link(2) detected (via NOTE_WRITE)');
+# OpenBSD (tested 7.3) doesn't reliably trigger NOTE_WRITE on link(2)
+# into directories, but usually it does (and more reliably than rename(2)
+# above) and doesn't drop the event entirely.
+open $fh, '>', "$tmpdir/tst";
+close $fh;
+link("$tmpdir/tst", "$tmpdir/new/link");
+my @read = map { $_->fullname } $kqn->read;
+$try = 0;
+while (!@read && $^O eq 'openbsd' && $try++ < 30) {
+	diag "delaying and retrying NOTE_WRITE detection for $^O (#$try)";
+	tick;
+	# no need to link(2) again, at least, kevent can just be late, here
+	@read = map { $_->fullname } $kqn->read;
+}
+$hit = [ grep(m!/link$!, @read) ];
+is_deeply($hit, ["$tmpdir/new/link"], 'link(2) detected (via NOTE_WRITE)')
+	or diag explain(\@read);
 
 $w->cancel;
-link("$tmpdir/new/tst", "$tmpdir/new/link2") or BAIL_OUT "link: $!";
+link("$tmpdir/new/tst", "$tmpdir/new/link2");
 $hit = [ map { $_->fullname } $kqn->read ];
 is_deeply($hit, [], 'link(2) not detected after cancel');
 

[PATCH 5/7] xap_helper.h: don't compress debug sections on OpenBSD

[Eric Wong]

ld(1) on OpenBSD 7.3 doesn't appear to support zlib-compressed
debug sections out-of-the-box.  Oh well, being able to build
this C++ bit at all is required to get acceptable performance
with -cindex --associate.
---
 lib/PublicInbox/XapHelperCxx.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/XapHelperCxx.pm b/lib/PublicInbox/XapHelperCxx.pm
index 4571676b..a22dda1e 100644
--- a/lib/PublicInbox/XapHelperCxx.pm
+++ b/lib/PublicInbox/XapHelperCxx.pm
@@ -15,8 +15,10 @@ my $bin = "$dir/xap_helper";
 my ($srcpfx) = (__FILE__ =~ m!\A(.+/)[^/]+\z!);
 my @srcs = map { $srcpfx.$_ } qw(xap_helper.h);
 my @pm_dep = map { $srcpfx.$_ } qw(Search.pm CodeSearch.pm);
+my $ldflags = '-Wl,-O1';
+$ldflags .= ' -Wl,--compress-debug-sections=zlib' if $^O ne 'openbsd';
 my $xflags = ($ENV{CXXFLAGS} // '-Wall -ggdb3 -O0') . ' ' .
-	($ENV{LDFLAGS} // '-Wl,-O1 -Wl,--compress-debug-sections=zlib') .
+	($ENV{LDFLAGS} // $ldflags) .
 	qq{ -DTHREADID=}.PublicInbox::Search::THREADID;
 
 sub xflags_chg () {

[PATCH 6/7] xap_helper.h: limit stderr assignment to glibc+FreeBSD

[Eric Wong]

This fixes the C++ xap_helper compilation on OpenBSD.
Assignable `FILE *' pointers appear to only be supported on
FreeBSD and glibc.  Based on my reading of musl and NetBSD
source code, this should also fix builds on those platforms.
---
 lib/PublicInbox/xap_helper.h | 55 ++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/xap_helper.h b/lib/PublicInbox/xap_helper.h
index e10527d1..92210511 100644
--- a/lib/PublicInbox/xap_helper.h
+++ b/lib/PublicInbox/xap_helper.h
@@ -50,9 +50,18 @@
 #	define SET_MAX_EXPANSION set_max_wildcard_expansion
 #endif
 
+#if defined(__FreeBSD__) || defined(__GLIBC__)
+#	define STDERR_ASSIGNABLE (1)
+#else
+#	define STDERR_ASSIGNABLE (0)
+#endif
+
 static const int sock_fd = 0; // SOCK_SEQPACKET as stdin :P
 static pid_t parent_pid;
+#if STDERR_ASSIGNABLE
 static FILE *orig_err = stderr;
+#endif
+static int orig_err_fd = -1;
 static void *srch_tree; // tsearch + tdelete + twalk
 static pid_t *worker_pids; // nr => pid
 static unsigned long nworker;
@@ -820,6 +829,37 @@ static void cleanup_pids(void)
 	worker_pids = NULL;
 }
 
+static void stderr_set(FILE *tmp_err)
+{
+#if STDERR_ASSIGNABLE
+	if (my_setlinebuf(tmp_err))
+		perror("W: setlinebuf(tmp_err)");
+	stderr = tmp_err;
+	return;
+#endif
+	int fd = fileno(tmp_err);
+	if (fd < 0) err(EXIT_FAILURE, "BUG: fileno(tmp_err)");
+	while (dup2(fd, STDERR_FILENO) < 0) {
+		if (errno != EINTR)
+			err(EXIT_FAILURE, "dup2(%d => 2)", fd);
+	}
+}
+
+static void stderr_restore(FILE *tmp_err)
+{
+#if STDERR_ASSIGNABLE
+	stderr = orig_err;
+	return;
+#endif
+	if (ferror(stderr) | fflush(stderr))
+		perror("ferror|fflush stderr");
+	while (dup2(orig_err_fd, STDERR_FILENO) < 0) {
+		if (errno != EINTR)
+			err(EXIT_FAILURE, "dup2(%d => 2)", orig_err_fd);
+	}
+	clearerr(stderr);
+}
+
 static void recv_loop(void) // worker process loop
 {
 	static char rbuf[4096 * 33]; // per-process
@@ -828,18 +868,15 @@ static void recv_loop(void) // worker process loop
 		struct req req = {};
 		if (!recv_req(&req, rbuf, &len))
 			continue;
-		if (req.fp[1]) {
-			if (my_setlinebuf(req.fp[1]))
-				perror("W: setlinebuf(req.fp[1])");
-			stderr = req.fp[1];
-		}
+		if (req.fp[1])
+			stderr_set(req.fp[1]);
 		req.argc = (int)SPLIT2ARGV(req.argv, rbuf, len);
 		if (req.argc > 0)
 			dispatch(&req);
 		if (ferror(req.fp[0]) | fclose(req.fp[0]))
 			perror("ferror|fclose fp[0]");
 		if (req.fp[1]) {
-			stderr = orig_err;
+			stderr_restore(req.fp[1]);
 			if (ferror(req.fp[1]) | fclose(req.fp[1]))
 				perror("ferror|fclose fp[1]");
 		}
@@ -895,6 +932,12 @@ int main(int argc, char *argv[])
 	code_nrp_init();
 	atexit(cleanup_all);
 
+	if (!STDERR_ASSIGNABLE) {
+		orig_err_fd = dup(STDERR_FILENO);
+		if (orig_err_fd < 0)
+			err(EXIT_FAILURE, "dup(2)");
+	}
+
 	nworker = 0;
 #ifdef _SC_NPROCESSORS_ONLN
 	long j = sysconf(_SC_NPROCESSORS_ONLN);

[PATCH 7/7] xap_helper.h: fix double-free on OpenBSD hdestroy

[Eric Wong]

hdestroy on OpenBSD assumes each key in the table can be freed,
so use strdup to fulfil that requirement.

This behavior differs from tested behavior on glibc and FreeBSD,
as well as what I can see from reading the musl and NetBSD
source code.  OpenBSD may be the only relevant OS which requires
this workaround.
---
 lib/PublicInbox/xap_helper.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/xap_helper.h b/lib/PublicInbox/xap_helper.h
index 92210511..17085adc 100644
--- a/lib/PublicInbox/xap_helper.h
+++ b/lib/PublicInbox/xap_helper.h
@@ -460,6 +460,16 @@ static enum exc_iter dump_roots_iter(struct req *req,
 	return ITER_OK;
 }
 
+static char *hsearch_enter_key(char *s)
+{
+#if defined(__OpenBSD__) /* hdestroy frees each key */
+	char *ret = strdup(s);
+	if (!ret) perror("strdup");
+	return ret;
+#endif // glibc, musl, FreeBSD, NetBSD do not free keys
+	return s;
+}
+
 static bool cmd_dump_roots(struct req *req)
 {
 	CLEANUP_DUMP_ROOTS struct dump_roots_tmp drt = {};
@@ -511,7 +521,8 @@ static bool cmd_dump_roots(struct req *req)
 	}
 	for (size_t i = 0; i < tot; ) {
 		ENTRY e;
-		e.key = drt.entries[i++];
+		e.key = hsearch_enter_key(drt.entries[i++]);
+		if (!e.key) return false;
 		e.data = drt.entries[i++];
 		if (!hsearch(e, ENTER)) {
 			warn("hsearch(%s => %s, ENTER)", e.key,

Re: [PATCH 0/7] various build fixes + OpenBSD compat, [PATCH 7/7] xap_helper.h: fix double-free on OpenBSD hdestroy

[Štěpán Němec]

On Wed, 30 Aug 2023 05:10:38 +0000
Eric Wong wrote:

> 7/7 had me shaking my head for a bit, but I'd rather pay
> that price than introduce weirdness with C++ templates.

On Wed, 30 Aug 2023 05:10:45 +0000
Eric Wong wrote:

> hdestroy on OpenBSD assumes each key in the table can be freed,
> so use strdup to fulfil that requirement.
>
> This behavior differs from tested behavior on glibc and FreeBSD,
> as well as what I can see from reading the musl and NetBSD
> source code.  OpenBSD may be the only relevant OS which requires
> this workaround.

I guess this won't help any, but it caught my attention and I've already
done the digging, so I might as well share the findings:

OpenBSD actually copied this behavior with the initial import (of
hcreate.c) from NetBSD:

  $OpenBSD: hcreate.c,v 1.1 2004/06/24 04:43:33 millert Exp $
  https://github.com/openbsd/src/commit/46f8fdc8de36
  Commit: millert <millert@openbsd.org>
  CommitDate: Thu Jun 24 04:43:33 2004 +0000

  Working hcreate(3) et al from NetBSD (cgd) via ray at cyth dot
  net. Now passes the regress tests.

NetBSD only changed the default to match FreeBSD in 2014, and also added
hdestroy1 to allow caller control of element freeing:

  $NetBSD: hcreate.c,v 1.9 2014/07/20 13:34:17 christos Exp $
  https://github.com/netbsd/src/commit/3b1b85d301bf6e608ce20b4152
  Commit:     christos <christos@NetBSD.org>
  CommitDate: Sun Jul 20 13:34:17 2014 +0000

  Our hdestroy implementation was non-conformant because it freed the
  key of each entry. Add a new function hdestroy1 that allows the user
  to control what gets freed. Pointed out by Pedro Giffuni at FreeBSD.

That said, I'm not sure what "non-conformant" in the NetBSD commit
message refers to.  POSIX doesn't say anything on the matter
https://pubs.opengroup.org/onlinepubs/9699919799/functions/hcreate.html
and there don't seem to be any other relevant standards.

The NetBSD original (with the unconditional freeing) was apparently a
from-scratch implementation from 2001:

/*
 * hcreate() / hsearch() / hdestroy()
 *
 * SysV/XPG4 hash table functions.
 *
 * Implementation done based on NetBSD manual page and Solaris manual page,
 * plus my own personal experience about how they're supposed to work.
 *
 * I tried to look at Knuth (as cited by the Solaris manual page), but
 * nobody had a copy in the office, so...
 */

In any case, it does suck that OpenBSD is now the odd one out, but
having no experience with the interface myself, I have no opinion on
which behavior makes more sense.

Thanks for working on these (though I've had no complaints regarding the
actual functionality I need on OpenBSD so far) and let me know if you
want me to test something.  (With cindex.t out of the way (says 'ok'
now, plus a few W: reaped unknown PID), "make test" now gets stuck on
lei-import-http.t for me (with or without this series; still the same
test instance); I haven't seen kqnotify.t fail in the few runs I did.)

-- 
Štěpán

Re: [PATCH 0/7] various build fixes + OpenBSD compat, [PATCH 7/7] xap_helper.h: fix double-free on OpenBSD hdestroy

[Eric Wong]

Štěpán Němec <stepnem@smrk.net> wrote:

<snip> thanks for the additional notes.

> In any case, it does suck that OpenBSD is now the odd one out, but
> having no experience with the interface myself, I have no opinion on
> which behavior makes more sense.

Yeah, it's very limited and neglected interface that hardly
anybody uses; but it's all POSIX C offers.  Most C projects end
up implementing their own hash table or importing something
well-known (Attractive Chaos khash, ccan, gnulib, etc...).


I intend to use rculfhash from Userspace-RCU for the lei FUSE
component.  That'll likely be Linux-only since I can't bear
using FUSE without threads nor readdirplus (only in FUSE 3+)
when dealing with Maildirs.  I don't think FUSE 3 support exists
outside of Linux, yet...

> Thanks for working on these (though I've had no complaints regarding the
> actual functionality I need on OpenBSD so far) and let me know if you
> want me to test something.  (With cindex.t out of the way (says 'ok'
> now, plus a few W: reaped unknown PID), "make test" now gets stuck on
> lei-import-http.t for me (with or without this series; still the same
> test instance); I haven't seen kqnotify.t fail in the few runs I did.)

What's lei-import-http.t stuck on?  I haven't seen that...

Re: [PATCH 0/7] various build fixes + OpenBSD compat, [PATCH 7/7] xap_helper.h: fix double-free on OpenBSD hdestroy

[Štěpán Němec]

On Wed, 30 Aug 2023 21:18:49 +0000
Eric Wong wrote:

> What's lei-import-http.t stuck on?  I haven't seen that...

Have you tried without p5-Inline?  I installed it now and the test passes.

Without it, it just hangs here:

t/lei-import-http.t .......... 1/? # inherited [::1]:34584 fd=3

in

perl -httpd -W0 --stdout=/tmp/pi-lei-import-http-91165-_BAi/1 --stderr=/tmp/pi-lei-import-http-91165-_BAi/2

kdump -TR -m 100: (2.2MB)
https://smrk.net/tmp/lei-import-http.t.kdump

(the end after the 37950 mark is just me interrupting the process this
morning)

-- 
Štěpán

Re: [PATCH 0/7] various build fixes + OpenBSD compat, [PATCH 7/7] xap_helper.h: fix double-free on OpenBSD hdestroy

[Štěpán Němec]

On Thu, 31 Aug 2023 11:11:52 +0200
Štěpán Němec wrote:

> On Wed, 30 Aug 2023 21:18:49 +0000
> Eric Wong wrote:
>
>> What's lei-import-http.t stuck on?  I haven't seen that...
>
> Have you tried without p5-Inline?  I installed it now and the test passes.

Ah, I see now that PERFORMANCE NOTES in lei-overview.pod says Inline::C
is required, so perhaps just some adjustments to the test
skipping/diagnostics and INSTALL instructions (which say Inline::C is
optional) would be helpful?

-- 
Štěpán

Re: [PATCH 0/7] various build fixes + OpenBSD compat, [PATCH 7/7] xap_helper.h: fix double-free on OpenBSD hdestroy

[Eric Wong]

Štěpán Němec <stepnem@smrk.net> wrote:
> On Thu, 31 Aug 2023 11:11:52 +0200
> Štěpán Němec wrote:
> > On Wed, 30 Aug 2023 21:18:49 +0000
> > Eric Wong wrote:
> >
> >> What's lei-import-http.t stuck on?  I haven't seen that...
> >
> > Have you tried without p5-Inline?  I installed it now and the test passes.
> 
> Ah, I see now that PERFORMANCE NOTES in lei-overview.pod says Inline::C
> is required, so perhaps just some adjustments to the test
> skipping/diagnostics and INSTALL instructions (which say Inline::C is
> optional) would be helpful?

Actually, Inline::C is only needed for lei on *BSDs nowadays.
Common/available-on-cfarm Linux arches can rely on stable
syscall numbers to do FD passing.

The problems with the lei tests w/o Inline::C on *BSD is trying
to skip them too fast is more likely to trigger a race with
EVFILT_SIGNAL (but not Linux signalfd).  Still working on
patches to fix it and other subtle things around signal
handling...

Re: [PATCH 0/7] various build fixes + OpenBSD compat, [PATCH 7/7] xap_helper.h: fix double-free on OpenBSD hdestroy

[Štěpán Němec]

On Fri, 01 Sep 2023 11:09:03 +0000
Eric Wong wrote:

> Štěpán Němec <stepnem@smrk.net> wrote:
>> Ah, I see now that PERFORMANCE NOTES in lei-overview.pod says Inline::C
>> is required, so perhaps just some adjustments to the test
>> skipping/diagnostics and INSTALL instructions (which say Inline::C is
>> optional) would be helpful?
>
> Actually, Inline::C is only needed for lei on *BSDs nowadays.
> Common/available-on-cfarm Linux arches can rely on stable
> syscall numbers to do FD passing.

I see, thanks.  How about this then:

-- >8 --
Subject: [PATCH] Clarify Inline::C dependency (optional on Linux, required
 elsewhere)

Link: https://public-inbox.org/meta/20230901110903.M876537@dcvr/
Fixes: 88c7c7c26b44 ("lei: wire up pure Perl sendmsg/recvmsg for Linux users")
Fixes: acefd91b302d ("syscall: implement sendmsg+recvmsg in pure Perl")
---
 Documentation/lei-overview.pod | 12 +++++++-----
 INSTALL                        |  7 ++++---
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/lei-overview.pod b/Documentation/lei-overview.pod
index 7095b504cdb8..e9a97d64fb56 100644
--- a/Documentation/lei-overview.pod
+++ b/Documentation/lei-overview.pod
@@ -119,11 +119,13 @@ code repository.
 
 =head1 PERFORMANCE NOTES
 
-L<Inline::C> is required, lei runs as a background daemon to reduce
-startup costs and can provide real-time L<kqueue(2)>/L<inotify(7)>
-Maildir monitoring.  L<IO::KQueue> (p5-IO-KQueue on FreeBSD) and
-L<Linux::Inotify2> (liblinux-inotify2-perl and perl-Linux-Inotify2 in
-.deb and .rpm-based distros, respectively) are recommended.
+L<Inline::C> is required on BSDs and can speed things up on Linux.
+
+lei runs as a background daemon to reduce startup costs and can
+provide real-time L<kqueue(2)>/L<inotify(7)> Maildir monitoring.
+L<IO::KQueue> (p5-IO-KQueue on FreeBSD) and L<Linux::Inotify2>
+(liblinux-inotify2-perl and perl-Linux-Inotify2 in .deb and .rpm-based
+distros, respectively) are recommended.
 
 L<Socket::MsgHdr> is optional (libsocket-msghdr-perl in Debian),
 and further improves startup performance.  Its effect is most felt
diff --git a/INSTALL b/INSTALL
index 5f080f2889ce..e8d686d8f21b 100644
--- a/INSTALL
+++ b/INSTALL
@@ -60,7 +60,7 @@ Where "deb" indicates package names for Debian-derived distributions,
 "pkg" is for the FreeBSD package (maybe other common BSDs, too), and
 "rpm" is for RPM-based distributions (only known to work on Fedora).
 
-Numerous optional modules are likely to be useful as well:
+Numerous other modules are likely to be useful as well:
 
 - DBD::SQLite                      deb: libdbd-sqlite3-perl
                                    pkg: p5-DBD-SQLite
@@ -76,8 +76,9 @@ Numerous optional modules are likely to be useful as well:
 - Inline::C                        deb: libinline-c-perl
                                    pkg: p5-Inline-C
                                    rpm: perl-Inline (or perl-Inline-C)
-                                   (speeds up process spawning on Linux,
-                                    see public-inbox-daemon(8))
+                                   (required for lei on *BSD; speeds up process
+				    spawning on Linux, see
+				    public-inbox-daemon(8))
 
 - Email::Address::XS               deb: libemail-address-xs-perl
                                    pkg: p5-Email-Address-XS

-- 
2.42.0

[PATCH v2] Clarify Inline::C dependency (optional on Linux, required elsewhere)

[Štěpán Němec]

Link: https://public-inbox.org/meta/20230901110903.M876537@dcvr/
Fixes: 88c7c7c26b44 ("lei: wire up pure Perl sendmsg/recvmsg for Linux users")
Fixes: acefd91b302d ("syscall: implement sendmsg+recvmsg in pure Perl")
---
v2: expand tabs, sorry about that...

 Documentation/lei-overview.pod | 12 +++++++-----
 INSTALL                        |  7 ++++---
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/lei-overview.pod b/Documentation/lei-overview.pod
index 7095b504cdb8..e9a97d64fb56 100644
--- a/Documentation/lei-overview.pod
+++ b/Documentation/lei-overview.pod
@@ -119,11 +119,13 @@ code repository.
 
 =head1 PERFORMANCE NOTES
 
-L<Inline::C> is required, lei runs as a background daemon to reduce
-startup costs and can provide real-time L<kqueue(2)>/L<inotify(7)>
-Maildir monitoring.  L<IO::KQueue> (p5-IO-KQueue on FreeBSD) and
-L<Linux::Inotify2> (liblinux-inotify2-perl and perl-Linux-Inotify2 in
-.deb and .rpm-based distros, respectively) are recommended.
+L<Inline::C> is required on BSDs and can speed things up on Linux.
+
+lei runs as a background daemon to reduce startup costs and can
+provide real-time L<kqueue(2)>/L<inotify(7)> Maildir monitoring.
+L<IO::KQueue> (p5-IO-KQueue on FreeBSD) and L<Linux::Inotify2>
+(liblinux-inotify2-perl and perl-Linux-Inotify2 in .deb and .rpm-based
+distros, respectively) are recommended.
 
 L<Socket::MsgHdr> is optional (libsocket-msghdr-perl in Debian),
 and further improves startup performance.  Its effect is most felt
diff --git a/INSTALL b/INSTALL
index 5f080f2889ce..8f08a11f7ee1 100644
--- a/INSTALL
+++ b/INSTALL
@@ -60,7 +60,7 @@ Where "deb" indicates package names for Debian-derived distributions,
 "pkg" is for the FreeBSD package (maybe other common BSDs, too), and
 "rpm" is for RPM-based distributions (only known to work on Fedora).
 
-Numerous optional modules are likely to be useful as well:
+Numerous other modules are likely to be useful as well:
 
 - DBD::SQLite                      deb: libdbd-sqlite3-perl
                                    pkg: p5-DBD-SQLite
@@ -76,8 +76,9 @@ Numerous optional modules are likely to be useful as well:
 - Inline::C                        deb: libinline-c-perl
                                    pkg: p5-Inline-C
                                    rpm: perl-Inline (or perl-Inline-C)
-                                   (speeds up process spawning on Linux,
-                                    see public-inbox-daemon(8))
+                                   (required for lei on *BSD; speeds up process
+                                    spawning on Linux, see
+                                    public-inbox-daemon(8))
 
 - Email::Address::XS               deb: libemail-address-xs-perl
                                    pkg: p5-Email-Address-XS

-- 
2.42.0

Re: [PATCH v2] Clarify Inline::C dependency (optional on Linux, required elsewhere)

[Eric Wong]

Štěpán Němec <stepnem@smrk.net> wrote:
> +++ b/Documentation/lei-overview.pod
> @@ -119,11 +119,13 @@ code repository.
>  
>  =head1 PERFORMANCE NOTES
>  
> -L<Inline::C> is required, lei runs as a background daemon to reduce
> -startup costs and can provide real-time L<kqueue(2)>/L<inotify(7)>
> -Maildir monitoring.  L<IO::KQueue> (p5-IO-KQueue on FreeBSD) and
> -L<Linux::Inotify2> (liblinux-inotify2-perl and perl-Linux-Inotify2 in
> -.deb and .rpm-based distros, respectively) are recommended.
> +L<Inline::C> is required on BSDs and can speed things up on Linux.
> +
> +lei runs as a background daemon to reduce startup costs and can
> +provide real-time L<kqueue(2)>/L<inotify(7)> Maildir monitoring.
> +L<IO::KQueue> (p5-IO-KQueue on FreeBSD) and L<Linux::Inotify2>
> +(liblinux-inotify2-perl and perl-Linux-Inotify2 in .deb and .rpm-based
> +distros, respectively) are recommended.

Thanks, this hunk I agree with.

>  L<Socket::MsgHdr> is optional (libsocket-msghdr-perl in Debian),
>  and further improves startup performance.  Its effect is most felt
> diff --git a/INSTALL b/INSTALL
> index 5f080f2889ce..8f08a11f7ee1 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -60,7 +60,7 @@ Where "deb" indicates package names for Debian-derived distributions,
>  "pkg" is for the FreeBSD package (maybe other common BSDs, too), and
>  "rpm" is for RPM-based distributions (only known to work on Fedora).
>  
> -Numerous optional modules are likely to be useful as well:
> +Numerous other modules are likely to be useful as well:

What is the reasoning for this change?
"optional" is an important point to state, IMHO.
Using "other" is more ambiguous, I think.

> @@ -76,8 +76,9 @@ Numerous optional modules are likely to be useful as well:
>  - Inline::C                        deb: libinline-c-perl
>                                     pkg: p5-Inline-C
>                                     rpm: perl-Inline (or perl-Inline-C)
> -                                   (speeds up process spawning on Linux,
> -                                    see public-inbox-daemon(8))
> +                                   (required for lei on *BSD; speeds up process
> +                                    spawning on Linux, see
> +                                    public-inbox-daemon(8))

I think breaking up the lines on punctuation improves readability:

                                   (required for lei on *BSD;
                                    speeds up process spawning on Linux,
                                    see public-inbox-daemon(8))

I particularly dislike having "process" and "spawning"
being on separate lines.

Thanks.

Re: [PATCH v2] Clarify Inline::C dependency (optional on Linux, required elsewhere)

[Štěpán Němec]

On Sat, 02 Sep 2023 18:50:15 +0000
Eric Wong wrote:

> Štěpán Němec <stepnem@smrk.net> wrote:
>> -Numerous optional modules are likely to be useful as well:
>> +Numerous other modules are likely to be useful as well:
>
> What is the reasoning for this change?
> "optional" is an important point to state, IMHO.
> Using "other" is more ambiguous, I think.

We've now established that at least Inline::C is not optional on all
systems.  Is that really the only case?  (I mean, some of the modules
have "optional" again in their respective parentheticals: is that just
redundancy, or does it imply that the others are less optional?)  Even
if it _is_ the only case, isn't saying "optional" misleading?

>> @@ -76,8 +76,9 @@ Numerous optional modules are likely to be useful as well:
>>  - Inline::C                        deb: libinline-c-perl
>>                                     pkg: p5-Inline-C
>>                                     rpm: perl-Inline (or perl-Inline-C)
>> -                                   (speeds up process spawning on Linux,
>> -                                    see public-inbox-daemon(8))
>> +                                   (required for lei on *BSD; speeds up process
>> +                                    spawning on Linux, see
>> +                                    public-inbox-daemon(8))
>
> I think breaking up the lines on punctuation improves readability:
>
>                                    (required for lei on *BSD;
>                                     speeds up process spawning on Linux,
>                                     see public-inbox-daemon(8))

That's much better, thank you!

> I particularly dislike having "process" and "spawning"
> being on separate lines.

Yeah, it's horrible, but somehow your sane version didn't occur to me...

Thanks,

  Štěpán

Re: [PATCH v2] Clarify Inline::C dependency (optional on Linux, required elsewhere)

[Eric Wong]

Štěpán Němec <stepnem@smrk.net> wrote:
> On Sat, 02 Sep 2023 18:50:15 +0000
> Eric Wong wrote:
> 
> > Štěpán Němec <stepnem@smrk.net> wrote:
> >> -Numerous optional modules are likely to be useful as well:
> >> +Numerous other modules are likely to be useful as well:
> >
> > What is the reasoning for this change?
> > "optional" is an important point to state, IMHO.
> > Using "other" is more ambiguous, I think.
> 
> We've now established that at least Inline::C is not optional on all
> systems.  Is that really the only case?  (I mean, some of the modules
> have "optional" again in their respective parentheticals: is that just
> redundancy, or does it imply that the others are less optional?)  Even
> if it _is_ the only case, isn't saying "optional" misleading?

Perhaps the per-module "optional, for .." statements should be
"only for ...".

Aside from the new WIP -cindex; all public-inbox-* tools should
all work fine without Inline::C.  lei isn't likely running on
most public-facing servers.

In fact, all the components of public-inbox are optional
depending on which pieces you want to use it for.

The original public-inbox-* stuff (mda/learn/watch/WWW/httpd/CGI)
should work w/o SQLite for v1 inboxes, even (I need to test before
releases).

IOW, I'm trying to keep installations that were configured
and setup in back in 2014 working with no new dependencies.

Honestly, I wish I could tell users a C compiler is required to
make my life easier (heck, Varnish requires one); but gcc and
clang are both gigantic; and tcc (while active) hasn't had a
release in ages.

Thanks.

Re: [PATCH v2] Clarify Inline::C dependency (optional on Linux, required elsewhere)

[Štěpán Němec]

On Sat, 02 Sep 2023 19:44:07 +0000
Eric Wong wrote:

> Perhaps the per-module "optional, for .." statements should be
> "only for ...".

Well, if that's really true (i.e., they're not needed/useful for
anything else)...

I guess you see INSTALL as mainly for public-inbox without lei (and
indeed the first sentence is "This is for folks who want to set up their
own public-inbox instance."), so from that perspective saying "optional"
about something that is optional for public-inbox but required for lei
makes sense, but lei is then mentioned repeatedly in the dependencies
section, and it seems a safe bet there are much more people using lei
than there are people setting up public-inbox instances, so I'm not sure
maintaining that perspective in INSTALL is the best way to help those
users.

Then again, maybe it's just in my head and I might as well let such
users speak for themselves if they exist and ever wish to do so.

So here's just a reroll with your suggestions implemented:

-- 8< --
Subject: [PATCH v3] Clarify Inline::C dependency (optional on Linux, required
 elsewhere)

Link: https://public-inbox.org/meta/20230901110903.M876537@dcvr/
Link: https://public-inbox.org/meta/20230902194407.M464597@dcvr/
Fixes: 88c7c7c26b44 ("lei: wire up pure Perl sendmsg/recvmsg for Linux users")
Fixes: acefd91b302d ("syscall: implement sendmsg+recvmsg in pure Perl")
---
changes since v2:
- add another Link: to the parent of this message
- don't change "optional" to "other", focus is on public-inbox
- don't repeat "optional" in individual module items, say "only for"

 Documentation/lei-overview.pod | 12 +++++++-----
 INSTALL                        | 11 ++++++-----
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/Documentation/lei-overview.pod b/Documentation/lei-overview.pod
index 7095b504cdb8..e9a97d64fb56 100644
--- a/Documentation/lei-overview.pod
+++ b/Documentation/lei-overview.pod
@@ -119,11 +119,13 @@ code repository.
 
 =head1 PERFORMANCE NOTES
 
-L<Inline::C> is required, lei runs as a background daemon to reduce
-startup costs and can provide real-time L<kqueue(2)>/L<inotify(7)>
-Maildir monitoring.  L<IO::KQueue> (p5-IO-KQueue on FreeBSD) and
-L<Linux::Inotify2> (liblinux-inotify2-perl and perl-Linux-Inotify2 in
-.deb and .rpm-based distros, respectively) are recommended.
+L<Inline::C> is required on BSDs and can speed things up on Linux.
+
+lei runs as a background daemon to reduce startup costs and can
+provide real-time L<kqueue(2)>/L<inotify(7)> Maildir monitoring.
+L<IO::KQueue> (p5-IO-KQueue on FreeBSD) and L<Linux::Inotify2>
+(liblinux-inotify2-perl and perl-Linux-Inotify2 in .deb and .rpm-based
+distros, respectively) are recommended.
 
 L<Socket::MsgHdr> is optional (libsocket-msghdr-perl in Debian),
 and further improves startup performance.  Its effect is most felt
diff --git a/INSTALL b/INSTALL
index 5f080f2889ce..2e1c7ef7bb0a 100644
--- a/INSTALL
+++ b/INSTALL
@@ -76,7 +76,8 @@ Numerous optional modules are likely to be useful as well:
 - Inline::C                        deb: libinline-c-perl
                                    pkg: p5-Inline-C
                                    rpm: perl-Inline (or perl-Inline-C)
-                                   (speeds up process spawning on Linux,
+                                   (required for lei on *BSD;
+                                    speeds up process spawning on Linux,
                                     see public-inbox-daemon(8))
 
 - Email::Address::XS               deb: libemail-address-xs-perl
@@ -88,17 +89,17 @@ Numerous optional modules are likely to be useful as well:
 - Parse::RecDescent                deb: libparse-recdescent-perl
                                    pkg: p5-Parse-RecDescent
                                    rpm: perl-ParseRecDescent
-                                   (optional, for public-inbox-imapd(1))
+                                   (only for public-inbox-imapd(1))
 
 - Mail::IMAPClient                 deb: libmail-imapclient-perl
                                    pkg: p5-Mail-IMAPClient
                                    rpm: perl-Mail-IMAPClient
-                                   (optional for lei and public-inbox-watch)
+                                   (only for lei and public-inbox-watch)
 
 - BSD::Resource                    deb: libbsd-resource-perl
                                    pkg: p5-BSD-Resource
                                    rpm: perl-BSD-Resource
-                                   (optional, for PSGI limiters
+                                   (only for PSGI limiters,
                                     see public-inbox-config(5))
 
 - Plack::Middleware::ReverseProxy  deb: libplack-middleware-reverseproxy-perl
@@ -113,7 +114,7 @@ Numerous optional modules are likely to be useful as well:
 * xapian-compact (tool)            deb: xapian-tools
                                    pkg: xapian-core
                                    rpm: xapian-core
-                                   (optional, for public-inbox-compact(1))
+                                   (only for public-inbox-compact(1))
 
 * curl (tool)                      deb, pkg, rpm: curl
                                    (for HTTP(S) externals with curl)

-- 
2.42.0

Re: [PATCH v2] Clarify Inline::C dependency (optional on Linux, required elsewhere)

[Eric Wong]

Štěpán Němec <stepnem@smrk.net> wrote:
> On Sat, 02 Sep 2023 19:44:07 +0000
> Eric Wong wrote:
> 
> > Perhaps the per-module "optional, for .." statements should be
> > "only for ...".
> 
> Well, if that's really true (i.e., they're not needed/useful for
> anything else)...
> 
> I guess you see INSTALL as mainly for public-inbox without lei (and
> indeed the first sentence is "This is for folks who want to set up their
> own public-inbox instance."), so from that perspective saying "optional"
> about something that is optional for public-inbox but required for lei
> makes sense, but lei is then mentioned repeatedly in the dependencies
> section, and it seems a safe bet there are much more people using lei
> than there are people setting up public-inbox instances, so I'm not sure
> maintaining that perspective in INSTALL is the best way to help those
> users.

Perhaps a separate "INSTALL-lei" file is warranted.  lei is newer,
and parts of it still somewhat unrefined/experimental IMHO.

> Then again, maybe it's just in my head and I might as well let such
> users speak for themselves if they exist and ever wish to do so.

Thanks for the different perspective.  In my mind, public-inbox-mda
and PublicInbox::WWW were "first" and everything came after.

> So here's just a reroll with your suggestions implemented:
> 
> -- 8< --
> Subject: [PATCH v3] Clarify Inline::C dependency (optional on Linux, required
>  elsewhere)

Thanks, applied as commit 18c6c4e81dca1d81f6ff2bb284554b5fca2ef163