xap_helper: stricter and harsher error handling

We'll require an error stream for dump_ibx and dump_roots
commands; they're too important to ignore.  Instead of writing
code to provide diagnostics for errors, rely on abort(3) and the
-ggdb3 compiler flag to generate nice core dumps for gdb since
all commands sent to xap_helper are from internal users.
We'll even abort on most usage errors since they could be
bugs in split2argv or our use of getopt(3).

We'll also just exit on ENOMEM errors since it's the easiest way
to recover from those errors by starting a new process which
closes all open Xapian DB handles.

diff --git a/lib/PublicInbox/XapHelper.pm b/lib/PublicInbox/XapHelper.pm
index 4157600f6035b9125afc087989c85848095b8d1c..428b732e597084313d8d34d82076c20900b0c1fb 100644 (file)
--- a/lib/PublicInbox/XapHelper.pm
+++ b/lib/PublicInbox/XapHelper.pm
@@ -16,6 +16,7 @@ use PublicInbox::DS qw(awaitpid);
 use autodie qw(open getsockopt);
 use POSIX qw(:signal_h);
 use Fcntl qw(LOCK_UN LOCK_EX);
+use Carp qw(croak);
 my $X = \%PublicInbox::Search::X;
 our (%SRCH, %WORKERS, $nworker, $workerset, $in);
 our $stderr = \*STDERR;
@@ -70,14 +71,14 @@ sub dump_ibx_iter ($$$) {
 
 sub emit_mset_stats ($$) {
        my ($req, $mset) = @_;
-       my $err = $req->{1} or return;
+       my $err = $req->{1} or croak "BUG: caller only passed 1 FD";
        say $err 'mset.size='.$mset->size.' nr_out='.$req->{nr_out}
 }
 
 sub cmd_dump_ibx {
        my ($req, $ibx_id, $qry_str) = @_;
-       $qry_str // return warn('usage: dump_ibx [OPTIONS] IBX_ID QRY_STR');
-       $req->{A} or return warn('dump_ibx requires -A PREFIX');
+       $qry_str // die 'usage: dump_ibx [OPTIONS] IBX_ID QRY_STR';
+       $req->{A} or die 'dump_ibx requires -A PREFIX';
        my $max = $req->{'m'} // $req->{srch}->{xdb}->get_doccount;
        my $opt = { relevance => -1, limit => $max, offset => $req->{o} // 0 };
        $opt->{eidx_key} = $req->{O} if defined $req->{O};
@@ -88,9 +89,7 @@ sub cmd_dump_ibx {
                        $t = dump_ibx_iter($req, $ibx_id, $it) // $t;
                }
        }
-       if (my $err = $req->{1}) {
-               say $err 'mset.size='.$mset->size.' nr_out='.$req->{nr_out}
-       }
+       emit_mset_stats($req, $mset);
 }
 
 sub dump_roots_iter ($$$) {
@@ -120,9 +119,8 @@ sub dump_roots_flush ($$) {
 
 sub cmd_dump_roots {
        my ($req, $root2id_file, $qry_str) = @_;
-       $qry_str // return
-               warn('usage: dump_roots [OPTIONS] ROOT2ID_FILE QRY_STR');
-       $req->{A} or return warn('dump_roots requires -A PREFIX');
+       $qry_str // die 'usage: dump_roots [OPTIONS] ROOT2ID_FILE QRY_STR';
+       $req->{A} or die 'dump_roots requires -A PREFIX';
        open my $fh, '<', $root2id_file;
        my $root2id; # record format: $OIDHEX "\0" uint32_t
        my @x = split(/\0/, read_all $fh);
@@ -150,7 +148,7 @@ sub dispatch {
        my ($req, $cmd, @argv) = @_;
        my $fn = $req->can("cmd_$cmd") or return;
        $GLP->getoptionsfromarray(\@argv, $req, @SPEC) or return;
-       my $dirs = delete $req->{d} or return warn 'no -d args';
+       my $dirs = delete $req->{d} or die 'no -d args';
        my $key = join("\0", @$dirs);
        $req->{srch} = $SRCH{$key} //= do {
                my $new = { qp_flags => $PublicInbox::Search::QP_FLAGS };
@@ -168,8 +166,7 @@ sub dispatch {
                $new->{qp} = $new->qparse_new;
                $new;
        };
-       eval { $fn->($req, @argv) };
-       warn "E: $@" if $@;
+       $fn->($req, @argv);
 }
 
 sub recv_loop {
@@ -192,13 +189,10 @@ sub recv_loop {
                my $i = 0;
                open($req->{$i++}, '+<&=', $_) for @fds;
                local $stderr = $req->{1} // \*STDERR;
-               if (chop($rbuf) ne "\0") {
-                       warn "not NUL-terminated";
-                       next;
-               }
+               die "not NUL-terminated" if chop($rbuf) ne "\0";
                my @argv = split(/\0/, $rbuf);
                $req->{nr_out} = 0;
-               eval { $req->dispatch(@argv) } if @argv;
+               $req->dispatch(@argv) if @argv;
        }
 }
 

diff --git a/lib/PublicInbox/xap_helper.h b/lib/PublicInbox/xap_helper.h
index 8602170fb5aa9bab0e04edf0c81ef9a1153f02a0..1380ffd0d260c58e6dfed951c64f9e807573082e 100644 (file)
--- a/lib/PublicInbox/xap_helper.h
+++ b/lib/PublicInbox/xap_helper.h
@@ -78,6 +78,10 @@
        assert(ckvar______ == (expect) && "BUG" && __FILE__ && __LINE__); \
 } while (0)
 
+// coredump on most usage errors since our only users are internal
+#define ABORT(...) do { warnx(__VA_ARGS__); abort(); } while (0)
+#define EABORT(...) do { warn(__VA_ARGS__); abort(); } while (0)
+
 // sock_fd is modified in signal handler, yes, it's SOCK_SEQPACKET
 static volatile int sock_fd = STDIN_FILENO;
 static sigset_t fullset, workerset;
@@ -145,10 +149,8 @@ struct worker {
 #define SPLIT2ARGV(dst,buf,len) split2argv(dst,buf,len,MY_ARRAY_SIZE(dst))
 static size_t split2argv(char **dst, char *buf, size_t len, size_t limit)
 {
-       if (buf[0] == 0 || len == 0 || buf[len - 1] != 0) {
-               warnx("bogus argument given");
-               return 0;
-       }
+       if (buf[0] == 0 || len == 0 || buf[len - 1] != 0)
+               ABORT("bogus argument given");
        size_t nr = 0;
        char *c = buf;
        for (size_t i = 1; i < len; i++) {
@@ -156,11 +158,11 @@ static size_t split2argv(char **dst, char *buf, size_t len, size_t limit)
                        dst[nr++] = c;
                        c = buf + i + 1;
                }
-               if (nr == limit) {
-                       warnx("too many args: %zu", nr);
-                       return 0;
-               }
+               if (nr == limit)
+                       ABORT("too many args: %zu == %zu", nr, limit);
        }
+       if (nr == 0) ABORT("no argument given");
+       if ((long)nr < 0) ABORT("too many args: %zu", nr);
        return (long)nr;
 }
 
@@ -242,6 +244,8 @@ static void emit_mset_stats(struct req *req, const Xapian::MSet *mset)
        if (req->fp[1])
                fprintf(req->fp[1], "mset.size=%llu nr_out=%zu\n",
                        (unsigned long long)mset->size(), req->nr_out);
+       else
+               ABORT("BUG: %s caller only passed 1 FD", req->argv[0]);
 }
 
 static bool starts_with(const std::string *s, const char *pfx, size_t pfx_len)
@@ -290,20 +294,14 @@ static enum exc_iter dump_ibx_iter(struct req *req, const char *ibx_id,
 
 static bool cmd_dump_ibx(struct req *req)
 {
-       if ((optind + 1) >= req->argc) {
-               warnx("usage: dump_ibx [OPTIONS] IBX_ID QRY_STR");
-               return false; // need ibx_id + qry_str
-       }
-       if (!req->pfxc) {
-               warnx("dump_ibx requires -A PREFIX");
-               return false;
-       }
+       if ((optind + 1) >= req->argc)
+               ABORT("usage: dump_ibx [OPTIONS] IBX_ID QRY_STR");
+       if (!req->pfxc)
+               ABORT("dump_ibx requires -A PREFIX");
 
        const char *ibx_id = req->argv[optind];
-       if (my_setlinebuf(req->fp[0])) { // for sort(1) pipe
-               perror("setlinebuf(fp[0])");
-               return false;
-       }
+       if (my_setlinebuf(req->fp[0])) // for sort(1) pipe
+               EABORT("setlinebuf(fp[0])"); // WTF?
        req->asc = true;
        req->sort_col = -1;
        Xapian::MSet mset = mail_mset(req, req->argv[optind + 1]);
@@ -344,24 +342,22 @@ static void fbuf_ensure(void *ptr)
 {
        struct fbuf *fbuf = (struct fbuf *)ptr;
        if (fbuf->fp && fclose(fbuf->fp))
-               perror("fclose(fbuf->fp)");
+               err(EXIT_FAILURE, "fclose(fbuf->fp)"); // ENOMEM?
        fbuf->fp = NULL;
        free(fbuf->ptr);
 }
 
-static bool fbuf_init(struct fbuf *fbuf)
+static void fbuf_init(struct fbuf *fbuf)
 {
        assert(!fbuf->ptr);
        fbuf->fp = open_memstream(&fbuf->ptr, &fbuf->len);
-       if (fbuf->fp) return true;
-       perror("open_memstream(fbuf)");
-       return false;
+       if (!fbuf->fp) err(EXIT_FAILURE, "open_memstream(fbuf)");
 }
 
 static void xclose(int fd)
 {
        if (close(fd) < 0 && errno != EINTR)
-               err(EXIT_FAILURE, "BUG: close");
+               EABORT("BUG: close");
 }
 
 #define CLEANUP_DUMP_ROOTS __attribute__((__cleanup__(dump_roots_ensure)))
@@ -372,19 +368,17 @@ static void dump_roots_ensure(void *ptr)
                xclose(drt->root2id_fd);
        hdestroy(); // idempotent
        if (drt->mm_ptr && munmap(drt->mm_ptr, drt->sb.st_size))
-               err(EXIT_FAILURE, "BUG: munmap");
+               EABORT("BUG: munmap(%p, %zu)", drt->mm_ptr, drt->sb.st_size);
        free(drt->entries);
        fbuf_ensure(&drt->wbuf);
 }
 
 static bool root2ids_str(struct fbuf *root_ids, Xapian::Document *doc)
 {
-       if (!fbuf_init(root_ids)) return false;
-
-       bool ok = true;
        Xapian::TermIterator cur = doc->termlist_begin();
        Xapian::TermIterator end = doc->termlist_end();
        ENTRY e, *ep;
+       fbuf_init(root_ids);
        for (cur.skip_to("G"); cur != end; cur++) {
                std::string tn = *cur;
                if (!starts_with(&tn, "G", 1))
@@ -393,21 +387,16 @@ static bool root2ids_str(struct fbuf *root_ids, Xapian::Document *doc)
                u.in = tn.c_str() + 1;
                e.key = u.out;
                ep = hsearch(e, FIND);
-               if (!ep) {
-                       warnx("hsearch miss `%s'", e.key);
-                       return false;
-               }
+               if (!ep) ABORT("hsearch miss `%s'", e.key);
                // ep->data is a NUL-terminated string matching /[0-9]+/
                fputc(' ', root_ids->fp);
                fputs((const char *)ep->data, root_ids->fp);
        }
        fputc('\n', root_ids->fp);
-       if (ferror(root_ids->fp) | fclose(root_ids->fp)) {
-               perror("ferror|fclose(root_ids)");
-               ok = false;
-       }
+       if (ferror(root_ids->fp) | fclose(root_ids->fp))
+               err(EXIT_FAILURE, "ferror|fclose(root_ids)"); // ENOMEM
        root_ids->fp = NULL;
-       return ok;
+       return true;
 }
 
 // writes term values matching @pfx for a given @doc, ending the line
@@ -440,20 +429,17 @@ static bool dump_roots_flush(struct req *req, struct dump_roots_tmp *drt)
        bool ok = true;
 
        if (!drt->wbuf.fp) return true;
-       if (fd < 0) err(EXIT_FAILURE, "BUG: fileno");
-       if (fclose(drt->wbuf.fp)) {
-               warn("fclose(drt->wbuf.fp)"); // malloc failure?
-               return false;
-       }
+       if (fd < 0) EABORT("BUG: fileno");
+       if (ferror(drt->wbuf.fp) | fclose(drt->wbuf.fp)) // ENOMEM?
+               err(EXIT_FAILURE, "ferror|fclose(drt->wbuf.fp)");
        drt->wbuf.fp = NULL;
        if (!drt->wbuf.len) goto done_free;
        while (flock(drt->root2id_fd, LOCK_EX)) {
                if (errno == EINTR) continue;
-               perror("LOCK_EX");
-               return false;
+               err(EXIT_FAILURE, "LOCK_EX"); // ENOLCK?
        }
        p = drt->wbuf.ptr;
-       do {
+       do { // write to client FD
                ssize_t n = write(fd, p, drt->wbuf.len);
                if (n > 0) {
                        drt->wbuf.len -= n;
@@ -465,10 +451,9 @@ static bool dump_roots_flush(struct req *req, struct dump_roots_tmp *drt)
        } while (drt->wbuf.len);
        while (flock(drt->root2id_fd, LOCK_UN)) {
                if (errno == EINTR) continue;
-               perror("LOCK_UN");
-               return false;
+               err(EXIT_FAILURE, "LOCK_UN"); // ENOLCK?
        }
-done_free:
+done_free: // OK to skip on errors, dump_roots_ensure calls fbuf_ensure
        free(drt->wbuf.ptr);
        drt->wbuf.ptr = NULL;
        return ok;
@@ -501,7 +486,7 @@ static char *hsearch_enter_key(char *s)
        // hdestroy frees each key on some platforms,
        // so give it something to free:
        char *ret = strdup(s);
-       if (!ret) perror("strdup");
+       if (!ret) err(EXIT_FAILURE, "strdup");
        return ret;
 // AFAIK there's no way to detect musl, assume non-glibc Linux is musl:
 #elif defined(__GLIBC__) || defined(__linux__) || \
@@ -518,61 +503,42 @@ static bool cmd_dump_roots(struct req *req)
 {
        CLEANUP_DUMP_ROOTS struct dump_roots_tmp drt = {};
        drt.root2id_fd = -1;
-       if ((optind + 1) >= req->argc) {
-               warnx("usage: dump_roots [OPTIONS] ROOT2ID_FILE QRY_STR");
-               return false; // need file + qry_str
-       }
-       if (!req->pfxc) {
-               warnx("dump_roots requires -A PREFIX");
-               return false;
-       }
+       if ((optind + 1) >= req->argc)
+               ABORT("usage: dump_roots [OPTIONS] ROOT2ID_FILE QRY_STR");
+       if (!req->pfxc)
+               ABORT("dump_roots requires -A PREFIX");
        const char *root2id_file = req->argv[optind];
        drt.root2id_fd = open(root2id_file, O_RDONLY);
-       if (drt.root2id_fd < 0) {
-               warn("open(%s)", root2id_file);
-               return false;
-       }
-       if (fstat(drt.root2id_fd, &drt.sb)) {
-               warn("fstat(%s)", root2id_file);
-               return false;
-       }
+       if (drt.root2id_fd < 0)
+               EABORT("open(%s)", root2id_file);
+       if (fstat(drt.root2id_fd, &drt.sb)) // ENOMEM?
+               err(EXIT_FAILURE, "fstat(%s)", root2id_file);
        // each entry is at least 43 bytes ({OIDHEX}\0{INT}\0),
        // so /32 overestimates the number of expected entries by
        // ~%25 (as recommended by Linux hcreate(3) manpage)
        size_t est = (drt.sb.st_size / 32) + 1; //+1 for "\0" termination
-       if ((uint64_t)drt.sb.st_size > (uint64_t)SIZE_MAX) {
-               warnx("%s size too big (%lld bytes > %zu)", root2id_file,
-                       (long long)drt.sb.st_size, SIZE_MAX);
-               return false;
-       }
+       if ((uint64_t)drt.sb.st_size > (uint64_t)SIZE_MAX)
+               err(EXIT_FAILURE, "%s size too big (%lld bytes > %zu)",
+                       root2id_file, (long long)drt.sb.st_size, SIZE_MAX);
        drt.mm_ptr = mmap(NULL, drt.sb.st_size, PROT_READ,
                                MAP_PRIVATE, drt.root2id_fd, 0);
-       if (drt.mm_ptr == MAP_FAILED) {
-               warn("mmap(%s)", root2id_file);
-               return false;
-       }
+       if (drt.mm_ptr == MAP_FAILED)
+               err(EXIT_FAILURE, "mmap(%zu, %s)", drt.sb.st_size,root2id_file);
        drt.entries = (char **)calloc(est * 2, sizeof(char *));
-       if (!drt.entries) {
-               warn("calloc(%zu * 2, %zu)", est, sizeof(char *));
-               return false;
-       }
+       if (!drt.entries)
+               err(EXIT_FAILURE, "calloc(%zu * 2, %zu)", est, sizeof(char *));
        size_t tot = split2argv(drt.entries, (char *)drt.mm_ptr,
                                drt.sb.st_size, est * 2);
        if (tot <= 0) return false; // split2argv already warned on error
-       if (!hcreate(est)) {
-               warn("hcreate(%zu)", est);
-               return false;
-       }
+       if (!hcreate(est))
+               err(EXIT_FAILURE, "hcreate(%zu)", est);
        for (size_t i = 0; i < tot; ) {
                ENTRY e;
-               e.key = hsearch_enter_key(drt.entries[i++]);
-               if (!e.key) return false;
+               e.key = hsearch_enter_key(drt.entries[i++]); // dies on ENOMEM
                e.data = drt.entries[i++];
-               if (!hsearch(e, ENTER)) {
-                       warn("hsearch(%s => %s, ENTER)", e.key,
-                               (const char *)e.data);
-                       return false;
-               }
+               if (!hsearch(e, ENTER))
+                       err(EXIT_FAILURE, "hsearch(%s => %s, ENTER)", e.key,
+                                       (const char *)e.data);
        }
        req->asc = true;
        req->sort_col = -1;
@@ -581,8 +547,8 @@ static bool cmd_dump_roots(struct req *req)
        // @UNIQ_FOLD in CodeSearchIdx.pm can handle duplicate lines fine
        // in case we need to retry on DB reopens
        for (Xapian::MSetIterator i = mset.begin(); i != mset.end(); i++) {
-               if (!drt.wbuf.fp && !fbuf_init(&drt.wbuf))
-                       return false;
+               if (!drt.wbuf.fp)
+                       fbuf_init(&drt.wbuf);
                for (int t = 10; t > 0; --t)
                        switch (dump_roots_iter(req, &drt, &i)) {
                        case ITER_OK: t = 0; break; // leave inner loop
@@ -672,6 +638,8 @@ again:
 
        // success! no signals for the rest of the request/response cycle
        CHECK(int, 0, sigprocmask(SIG_SETMASK, &fullset, NULL));
+       if (r > 0 && msg.msg_flags)
+               ABORT("unexpected msg_flags");
 
        *len = r;
        if (cmsg.hdr.cmsg_level == SOL_SOCKET &&
@@ -806,7 +774,7 @@ static void dispatch(struct req *req)
                        break;
                }
        }
-       if (!req->fn) goto cmd_err;
+       if (!req->fn) ABORT("not handled: `%s'", req->argv[0]);
 
        kfp = open_memstream(&fbuf.ptr, &size);
        // write padding, first
@@ -825,53 +793,48 @@ static void dispatch(struct req *req)
                case 'd': fwrite(optarg, strlen(optarg) + 1, 1, kfp); break;
                case 'k':
                        req->sort_col = strtol(optarg, &end, 10);
-                       if (*end) goto cmd_err;
+                       if (*end) ABORT("-k %s", optarg);
                        switch (req->sort_col) {
-                       case LONG_MAX: case LONG_MIN: goto cmd_err;
+                       case LONG_MAX: case LONG_MIN: ABORT("-k %s", optarg);
                        }
                        break;
                case 'm':
                        req->max = strtoull(optarg, &end, 10);
-                       if (*end) goto cmd_err;
-                       if (req->max == ULLONG_MAX) goto cmd_err;
+                       if (*end || req->max == ULLONG_MAX)
+                               ABORT("-m %s", optarg);
                        break;
                case 'o':
                        req->off = strtoull(optarg, &end, 10);
-                       if (*end) goto cmd_err;
-                       if (req->off == ULLONG_MAX) goto cmd_err;
+                       if (*end || req->off == ULLONG_MAX)
+                               ABORT("-o %s", optarg);
                        break;
                case 'r': req->relevance = true; break;
                case 't': req->collapse_threads = true; break;
                case 'A':
                        req->pfxv[req->pfxc++] = optarg;
-                       if (MY_ARG_MAX == req->pfxc) goto cmd_err;
+                       if (MY_ARG_MAX == req->pfxc)
+                               ABORT("too many -A");
                        break;
                case 'O': req->Oeidx_key = optarg - 1; break; // pad "O" prefix
                case 'T':
                        req->timeout_sec = strtoul(optarg, &end, 10);
-                       if (*end) goto cmd_err;
-                       if (req->timeout_sec == ULONG_MAX) goto cmd_err;
+                       if (*end || req->timeout_sec == ULONG_MAX)
+                               ABORT("-T %s", optarg);
                        break;
-               default: goto cmd_err;
+               default: ABORT("bad switch `-%c'", c);
                }
        }
-       if (ferror(kfp) | fclose(kfp)) {
-               perror("ferror|fclose");
-               goto cmd_err;
-       }
+       if (ferror(kfp) | fclose(kfp))
+               err(EXIT_FAILURE, "ferror|fclose"); // likely ENOMEM
        fbuf.srch->db = NULL;
        fbuf.srch->qp = NULL;
        fbuf.srch->paths_len = size - offsetof(struct srch, paths);
        if (fbuf.srch->paths_len <= 0) {
                free_srch(fbuf.srch);
-               warnx("no -d args");
-               goto cmd_err;
+               ABORT("no -d args");
        }
        s = (struct srch **)tsearch(fbuf.srch, &srch_tree, srch_cmp);
-       if (!s) {
-               perror("tsearch");
-               goto cmd_err;
-       }
+       if (!s) err(EXIT_FAILURE, "tsearch"); // likely ENOMEM
        req->srch = *s;
        if (req->srch != fbuf.srch) { // reuse existing
                free_srch(fbuf.srch);
@@ -881,11 +844,11 @@ static void dispatch(struct req *req)
                void *del = tdelete(fbuf.srch, &srch_tree, srch_cmp);
                assert(del);
                free_srch(fbuf.srch);
-               goto cmd_err;
+               goto cmd_err; // srch_init already warned
        }
        try {
                if (!req->fn(req))
-                       goto cmd_err;
+                       warnx("`%s' failed", req->argv[0]);
        } catch (const Xapian::Error & e) {
                warnx("Xapian::Error: %s", e.get_description().c_str());
        } catch (...) {
@@ -924,7 +887,7 @@ static void stderr_restore(FILE *tmp_err)
        return;
 #endif
        if (ferror(stderr) | fflush(stderr))
-               perror("ferror|fflush stderr");
+               err(EXIT_FAILURE, "ferror|fflush stderr");
        while (dup2(orig_err_fd, STDERR_FILENO) < 0) {
                if (errno != EINTR)
                        err(EXIT_FAILURE, "dup2(%d => 2)", orig_err_fd);
@@ -953,8 +916,7 @@ static void recv_loop(void) // worker process loop
                if (req.fp[1])
                        stderr_set(req.fp[1]);
                req.argc = (int)SPLIT2ARGV(req.argv, rbuf, len);
-               if (req.argc > 0)
-                       dispatch(&req);
+               dispatch(&req);
                if (ferror(req.fp[0]) | fclose(req.fp[0]))
                        perror("ferror|fclose fp[0]");
                if (req.fp[1]) {
@@ -1066,7 +1028,7 @@ static void reaped_worker(pid_t pid, int st)
        if (WIFEXITED(st) && WEXITSTATUS(st) == EX_NOINPUT)
                alive = false;
        else if (st)
-               warnx("worker[%lu] died $?=%d", nr, st);
+               warnx("worker[%lu] died $?=%d alive=%d", nr, st, (int)alive);
        if (alive)
                start_workers();
 }

diff --git a/t/xap_helper.t b/t/xap_helper.t
index 83f59d7d70908cbc520dc8e0d065268a55c37506..9e0b234d9fb332da7deeadfffe4053ef63d105f5 100644 (file)
--- a/t/xap_helper.t
+++ b/t/xap_helper.t
@@ -47,11 +47,10 @@ is(scalar(@int), 1, 'have 1 internal shard') or diag explain(\@int);
 
 my $doreq = sub {
        my ($s, @arg) = @_;
-       my $err = pop @arg if ref($arg[-1]);
+       my $err = ref($arg[-1]) ? pop(@arg) : \*STDERR;
        pipe(my $x, my $y);
        my $buf = join("\0", @arg, '');
-       my @fds = fileno($y);
-       push @fds, fileno($err) if $err;
+       my @fds = (fileno($y), fileno($err));
        my $n = $PublicInbox::IPC::send_cmd->($s, \@fds, $buf, 0) //
                xbail "send: $!";
        my $exp = length($buf);