cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery

[Steffen Nurpmeso]

Once more, hey,

Steffen Nurpmeso wrote in
 <20201222150957._aAW5%steffen at sdaoden.eu>:
 |Steffen Nurpmeso wrote in
 | <20201222135537.ovyl4%steffen at sdaoden.eu>:
 ||gs-cgit-lists.zx2c4.com at gluelogic.com wrote in
 || <20201222061206.GA54419 at xps13>:
 |||>Steffen Nurpmeso wrote in
 |||> <20201221193127.zbZeP%steffen at sdaoden.eu>:
 |||>|John Keeping wrote in
 |||>| <X+DiDgGaPaynnocI at john.keeping.me.uk>:
 |||>||On Mon, Dec 21, 2020 at 05:26:19PM +0100, Steffen Nurpmeso wrote:
 |||>||> I discovered today that cgit no longer delivers pages, and it must
 |||>||> have been like that for some time.  The server looks show
 |||>||> successful delivery, the cgit cache is populated and rotated just
 |||>||> correctly, but all cgit delivers is that final error of main() as
 ...
 |||>||> I am pretty sure cgit delivered some weeks ago, the most notable
 |||>||> difference is that AlpineLinux switched to Lighttpd 1.4.56 then
 |||>||> .57, which seems to have brought tremendous changes under the
 |||> ...
 |||>|But the file was generated normally:
 ...
 |||>||and this may be caused by sendfile(2) failing due to some difference \
 |||>||in
 |||>||how the web server is setting up the output file descriptor.  You may
 ...

Looking at lighttpd commit history it seems he now uses splice(2)
for CGI if possible.

  ...
 |||I would like to propose an alternative and more portable solution:
 |||
 |||cgit cache should fallback to lseek, xread, xwrite if sendfile fails.

It would be cool if the sendfile(2) could be avoided via
configuration.  It seems so wasteful to always have that
system-call, context-switch and such.
What do you think of (on [master] aka
adcc4f822fe11836e5f942fc1ae0f00db4eb8d5f plus the other patch),
the following runs on my server VM now and works:

diff --git a/cache.c b/cache.c
index b09769e..8a66db9 100644
--- a/cache.c
+++ b/cache.c
@@ -25,6 +25,7 @@
 struct cache_slot {
 	const char *key;
 	size_t keylen;
+	int sndfpok;
 	int ttl;
 	cache_fill_fn fn;
 	int cache_fd;
@@ -92,7 +93,7 @@ static int print_slot(struct cache_slot *slot)
 
 	start_off = slot->keylen + 1;
 
-	do {
+	if (slot->sndfpok) for(;;) {
 		ret = sendfile(STDOUT_FILENO, slot->cache_fd, &start_off,
 				slot->cache_st.st_size - start_off);
 		if (ret < 0) {
@@ -103,7 +104,7 @@ static int print_slot(struct cache_slot *slot)
 			return errno;
 		}
 		return 0;
-	} while (1);
+	}
 #endif
 
 	i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET);
@@ -350,7 +351,7 @@ static int process_slot(struct cache_slot *slot)
 
 /* Print cached content to stdout, generate the content if necessary. */
 int cache_process(int size, const char *path, const char *key, int ttl,
-		  cache_fill_fn fn)
+		  cache_fill_fn fn, int sndfpok)
 {
 	unsigned long hash;
 	int i;
@@ -383,6 +384,7 @@ int cache_process(int size, const char *path, const char *key, int ttl,
 	strbuf_addbuf(&lockname, &filename);
 	strbuf_addstr(&lockname, ".lock");
 	slot.fn = fn;
+	slot.sndfpok = sndfpok;
 	slot.ttl = ttl;
 	slot.stdout_fd = -1;
 	slot.cache_name = filename.buf;
diff --git a/cache.h b/cache.h
index 470da4f..3e25daa 100644
--- a/cache.h
+++ b/cache.h
@@ -17,12 +17,13 @@ typedef void (*cache_fill_fn)(void);
  *   key     the key used to lookup cache files
  *   ttl     max cache time in seconds for this key
  *   fn      content generator function for this key
+ *   sndfpok allowed to use sendfile(2)
  *
  * Return value
  *   0 indicates success, everything else is an error
  */
 extern int cache_process(int size, const char *path, const char *key, int ttl,
-			 cache_fill_fn fn);
+			 cache_fill_fn fn, int sndfpok);
 
 
 /* List info about all cache entries on stdout */
diff --git a/cgit.c b/cgit.c
index 08d81a1..684f97a 100644
--- a/cgit.c
+++ b/cgit.c
@@ -197,6 +197,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.enable_git_config = atoi(value);
 	else if (!strcmp(name, "max-stats"))
 		ctx.cfg.max_stats = cgit_find_stats_period(value, NULL);
+	else if (!strcmp(name, "cache-sendfile"))
+		ctx.cfg.cache_sendfile = atoi(value);
 	else if (!strcmp(name, "cache-size"))
 		ctx.cfg.cache_size = atoi(value);
 	else if (!strcmp(name, "cache-root"))
@@ -363,6 +365,7 @@ static void prepare_context(void)
 {
 	memset(&ctx, 0, sizeof(ctx));
 	ctx.cfg.agefile = "info/web/last-modified";
+	ctx.cfg.cache_sendfile = 1;
 	ctx.cfg.cache_size = 0;
 	ctx.cfg.cache_max_create_time = 5;
 	ctx.cfg.cache_root = CGIT_CACHE_ROOT;
@@ -1103,7 +1106,8 @@ int cmd_main(int argc, const char **argv)
 	if (!ctx.env.authenticated || (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD")))
 		ctx.cfg.cache_size = 0;
 	err = cache_process(ctx.cfg.cache_size, ctx.cfg.cache_root,
-			    ctx.qry.raw, ttl, process_request);
+			    ctx.qry.raw, ttl, process_request,
+			    ctx.cfg.cache_sendfile);
 	cgit_cleanup_filters();
 	if (err)
 		cgit_print_error("Error processing page: %s (%d)",
diff --git a/cgit.h b/cgit.h
index 69b5c13..21677a6 100644
--- a/cgit.h
+++ b/cgit.h
@@ -215,6 +215,7 @@ struct cgit_config {
 	char *repository_sort;
 	char *virtual_root;	/* Always ends with '/'. */
 	char *strict_export;
+	int cache_sendfile;
 	int cache_size;
 	int cache_dynamic_ttl;
 	int cache_max_create_time;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 33a6a8c..7ea1b81 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -83,6 +83,10 @@ cache-scanrc-ttl::
 	of scanning a path for git repositories. See also: "CACHE". Default
 	value: "15".
 
+cache-sendfile::
+	Whether sendfile(2) optimization may be used for CGI outputs, when
+	available. Default value: "1".
+
 case-sensitive-sort::
 	Sort items in the repo list case sensitively. Default value: "1".
 	See also: repository-sort, section-sort.

--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)