[PATCH] Implement -u (UNIX socket batch processing)

[PATCH] Implement -u (UNIX socket batch processing)

[Michael Stapelberg]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

Quoting from the commit message for your convenience:

> The option argument identifies the file descriptor number of a UNIX
> listening socket which was inherited from the parent process when said
> parent exec'd mandoc.
>
> mandoc will accept one connection at a time on that socket. As long as
> the connection lives, mandoc reads 1-byte messages with ancillary data
> (control information) from the connection. The ancillary data is used to
> pass three file descriptors from the parent process to mandoc. These
> file descriptors are then dup2'ed to fd 0, fd 1 and fd 2, respectively.
>
> After the file descriptors have been set up, a single manpage is read
> from stdin and processed to stdout/stderr.
>
> This change effectively adds a batch processing mode to mandoc, which
> eliminates the fork+exec overhead that otherwise accompanies the
> conversion of a large number of manpages. The resulting speed-up has
> been observed to exceed a factor of 6, reducing the total conversion
> wall-clock time of about a 470k manpages from more than 2 hours to
> a mere 22 minutes.

To expand on the rationale: quicker conversion…

1. allows us to provide our users more recent manpages (we can run
more frequently)
2. hogs fewer resources which can instead be used for actually serving manpages
3. massively simplifies our disaster recovery story: we can just
re-generate the entire repository within a small number of hours,
instead of facing days of downtime

As per http://man.openbsd.org/OpenBSD-current/man3/CMSG_DATA.3,
control information has been around since 4.2BSD, so support across
operating systems should be good.

No file system objects are required, no data format needs to be
defined and no additional library dependencies are added.

I have attached a minimal example implementation (demo.c) so that you
can convince yourself this approach works.

Please let me know what you think. Thanks!

-- 
Best regards,
Michael

[-- Attachment #2: demo.c --]
[-- Type: text/x-csrc, Size: 2820 bytes --]

#include <err.h>
#include <fcntl.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/un.h>
#include <unistd.h>

void run_mandoc(int sockfd) {
  char sockfdstr[10];
  if (snprintf(sockfdstr, sizeof(sockfdstr), "%d", sockfd) == -1)
    err(EXIT_FAILURE, "snprintf");
  if (execlp("mandoc", "mandoc", "-Thtml", "-u", sockfdstr, NULL) == -1)
    err(EXIT_FAILURE, "execlp(mandoc)");
}

ssize_t sock_fd_write(int fd, int fd0, int fd1, int fd2) {
  struct msghdr msg;
  struct iovec iov;
  union {
    struct cmsghdr cmsghdr;
    char control[CMSG_SPACE(3 * sizeof(int))];
  } cmsgu;
  struct cmsghdr *cmsg;
  unsigned char dummy[1] = {'\0'};

  iov.iov_base = dummy;
  iov.iov_len = sizeof(dummy);

  msg.msg_name = NULL;
  msg.msg_namelen = 0;
  msg.msg_iov = &iov;
  msg.msg_iovlen = 1;

  msg.msg_control = cmsgu.control;
  msg.msg_controllen = sizeof(cmsgu.control);

  cmsg = CMSG_FIRSTHDR(&msg);
  cmsg->cmsg_len = CMSG_LEN(3 * sizeof(int));
  cmsg->cmsg_level = SOL_SOCKET;
  cmsg->cmsg_type = SCM_RIGHTS;

  int *walk = (int *)CMSG_DATA(cmsg);
  *(walk++) = fd0;
  *(walk++) = fd1;
  *(walk++) = fd2;

  return sendmsg(fd, &msg, 0);
}

int process_manpage(struct sockaddr *addr) {
  int fd;

  if ((fd = socket(AF_LOCAL, SOCK_STREAM, 0)) < 0)
    return -1;

  if (connect(fd, addr, sizeof(struct sockaddr_un)) < 0)
    return -1;

  int pstdin[2];
  int pstdout[2];
  int pstderr[2];
  if (pipe(pstdin) == -1 || pipe(pstdout) == -1 || pipe(pstderr) == -1)
    return -1;

  if (sock_fd_write(fd, pstdin[0], pstdout[1], pstderr[1]) < 0)
    return -1;

  close(pstdin[0]);
  close(pstdout[1]);
  close(pstderr[1]);

  const char *man = ".TH example code\n";
  if (write(pstdin[1], man, strlen(man)) < 0)
    return -1;
  close(pstdin[1]);
  char buf[4096];
  ssize_t n = read(pstdout[0], buf, sizeof(buf));
  if (n < 0)
    return -1;
  printf("read: \"%.*s\"\n", (int)n, buf);
  return 0;
}

int setup_socket(struct sockaddr *addr) {
  int sockfd;

  if ((sockfd = socket(AF_LOCAL, SOCK_STREAM, 0)) < 0)
    return -1;

  if (bind(sockfd, addr, sizeof(struct sockaddr_un)) < 0)
    return -1;

  if (listen(sockfd, 1) < 0)
    return -1;

  return sockfd;
}

int main(int argc, char **argv) {
  struct sockaddr_un addr;
  memset(&addr, 0, sizeof(struct sockaddr_un));
  addr.sun_family = AF_LOCAL;

  int sockfd = setup_socket((struct sockaddr *)&addr);
  if (sockfd == -1)
    err(EXIT_FAILURE, "socket setup");

  pid_t pid = fork();
  switch (pid) {
  case -1:
    err(EXIT_FAILURE, "fork");
  case 0:
    run_mandoc(sockfd);
    break;
  default:
    if (process_manpage((struct sockaddr *)&addr) == -1)
      err(EXIT_FAILURE, "process_manpage");
    if (kill(pid, SIGKILL) == -1)
      err(EXIT_FAILURE, "kill(%d)", pid);
    break;
  }
}

[-- Attachment #3: 0001-Implement-u-UNIX-socket-batch-processing.patch --]
[-- Type: text/x-patch, Size: 5322 bytes --]

From f8663463a4c4b3cca212951499629bf8b2d4d42b Mon Sep 17 00:00:00 2001
From: Michael Stapelberg <stapelberg@debian.org>
Date: Tue, 17 Jan 2017 22:27:16 +0100
Subject: [PATCH] Implement -u (UNIX socket batch processing)

The option argument identifies the file descriptor number of a UNIX
listening socket which was inherited from the parent process when said
parent exec'd mandoc.

mandoc will accept one connection at a time on that socket. As long as
the connection lives, mandoc reads 1-byte messages with ancillary data
(control information) from the connection. The ancillary data is used to
pass three file descriptors from the parent process to mandoc. These
file descriptors are then dup2'ed to fd 0, fd 1 and fd 2, respectively.

After the file descriptors have been set up, a single manpage is read
from stdin and processed to stdout/stderr.

This change effectively adds a batch processing mode to mandoc, which
eliminates the fork+exec overhead that otherwise accompanies the
conversion of a large number of manpages. The resulting speed-up has
been observed to exceed a factor of 6, reducing the total conversion
wall-clock time of about a 470k manpages from more than 2 hours to
a mere 22 minutes.
---
 main.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 113 insertions(+), 1 deletion(-)

diff --git a/main.c b/main.c
index b64b3be..1225ace 100644
--- a/main.c
+++ b/main.c
@@ -41,6 +41,10 @@
 #include <time.h>
 #include <unistd.h>
 
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+
 #include "mandoc_aux.h"
 #include "mandoc.h"
 #include "roff.h"
@@ -109,6 +113,53 @@ static	char		 *help_argv[] = {help_arg, NULL};
 static	enum mandoclevel  rc;
 
 
+#define NUM_FDS 3
+static int read_fds(int clientfd, int *fds) {
+	struct msghdr	  msg;
+	unsigned char	  dummy[1];
+	struct iovec	  iov[1];
+	ssize_t		  n;
+	struct cmsghdr	  *cmsg;
+	int		  *walk;
+	int		  cnt;
+	/* Union used for alignment. */
+	union {
+		uint8_t controlbuf[CMSG_SPACE(NUM_FDS * sizeof(int))];
+		struct cmsghdr align;
+	} u;
+
+	memset(&msg, '\0', sizeof(msg));
+	msg.msg_control = u.controlbuf;
+	msg.msg_controllen = sizeof(u.controlbuf);
+	/* Read a dummy byte — sendmsg cannot send an empty message,
+	 * even if we are only interested in the OOB data. */
+	iov[0].iov_base = dummy;
+	iov[0].iov_len = sizeof(dummy);
+	msg.msg_iov = iov;
+	msg.msg_iovlen = 1;
+
+	if ((n = recvmsg(clientfd, &msg, 0)) < 0)
+		return n;
+
+	if ((cmsg = CMSG_FIRSTHDR(&msg)) == NULL)
+		return -1;
+
+	if (cmsg->cmsg_level != SOL_SOCKET ||
+	    cmsg->cmsg_type != SCM_RIGHTS)
+		return -1;
+
+	if (cmsg->cmsg_len != CMSG_LEN(NUM_FDS * sizeof(int)))
+		return -1;
+
+	walk = (int*)CMSG_DATA(cmsg);
+	for (cnt = 0; cnt < NUM_FDS; cnt++) {
+		fds[cnt] = *walk;
+		walk++;
+	}
+	return NUM_FDS;
+}
+
+
 int
 main(int argc, char *argv[])
 {
@@ -133,6 +184,16 @@ main(int argc, char *argv[])
 	int		 status, signum;
 	int		 c;
 	pid_t		 pager_pid, tc_pgid, man_pgid, pid;
+	int		 sockfd;
+	const char 	 *errstr;
+	struct sockaddr_un peer;
+	socklen_t	 len;
+	int		 clientfd;
+	int		 old_stdin;
+	int		 old_stdout;
+	int		 old_stderr;
+	int		 fds[3];
+	int		 n;
 
 #if HAVE_PROGNAME
 	progname = getprogname();
@@ -194,8 +255,11 @@ main(int argc, char *argv[])
 	show_usage = 0;
 	outmode = OUTMODE_DEF;
 
+	sockfd = -1;
+	errstr = NULL;
+
 	while (-1 != (c = getopt(argc, argv,
-			"aC:cfhI:iK:klM:m:O:S:s:T:VW:w"))) {
+			"aC:cfhI:iK:klM:m:O:S:s:T:u:VW:w"))) {
 		switch (c) {
 		case 'a':
 			outmode = OUTMODE_ALL;
@@ -261,6 +325,13 @@ main(int argc, char *argv[])
 			if ( ! toptions(&curp, optarg))
 				return (int)MANDOCLEVEL_BADARG;
 			break;
+		case 'u':
+			sockfd = strtonum(optarg, 0, INT_MAX, &errstr);
+			if (errstr) {
+				warnx("-u %s: %s", optarg, errstr);
+				return (int)MANDOCLEVEL_BADARG;
+			}
+			break;
 		case 'W':
 			if ( ! woptions(&curp, optarg))
 				return (int)MANDOCLEVEL_BADARG;
@@ -436,6 +507,47 @@ main(int argc, char *argv[])
 	mchars_alloc();
 	curp.mp = mparse_alloc(options, curp.wlevel, mmsg, defos);
 
+	/* If -u was specified, accept UNIX socket connections until killed. */
+	while (sockfd > 0) {
+		len = sizeof(struct sockaddr_un);
+		clientfd = accept(sockfd, (struct sockaddr *)&peer, &len);
+		if (clientfd < 0)
+			err((int)MANDOCLEVEL_SYSERR, "accept");
+
+		/* We always swap file descriptors so that we can always
+		 * unconditionally use their file descriptor numbers. */
+		fflush(stdout);
+		fflush(stderr);
+		old_stdin = dup(STDIN_FILENO);
+		old_stdout = dup(STDOUT_FILENO);
+		old_stderr = dup(STDERR_FILENO);
+
+		while (1) {
+			if ((n = read_fds(clientfd, fds)) < 0)
+				break;
+
+			if (dup2(fds[0], STDIN_FILENO) == -1 ||
+			    dup2(fds[1], STDOUT_FILENO) == -1 ||
+			    dup2(fds[2], STDERR_FILENO) == -1)
+				err((int)MANDOCLEVEL_SYSERR, "dup2");
+
+			close(fds[0]);
+			close(fds[1]);
+			close(fds[2]);
+
+			parse(&curp, STDIN_FILENO, "<unixfd>");
+			mparse_reset(curp.mp);
+			fflush(stdout);
+			fflush(stderr);
+			/* Close file descriptors by restoring the old ones. */
+			dup2(old_stdin, STDIN_FILENO);
+			dup2(old_stdout, STDOUT_FILENO);
+			dup2(old_stderr, STDERR_FILENO);
+		}
+
+		close(clientfd);
+	}
+
 	/*
 	 * Conditionally start up the lookaside buffer before parsing.
 	 */
-- 
2.11.0

Re: [PATCH] Implement -u (UNIX socket batch processing)

[Michael Stapelberg]

[-- Attachment #1: Type: text/plain, Size: 2299 bytes --]

Attaching revision 2 which resets ->gzip in mparse_reset(). I
previously did not link against libz, so this issue escaped me.

On Sat, Jan 21, 2017 at 1:29 PM, Michael Stapelberg
<stapelberg@debian.org> wrote:
> Quoting from the commit message for your convenience:
>
>> The option argument identifies the file descriptor number of a UNIX
>> listening socket which was inherited from the parent process when said
>> parent exec'd mandoc.
>>
>> mandoc will accept one connection at a time on that socket. As long as
>> the connection lives, mandoc reads 1-byte messages with ancillary data
>> (control information) from the connection. The ancillary data is used to
>> pass three file descriptors from the parent process to mandoc. These
>> file descriptors are then dup2'ed to fd 0, fd 1 and fd 2, respectively.
>>
>> After the file descriptors have been set up, a single manpage is read
>> from stdin and processed to stdout/stderr.
>>
>> This change effectively adds a batch processing mode to mandoc, which
>> eliminates the fork+exec overhead that otherwise accompanies the
>> conversion of a large number of manpages. The resulting speed-up has
>> been observed to exceed a factor of 6, reducing the total conversion
>> wall-clock time of about a 470k manpages from more than 2 hours to
>> a mere 22 minutes.
>
> To expand on the rationale: quicker conversion…
>
> 1. allows us to provide our users more recent manpages (we can run
> more frequently)
> 2. hogs fewer resources which can instead be used for actually serving manpages
> 3. massively simplifies our disaster recovery story: we can just
> re-generate the entire repository within a small number of hours,
> instead of facing days of downtime
>
> As per http://man.openbsd.org/OpenBSD-current/man3/CMSG_DATA.3,
> control information has been around since 4.2BSD, so support across
> operating systems should be good.
>
> No file system objects are required, no data format needs to be
> defined and no additional library dependencies are added.
>
> I have attached a minimal example implementation (demo.c) so that you
> can convince yourself this approach works.
>
> Please let me know what you think. Thanks!
>
> --
> Best regards,
> Michael



-- 
Best regards,
Michael

[-- Attachment #2: 0001-Implement-u-UNIX-socket-batch-processing.patch --]
[-- Type: text/x-patch, Size: 5554 bytes --]

From 6909bf9759e37d21e4016ffebef1e453a3abfc07 Mon Sep 17 00:00:00 2001
From: Michael Stapelberg <stapelberg@debian.org>
Date: Tue, 17 Jan 2017 22:27:16 +0100
Subject: [PATCH] Implement -u (UNIX socket batch processing)

The option argument identifies the file descriptor number of a UNIX
listening socket which was inherited from the parent process when said
parent exec'd mandoc.

mandoc will accept one connection at a time on that socket. As long as
the connection lives, mandoc reads 1-byte messages with ancillary data
(control information) from the connection. The ancillary data is used to
pass three file descriptors from the parent process to mandoc. These
file descriptors are then dup2'ed to fd 0, fd 1 and fd 2, respectively.

After the file descriptors have been set up, a single manpage is read
from stdin and processed to stdout/stderr.

This change effectively adds a batch processing mode to mandoc, which
eliminates the fork+exec overhead that otherwise accompanies the
conversion of a large number of manpages. The resulting speed-up has
been observed to exceed a factor of 6, reducing the total conversion
wall-clock time of about a 470k manpages from more than 2 hours to
a mere 22 minutes.
---
 main.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 read.c |   1 +
 2 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/main.c b/main.c
index b64b3be..1225ace 100644
--- a/main.c
+++ b/main.c
@@ -41,6 +41,10 @@
 #include <time.h>
 #include <unistd.h>
 
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+
 #include "mandoc_aux.h"
 #include "mandoc.h"
 #include "roff.h"
@@ -109,6 +113,53 @@ static	char		 *help_argv[] = {help_arg, NULL};
 static	enum mandoclevel  rc;
 
 
+#define NUM_FDS 3
+static int read_fds(int clientfd, int *fds) {
+	struct msghdr	  msg;
+	unsigned char	  dummy[1];
+	struct iovec	  iov[1];
+	ssize_t		  n;
+	struct cmsghdr	  *cmsg;
+	int		  *walk;
+	int		  cnt;
+	/* Union used for alignment. */
+	union {
+		uint8_t controlbuf[CMSG_SPACE(NUM_FDS * sizeof(int))];
+		struct cmsghdr align;
+	} u;
+
+	memset(&msg, '\0', sizeof(msg));
+	msg.msg_control = u.controlbuf;
+	msg.msg_controllen = sizeof(u.controlbuf);
+	/* Read a dummy byte — sendmsg cannot send an empty message,
+	 * even if we are only interested in the OOB data. */
+	iov[0].iov_base = dummy;
+	iov[0].iov_len = sizeof(dummy);
+	msg.msg_iov = iov;
+	msg.msg_iovlen = 1;
+
+	if ((n = recvmsg(clientfd, &msg, 0)) < 0)
+		return n;
+
+	if ((cmsg = CMSG_FIRSTHDR(&msg)) == NULL)
+		return -1;
+
+	if (cmsg->cmsg_level != SOL_SOCKET ||
+	    cmsg->cmsg_type != SCM_RIGHTS)
+		return -1;
+
+	if (cmsg->cmsg_len != CMSG_LEN(NUM_FDS * sizeof(int)))
+		return -1;
+
+	walk = (int*)CMSG_DATA(cmsg);
+	for (cnt = 0; cnt < NUM_FDS; cnt++) {
+		fds[cnt] = *walk;
+		walk++;
+	}
+	return NUM_FDS;
+}
+
+
 int
 main(int argc, char *argv[])
 {
@@ -133,6 +184,16 @@ main(int argc, char *argv[])
 	int		 status, signum;
 	int		 c;
 	pid_t		 pager_pid, tc_pgid, man_pgid, pid;
+	int		 sockfd;
+	const char 	 *errstr;
+	struct sockaddr_un peer;
+	socklen_t	 len;
+	int		 clientfd;
+	int		 old_stdin;
+	int		 old_stdout;
+	int		 old_stderr;
+	int		 fds[3];
+	int		 n;
 
 #if HAVE_PROGNAME
 	progname = getprogname();
@@ -194,8 +255,11 @@ main(int argc, char *argv[])
 	show_usage = 0;
 	outmode = OUTMODE_DEF;
 
+	sockfd = -1;
+	errstr = NULL;
+
 	while (-1 != (c = getopt(argc, argv,
-			"aC:cfhI:iK:klM:m:O:S:s:T:VW:w"))) {
+			"aC:cfhI:iK:klM:m:O:S:s:T:u:VW:w"))) {
 		switch (c) {
 		case 'a':
 			outmode = OUTMODE_ALL;
@@ -261,6 +325,13 @@ main(int argc, char *argv[])
 			if ( ! toptions(&curp, optarg))
 				return (int)MANDOCLEVEL_BADARG;
 			break;
+		case 'u':
+			sockfd = strtonum(optarg, 0, INT_MAX, &errstr);
+			if (errstr) {
+				warnx("-u %s: %s", optarg, errstr);
+				return (int)MANDOCLEVEL_BADARG;
+			}
+			break;
 		case 'W':
 			if ( ! woptions(&curp, optarg))
 				return (int)MANDOCLEVEL_BADARG;
@@ -436,6 +507,47 @@ main(int argc, char *argv[])
 	mchars_alloc();
 	curp.mp = mparse_alloc(options, curp.wlevel, mmsg, defos);
 
+	/* If -u was specified, accept UNIX socket connections until killed. */
+	while (sockfd > 0) {
+		len = sizeof(struct sockaddr_un);
+		clientfd = accept(sockfd, (struct sockaddr *)&peer, &len);
+		if (clientfd < 0)
+			err((int)MANDOCLEVEL_SYSERR, "accept");
+
+		/* We always swap file descriptors so that we can always
+		 * unconditionally use their file descriptor numbers. */
+		fflush(stdout);
+		fflush(stderr);
+		old_stdin = dup(STDIN_FILENO);
+		old_stdout = dup(STDOUT_FILENO);
+		old_stderr = dup(STDERR_FILENO);
+
+		while (1) {
+			if ((n = read_fds(clientfd, fds)) < 0)
+				break;
+
+			if (dup2(fds[0], STDIN_FILENO) == -1 ||
+			    dup2(fds[1], STDOUT_FILENO) == -1 ||
+			    dup2(fds[2], STDERR_FILENO) == -1)
+				err((int)MANDOCLEVEL_SYSERR, "dup2");
+
+			close(fds[0]);
+			close(fds[1]);
+			close(fds[2]);
+
+			parse(&curp, STDIN_FILENO, "<unixfd>");
+			mparse_reset(curp.mp);
+			fflush(stdout);
+			fflush(stderr);
+			/* Close file descriptors by restoring the old ones. */
+			dup2(old_stdin, STDIN_FILENO);
+			dup2(old_stdout, STDOUT_FILENO);
+			dup2(old_stderr, STDERR_FILENO);
+		}
+
+		close(clientfd);
+	}
+
 	/*
 	 * Conditionally start up the lookaside buffer before parsing.
 	 */
diff --git a/read.c b/read.c
index d20a609..d96eb5c 100644
--- a/read.c
+++ b/read.c
@@ -836,6 +836,7 @@ mparse_reset(struct mparse *curp)
 
 	free(curp->sodest);
 	curp->sodest = NULL;
+	curp->gzip = 0;
 }
 
 void
-- 
2.11.0

Re: [PATCH] Implement -u (UNIX socket batch processing)

[Ingo Schwarze]

Hi Michael,

Michael Stapelberg wrote on Sat, Jan 28, 2017 at 02:02:28PM +0100:

> Attaching revision 2 which resets ->gzip in mparse_reset(). I
> previously did not link against libz, so this issue escaped me.

independent of your added code, this part of your patch is
correct.  You found a genuine bug here, so i committed that
part of your patch, see below.

Thanks,
  Ingo


Log Message:
-----------
If an application parses multiple files with mparse_readfd(3) but 
without using mparse_open(3) to open the files, and if one of the
files includes a gzip'ed file with .so, then the gzip flag remains
set and the next main file will be expected to be gzip'ed.
Fix this by clearing the gzip flag in mparse_reset(3).

Bug found and patch provided by Michael <Stapelberg at debian dot org>.

Modified Files:
--------------
    mdocml:
        read.c

Revision Data
-------------
Index: read.c
===================================================================
RCS file: /home/cvs/mdocml/mdocml/read.c,v
retrieving revision 1.158
retrieving revision 1.159
diff -Lread.c -Lread.c -u -p -r1.158 -r1.159
--- read.c
+++ read.c
@@ -837,6 +837,7 @@ mparse_reset(struct mparse *curp)
 
 	free(curp->sodest);
 	curp->sodest = NULL;
+	curp->gzip = 0;
 }
 
 void
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: [PATCH] Implement -u (UNIX socket batch processing)

[Ingo Schwarze]

Hi Michael,

On Sat, Jan 21, 2017 at 1:29 PM, Michael Stapelberg wrote:
> Quoting from the commit message for your convenience:

>> The option argument identifies the file descriptor number of a UNIX
>> listening socket which was inherited from the parent process when said
>> parent exec'd mandoc.
>>
>> mandoc will accept one connection at a time on that socket. As long as
>> the connection lives, mandoc reads 1-byte messages with ancillary data
>> (control information) from the connection. The ancillary data is used to
>> pass three file descriptors from the parent process to mandoc. These
>> file descriptors are then dup2'ed to fd 0, fd 1 and fd 2, respectively.
>>
>> After the file descriptors have been set up, a single manpage is read
>> from stdin and processed to stdout/stderr.
>>
>> This change effectively adds a batch processing mode to mandoc, which
>> eliminates the fork+exec overhead that otherwise accompanies the
>> conversion of a large number of manpages. The resulting speed-up has
>> been observed to exceed a factor of 6, reducing the total conversion
>> wall-clock time of about a 470k manpages from more than 2 hours to
>> a mere 22 minutes.

> To expand on the rationale: quicker conversion
>
> 1. allows us to provide our users more recent manpages (we can run
> more frequently)
> 2. hogs fewer resources which can instead be used for actually serving
> manpages
> 3. massively simplifies our disaster recovery story: we can just
> re-generate the entire repository within a small number of hours,
> instead of facing days of downtime

I think i understand why you need this.  I repeatedly thought about
it and came to the conclusion that there is little to improve with
the basic concept.  In particular, opening the file descriptors,
in particular those for writing, in the controlling process is the
right thing to do.  We do not want the same process that is doing
the parsing and formatting to open files for writing.  That would
be unsafe in case of programming errors, and parsers are notoriously
buggy.  So your original idea of passing the descriptors is clearly
better than my first idea of passing filenames instead.

> As per http://man.openbsd.org/OpenBSD-current/man3/CMSG_DATA.3,
> control information has been around since 4.2BSD, so support across
> operating systems should be good.

Yes, but i don't like the idea of clobbering that into main.c.
At some point, i will have to do privilege separation in mandoc.
That will almost certainly use the OpenBSD imsg library, which
internally uses cmsg.  It will not be a protability problem: imsg
is very small and can easily be bundled for systems not having it,
exactly like ohash.

But if i would commit your code now, it would conflict later, using
cmsg on two different levels.  Besides, even though your code is
required for your purpose, and even though it may also help speeding
up bulk comparisons that's i'm sometimes doing to catch regressions,
it is not useful for the general public, so it does not belong into
main.c.  Just like the code in cgi.c, only a handful of server
administrators will need it.

Besides, for the daemon you need, main.c is mostly the wrong codebase.
It contains huge amounts of code that, in your server, are just in
the way: many command line options for interactive use, makewhatis(8)
integration, database and search functionality, pager and process
group control, file system lookup, preformatted passthrough...

So your code should be in a separate program.  I have called it
mandocd.c.  It now has 270 lines (compare to the 1108 in main.c),
your read_fds() is the largest non-main function, and even in main(),
the meat of the code is what you wrote, plus relatively small glue.


There was one major problem with what you wrote.  You used abstract
sockets, and that is non-portable and Linux only.  On other systems,
you cannot call bind(2) with a name starting in '\0'.  To be portable,
you have to create the socket with socketpair(2) and without bind(2),
listen(2), and accept(2).  The fact that you cannot reconnect when
you lose the connection may seem like a downside at first, but i don't
think it's really any inconvenience.  If the controlling process
wants to reconnect for whatever reason, it can just as well spawn
a new mandocd.  I'm not even sure that your client code contained 
support for reconnecting, even though your server side supported it.

If you absolutely need reconnection capability, we have to use a
named socket in the file system.  But i'd rather not do that if we
can avoid it.  It just causes security grief because we would have
to protect the socket against unauthorized access.

I committed new files mandocd.c and catman.c to bsd.lv such that
we can work on them in the tree.  catman.c is an improved version
of your demo.c that actually does something useful: Try making
directories src/ and dst/, put some mdoc(7) and/or man(7) manual
pages into src/, and run

  ./catman src dst

or

  ./catman -T html src dst

Formatted versions appear in dst/, with the same relative file
names.  Of course, this is not polished.  For example, if you use
any subdirs in src/, you have to create the same subdirs in dst/
by hand - i'll probably fix that, and other quirks, later.

You didn't have a license on demo.c, but i assume you intended to
license it under the same license as your patch to main.c.
Please review the Copyright/license headers in the two files
and protest if i misunderstood what you intended.


The important questions right now are:

 * Does the concept of a stand-alone file mandocd.c
   and a "mandocd" executable separate from "mandoc"
   work for you?

 * Does using socketpair(2) instead of socket(2), bind(2), listen(2),
   accept(2) work for you?

Of course, any other comments on the committed code are also
welcome.

Right now, i don't plan to merge this back to VERSION_1_13.
You don't need SQLite3 support anyway, so you should be fine
switching to mandoc-1.14.1 when it comes out.

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: [PATCH] Implement -u (UNIX socket batch processing)

[Michael Stapelberg]

[-- Attachment #1: Type: text/plain, Size: 7167 bytes --]

Thanks for the thoughtful response, replies inline:

On Sat, Feb 4, 2017 at 1:57 PM, Ingo Schwarze <schwarze@usta.de> wrote:
> Hi Michael,
>
> On Sat, Jan 21, 2017 at 1:29 PM, Michael Stapelberg wrote:
>> Quoting from the commit message for your convenience:
>
>>> The option argument identifies the file descriptor number of a UNIX
>>> listening socket which was inherited from the parent process when said
>>> parent exec'd mandoc.
>>>
>>> mandoc will accept one connection at a time on that socket. As long as
>>> the connection lives, mandoc reads 1-byte messages with ancillary data
>>> (control information) from the connection. The ancillary data is used to
>>> pass three file descriptors from the parent process to mandoc. These
>>> file descriptors are then dup2'ed to fd 0, fd 1 and fd 2, respectively.
>>>
>>> After the file descriptors have been set up, a single manpage is read
>>> from stdin and processed to stdout/stderr.
>>>
>>> This change effectively adds a batch processing mode to mandoc, which
>>> eliminates the fork+exec overhead that otherwise accompanies the
>>> conversion of a large number of manpages. The resulting speed-up has
>>> been observed to exceed a factor of 6, reducing the total conversion
>>> wall-clock time of about a 470k manpages from more than 2 hours to
>>> a mere 22 minutes.
>
>> To expand on the rationale: quicker conversion
>>
>> 1. allows us to provide our users more recent manpages (we can run
>> more frequently)
>> 2. hogs fewer resources which can instead be used for actually serving
>> manpages
>> 3. massively simplifies our disaster recovery story: we can just
>> re-generate the entire repository within a small number of hours,
>> instead of facing days of downtime
>
> I think i understand why you need this.  I repeatedly thought about
> it and came to the conclusion that there is little to improve with
> the basic concept.  In particular, opening the file descriptors,
> in particular those for writing, in the controlling process is the
> right thing to do.  We do not want the same process that is doing
> the parsing and formatting to open files for writing.  That would
> be unsafe in case of programming errors, and parsers are notoriously
> buggy.  So your original idea of passing the descriptors is clearly
> better than my first idea of passing filenames instead.
>
>> As per http://man.openbsd.org/OpenBSD-current/man3/CMSG_DATA.3,
>> control information has been around since 4.2BSD, so support across
>> operating systems should be good.
>
> Yes, but i don't like the idea of clobbering that into main.c.
> At some point, i will have to do privilege separation in mandoc.
> That will almost certainly use the OpenBSD imsg library, which
> internally uses cmsg.  It will not be a protability problem: imsg
> is very small and can easily be bundled for systems not having it,
> exactly like ohash.
>
> But if i would commit your code now, it would conflict later, using
> cmsg on two different levels.  Besides, even though your code is
> required for your purpose, and even though it may also help speeding
> up bulk comparisons that's i'm sometimes doing to catch regressions,
> it is not useful for the general public, so it does not belong into
> main.c.  Just like the code in cgi.c, only a handful of server
> administrators will need it.
>
> Besides, for the daemon you need, main.c is mostly the wrong codebase.
> It contains huge amounts of code that, in your server, are just in
> the way: many command line options for interactive use, makewhatis(8)
> integration, database and search functionality, pager and process
> group control, file system lookup, preformatted passthrough...
>
> So your code should be in a separate program.  I have called it
> mandocd.c.  It now has 270 lines (compare to the 1108 in main.c),
> your read_fds() is the largest non-main function, and even in main(),
> the meat of the code is what you wrote, plus relatively small glue.

That looks good, thanks for merging the patch!

>
>
> There was one major problem with what you wrote.  You used abstract
> sockets, and that is non-portable and Linux only.  On other systems,
> you cannot call bind(2) with a name starting in '\0'.  To be portable,
> you have to create the socket with socketpair(2) and without bind(2),
> listen(2), and accept(2).  The fact that you cannot reconnect when
> you lose the connection may seem like a downside at first, but i don't
> think it's really any inconvenience.  If the controlling process
> wants to reconnect for whatever reason, it can just as well spawn
> a new mandocd.  I'm not even sure that your client code contained
> support for reconnecting, even though your server side supported it.

Agreed. I added reconnects in the early stages of development of this
patch, when I was still using a named socket. With the feature in its
current form, I agree that we do not need reconnects.

>
> If you absolutely need reconnection capability, we have to use a
> named socket in the file system.  But i'd rather not do that if we
> can avoid it.  It just causes security grief because we would have
> to protect the socket against unauthorized access.
>
> I committed new files mandocd.c and catman.c to bsd.lv such that
> we can work on them in the tree.  catman.c is an improved version
> of your demo.c that actually does something useful: Try making
> directories src/ and dst/, put some mdoc(7) and/or man(7) manual
> pages into src/, and run
>
>   ./catman src dst
>
> or
>
>   ./catman -T html src dst
>
> Formatted versions appear in dst/, with the same relative file
> names.  Of course, this is not polished.  For example, if you use
> any subdirs in src/, you have to create the same subdirs in dst/
> by hand - i'll probably fix that, and other quirks, later.
>
> You didn't have a license on demo.c, but i assume you intended to
> license it under the same license as your patch to main.c.

Yes, you are correct.

> Please review the Copyright/license headers in the two files
> and protest if i misunderstood what you intended.
>
>
> The important questions right now are:
>
>  * Does the concept of a stand-alone file mandocd.c
>    and a "mandocd" executable separate from "mandoc"
>    work for you?

Absolutely.

>
>  * Does using socketpair(2) instead of socket(2), bind(2), listen(2),
>    accept(2) work for you?

Yes.

I went ahead and changed debiman to use this interface (and I realize
you still marked it as experimental, so I’ll gladly update when
necessary): https://github.com/Debian/debiman/commit/3715b1eaf9c1793b9a8c7b1787e2d6511ca2b004

>
> Of course, any other comments on the committed code are also
> welcome.

I added a tiny patch which includes stdint.h which is required to find
uint8_t on Linux (fixes compilation of mandocd).

>
> Right now, i don't plan to merge this back to VERSION_1_13.
> You don't need SQLite3 support anyway, so you should be fine
> switching to mandoc-1.14.1 when it comes out.

Yep, sounds good.

-- 
Best regards,
Michael

[-- Attachment #2: 0001-mandocd-add-stdint.h-to-fix-compilation-on-Linux.patch --]
[-- Type: text/x-patch, Size: 532 bytes --]

From ed80ba44eedd0543f9da9b1e231204262121fbd8 Mon Sep 17 00:00:00 2001
From: Michael Stapelberg <stapelberg@debian.org>
Date: Sun, 5 Feb 2017 15:39:46 +0100
Subject: [PATCH] mandocd: add stdint.h to fix compilation on Linux

---
 mandocd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mandocd.c b/mandocd.c
index fce2812..406e991 100644
--- a/mandocd.c
+++ b/mandocd.c
@@ -28,6 +28,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <stdint.h>
 
 #include "mandoc.h"
 #include "roff.h"
-- 
2.11.0