"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
gotwebd: finishing the template refactoring
To:
gameoftrees@openbsd.org
Date:
Tue, 21 Feb 2023 20:19:32 +0100

Download raw body.

Thread 
Now that we have everything under the templates, I'd like to close the
whole refactoring.  The idea is to have only one entrypoint,
gotweb_render_page() that renders the template (function) given as
argument.

The idea is to simplify gotweb_process_request() a bit more, handling
all the cases inside the big switch.  It's also a nice preparation
step towards an incoming diff to have gotwebd return a non-200 HTTP
code on failure.

The only real difference is that gotweb_render_index() now skips
directory entries which fails to handle.  I don't feel too bad about
it.  Unlike other parts of got, since gotwebd speaks HTTP, we need to
decide up until which point we care about errors and generate page
output only after that.  In this case, I decided to care about the
failure in opening /var/www/got/public and not about failure in
reading its content.  The follow-up diff regarding the HTTP status
code will make this clearer.

as usual, my instance is running with this applied.


diffstat /home/op/w/gotd
 M  gotwebd/config.c    |   1+   0-
 M  gotwebd/fcgi.c      |   1+   0-
 M  gotwebd/gotweb.c    |  75+  79-
 M  gotwebd/gotwebd.c   |   1+   0-
 M  gotwebd/gotwebd.h   |  10+   5-
 M  gotwebd/pages.tmpl  |   9+  13-
 M  gotwebd/parse.y     |   3+   1-
 M  gotwebd/sockets.c   |   1+   0-

8 files changed, 101 insertions(+), 98 deletions(-)

diff /home/op/w/gotd
commit - fd8d60a2d11af314daec9c6c7ad0ea5c7ac0abd0
path + /home/op/w/gotd
blob - ec88e1ef5acdb77906f2588283329e2a1f4eda21
file + gotwebd/config.c
--- gotwebd/config.c
+++ gotwebd/config.c
@@ -37,6 +37,7 @@
 #include <imsg.h>
 
 #include "got_opentemp.h"
+#include "got_reference.h"
 
 #include "proc.h"
 #include "gotwebd.h"
blob - dd6dbe5f5b29eee48c7bcc972741e3a471d95c6b
file + gotwebd/fcgi.c
--- gotwebd/fcgi.c
+++ gotwebd/fcgi.c
@@ -33,6 +33,7 @@
 #include <unistd.h>
 
 #include "got_error.h"
+#include "got_reference.h"
 
 #include "proc.h"
 #include "gotwebd.h"
blob - 2ed6735a30264d09cf3010f9aa71887a25d9a6b6
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -84,7 +84,7 @@ static const struct got_error *gotweb_render_index(str
     char *);
 static const struct got_error *gotweb_assign_querystring(struct querystring **,
     char *, char *);
-static const struct got_error *gotweb_render_index(struct request *);
+static int gotweb_render_index(struct template *);
 static const struct got_error *gotweb_init_repo_dir(struct repo_dir **,
     const char *);
 static const struct got_error *gotweb_load_got_path(struct request *c,
@@ -143,17 +143,12 @@ gotweb_process_request(struct request *c)
 gotweb_process_request(struct request *c)
 {
 	const struct got_error *error = NULL, *error2 = NULL;
-	struct got_blob_object *blob = NULL;
 	struct server *srv = NULL;
 	struct querystring *qs = NULL;
 	struct repo_dir *repo_dir = NULL;
-	struct got_reflist_head refs;
-	FILE *fp = NULL;
 	uint8_t err[] = "gotwebd experienced an error: ";
-	int r, html = 0, fd = -1;
+	int r, html = 0;
 
-	TAILQ_INIT(&refs);
-
 	/* init the transport */
 	error = gotweb_init_transport(&c->t);
 	if (error) {
@@ -216,7 +211,8 @@ gotweb_process_request(struct request *c)
 		if (error)
 			goto done;
 
-		error2 = got_open_blob_for_output(&blob, &fd, &binary, c);
+		error2 = got_open_blob_for_output(&c->t->blob, &c->t->fd,
+		    &binary, c);
 		if (error2)
 			goto render;
 
@@ -229,12 +225,12 @@ gotweb_process_request(struct request *c)
 			goto done;
 
 		for (;;) {
-			error = got_object_blob_read_block(&len, blob);
+			error = got_object_blob_read_block(&len, c->t->blob);
 			if (error)
 				goto done;
 			if (len == 0)
 				break;
-			buf = got_object_blob_get_read_buf(blob);
+			buf = got_object_blob_get_read_buf(c->t->blob);
 			if (fcgi_gen_binary_response(c, buf, len) == -1)
 				goto done;
 		}
@@ -258,7 +254,8 @@ gotweb_process_request(struct request *c)
 		if (error)
 			goto done;
 
-		error2 = got_open_blob_for_output(&blob, &fd, &binary, c);
+		error2 = got_open_blob_for_output(&c->t->blob, &c->t->fd,
+		    &binary, c);
 		if (error2)
 			goto render;
 		if (binary) {
@@ -288,9 +285,6 @@ render:
 		goto done;
 	html = 1;
 
-	if (gotweb_render_header(c->tp) == -1)
-		goto err;
-
 	if (error2) {
 		error = error2;
 		goto err;
@@ -303,15 +297,15 @@ render:
 			log_warnx("%s: %s", __func__, error->msg);
 			goto err;
 		}
-		if (gotweb_render_blame(c->tp) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_blame) == -1)
 			goto done;
 		break;
 	case BLOB:
-		if (gotweb_render_blob(c->tp, blob) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_blob) == -1)
 			goto err;
 		break;
 	case BRIEFS:
-		if (gotweb_render_briefs(c->tp) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_briefs) == -1)
 			goto err;
 		break;
 	case COMMITS:
@@ -320,7 +314,7 @@ render:
 			log_warnx("%s: %s", __func__, error->msg);
 			goto err;
 		}
-		if (gotweb_render_commits(c->tp) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_commits) == -1)
 			goto err;
 		break;
 	case DIFF:
@@ -329,23 +323,28 @@ render:
 			log_warnx("%s: %s", __func__, error->msg);
 			goto err;
 		}
-		error = got_open_diff_for_output(&fp, &fd, c);
+		error = got_open_diff_for_output(&c->t->fp, &c->t->fd, c);
 		if (error) {
 			log_warnx("%s: %s", __func__, error->msg);
 			goto err;
 		}
-		if (gotweb_render_diff(c->tp, fp) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_diff) == -1)
 			goto err;
 		break;
 	case INDEX:
-		error = gotweb_render_index(c);
-		if (error) {
-			log_warnx("%s: %s", __func__, error->msg);
+		c->t->nrepos = scandir(srv->repos_path, &c->t->repos, NULL,
+		    alphasort);
+		if (c->t->nrepos == -1) {
+			c->t->repos = NULL;
+			error = got_error_from_errno2("scandir",
+			    srv->repos_path);
 			goto err;
 		}
+		if (gotweb_render_page(c->tp, gotweb_render_index) == -1)
+			goto err;
 		break;
 	case SUMMARY:
-		error = got_ref_list(&refs, c->t->repo, "refs/heads",
+		error = got_ref_list(&c->t->refs, c->t->repo, "refs/heads",
 		    got_ref_cmp_by_name, NULL);
 		if (error) {
 			log_warnx("%s: got_ref_list: %s", __func__,
@@ -360,7 +359,7 @@ render:
 			goto err;
 		}
 		qs->action = SUMMARY;
-		if (gotweb_render_summary(c->tp, &refs) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_summary) == -1)
 			goto done;
 		break;
 	case TAG:
@@ -374,7 +373,7 @@ render:
 			    "bad commit id");
 			goto err;
 		}
-		if (gotweb_render_tag(c->tp) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_tag) == -1)
 			goto done;
 		break;
 	case TAGS:
@@ -383,7 +382,7 @@ render:
 			log_warnx("%s: %s", __func__, error->msg);
 			goto err;
 		}
-		if (gotweb_render_tags(c->tp) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_tags) == -1)
 			goto done;
 		break;
 	case TREE:
@@ -392,7 +391,7 @@ render:
 			log_warnx("%s: %s", __func__, error->msg);
 			goto err;
 		}
-		if (gotweb_render_tree(c->tp) == -1)
+		if (gotweb_render_page(c->tp, gotweb_render_tree) == -1)
 			goto err;
 		break;
 	case ERR:
@@ -420,21 +419,7 @@ done:
 	if (html && fcgi_printf(c, "</div>\n") == -1)
 		return;
 done:
-	if (blob)
-		got_object_blob_close(blob);
-	if (fp) {
-		error = got_gotweb_flushfile(fp, fd);
-		if (error)
-			log_warnx("%s: got_gotweb_flushfile failure: %s",
-			    __func__, error->msg);
-		fd = -1;
-	}
-	if (fd != -1)
-		close(fd);
-	if (html && srv != NULL)
-		gotweb_render_footer(c->tp);
-
-	got_ref_list_free(&refs);
+	return;
 }
 
 struct server *
@@ -473,6 +458,7 @@ gotweb_init_transport(struct transport **t)
 
 	TAILQ_INIT(&(*t)->repo_commits);
 	TAILQ_INIT(&(*t)->repo_tags);
+	TAILQ_INIT(&(*t)->refs);
 
 	(*t)->repo = NULL;
 	(*t)->repo_dir = NULL;
@@ -482,6 +468,8 @@ gotweb_init_transport(struct transport **t)
 	(*t)->next_disp = 0;
 	(*t)->prev_disp = 0;
 
+	(*t)->fd = -1;
+
 	return error;
 }
 
@@ -769,9 +757,12 @@ gotweb_free_transport(struct transport *t)
 void
 gotweb_free_transport(struct transport *t)
 {
+	const struct got_error *err;
 	struct repo_commit *rc = NULL, *trc = NULL;
 	struct repo_tag *rt = NULL, *trt = NULL;
+	int i;
 
+	got_ref_list_free(&t->refs);
 	TAILQ_FOREACH_SAFE(rc, &t->repo_commits, entry, trc) {
 		TAILQ_REMOVE(&t->repo_commits, rc, entry);
 		gotweb_free_repo_commit(rc);
@@ -785,6 +776,22 @@ gotweb_free_transport(struct transport *t)
 	free(t->more_id);
 	free(t->next_id);
 	free(t->prev_id);
+	if (t->blob)
+		got_object_blob_close(t->blob);
+	if (t->fp) {
+		err = got_gotweb_flushfile(t->fp, t->fd);
+		if (err)
+			log_warnx("%s: got_gotweb_flushfile failure: %s",
+			    __func__, err->msg);
+		t->fd = -1;
+	}
+	if (t->fd != -1)
+		close(t->fd);
+	if (t->repos) {
+		for (i = 0; i < t->nrepos; ++i)
+			free(t->repos[i]);
+		free(t->repos);
+	}
 	free(t);
 }
 
@@ -847,30 +854,24 @@ static const struct got_error *
 	}
 }
 
-static const struct got_error *
-gotweb_render_index(struct request *c)
+static int
+gotweb_render_index(struct template *tp)
 {
 	const struct got_error *error = NULL;
+	struct request *c = tp->tp_arg;
 	struct server *srv = c->srv;
 	struct transport *t = c->t;
 	struct querystring *qs = t->qs;
 	struct repo_dir *repo_dir = NULL;
-	struct dirent **sd_dent = NULL;
-	unsigned int d_cnt, d_i, d_disp = 0;
+	struct dirent **sd_dent = t->repos;
+	unsigned int d_i, d_disp = 0;
 	unsigned int d_skipped = 0;
-	int type;
+	int type, r;
 
-	d_cnt = scandir(srv->repos_path, &sd_dent, NULL, alphasort);
-	if (d_cnt == -1) {
-		sd_dent = NULL;
-		error = got_error_from_errno2("scandir", srv->repos_path);
-		goto done;
-	}
-
 	if (gotweb_render_repo_table_hdr(c->tp) == -1)
-		goto done;
+		return -1;
 
-	for (d_i = 0; d_i < d_cnt; d_i++) {
+	for (d_i = 0; d_i < t->nrepos; d_i++) {
 		if (srv->max_repos > 0 && t->prev_disp == srv->max_repos)
 			break;
 
@@ -883,7 +884,7 @@ gotweb_render_index(struct request *c)
 		error = got_path_dirent_type(&type, srv->repos_path,
 		    sd_dent[d_i]);
 		if (error)
-			goto done;
+			continue;
 		if (type != DT_DIR) {
 			d_skipped++;
 			continue;
@@ -897,50 +898,45 @@ gotweb_render_index(struct request *c)
 
 		error = gotweb_init_repo_dir(&repo_dir, sd_dent[d_i]->d_name);
 		if (error)
-			goto done;
+			continue;
 
 		error = gotweb_load_got_path(c, repo_dir);
-		if (error && error->code == GOT_ERR_NOT_GIT_REPO) {
-			error = NULL;
+		if (error && error->code == GOT_ERR_LONELY_PACKIDX) {
+			if (error->code != GOT_ERR_NOT_GIT_REPO)
+				log_warnx("%s: %s: %s", __func__,
+				    sd_dent[d_i]->d_name, error->msg);
 			gotweb_free_repo_dir(repo_dir);
 			repo_dir = NULL;
 			d_skipped++;
 			continue;
 		}
-		if (error && error->code != GOT_ERR_LONELY_PACKIDX)
-			goto done;
 
 		d_disp++;
 		t->prev_disp++;
 
-		if (gotweb_render_repo_fragment(c->tp, repo_dir) == -1)
-			goto done;
-
+		r = gotweb_render_repo_fragment(c->tp, repo_dir);
 		gotweb_free_repo_dir(repo_dir);
-		repo_dir = NULL;
+		if (r == -1)
+			return -1;
+
 		t->next_disp++;
 		if (d_disp == srv->max_repos_display)
 			break;
 	}
-	t->repos_total = d_cnt - d_skipped;
+	t->repos_total = t->nrepos - d_skipped;
 
 	if (srv->max_repos_display == 0)
-		goto done;
+		return 0;
 	if (srv->max_repos > 0 && srv->max_repos < srv->max_repos_display)
-		goto done;
+		return 0;
 	if (t->repos_total <= srv->max_repos ||
 	    t->repos_total <= srv->max_repos_display)
-		goto done;
+		return 0;
 
 	if (gotweb_render_navs(c->tp) == -1)
-		goto done;
-done:
-	if (sd_dent) {
-		for (d_i = 0; d_i < d_cnt; d_i++)
-			free(sd_dent[d_i]);
-		free(sd_dent);
-	}
-	return error;
+		return -1;
+
+	return 0;
 }
 
 static inline int
blob - cc0a4954a61a219297305919f2441a682fcb9a78
file + gotwebd/gotwebd.c
--- gotwebd/gotwebd.c
+++ gotwebd/gotwebd.c
@@ -41,6 +41,7 @@
 #include <util.h>
 
 #include "got_opentemp.h"
+#include "got_reference.h"
 
 #include "proc.h"
 #include "gotwebd.h"
blob - 98e9f879c0f9fc272de90ddc649d0c9aa2d88875
file + gotwebd/gotwebd.h
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -186,6 +186,7 @@ struct transport {
 struct transport {
 	TAILQ_HEAD(repo_commits_head, repo_commit)	 repo_commits;
 	TAILQ_HEAD(repo_tags_head, repo_tag)		 repo_tags;
+	struct got_reflist_head	 refs;
 	struct got_repository	*repo;
 	struct repo_dir		*repo_dir;
 	struct querystring	*qs;
@@ -196,6 +197,11 @@ struct transport {
 	unsigned int		 next_disp;
 	unsigned int		 prev_disp;
 	unsigned int		 tag_count;
+	struct got_blob_object	*blob;
+	int			 fd;
+	FILE			*fp;
+	struct dirent		**repos;
+	int			 nrepos;
 };
 
 enum socket_priv_fds {
@@ -463,20 +469,19 @@ int	gotweb_render_header(struct template *);
 void gotweb_free_transport(struct transport *);
 
 /* pages.tmpl */
-int	gotweb_render_header(struct template *);
-int	gotweb_render_footer(struct template *);
+int	gotweb_render_page(struct template *, int (*)(struct template *));
 int	gotweb_render_repo_table_hdr(struct template *);
 int	gotweb_render_repo_fragment(struct template *, struct repo_dir *);
 int	gotweb_render_briefs(struct template *);
 int	gotweb_render_navs(struct template *);
 int	gotweb_render_commits(struct template *);
-int	gotweb_render_blob(struct template *, struct got_blob_object *);
+int	gotweb_render_blob(struct template *);
 int	gotweb_render_tree(struct template *);
 int	gotweb_render_tags(struct template *);
 int	gotweb_render_tag(struct template *);
-int	gotweb_render_diff(struct template *, FILE *);
+int	gotweb_render_diff(struct template *);
 int	gotweb_render_branches(struct template *, struct got_reflist_head *);
-int	gotweb_render_summary(struct template *, struct got_reflist_head *);
+int	gotweb_render_summary(struct template *);
 int	gotweb_render_blame(struct template *);
 int	gotweb_render_rss(struct template *);
 
blob - 8f5000ba428821f634226fcc4a467c4605e51645
file + gotwebd/pages.tmpl
--- gotwebd/pages.tmpl
+++ gotwebd/pages.tmpl
@@ -53,7 +53,8 @@ static inline int rss_author(struct template *, char *
 
 !}
 
-{{ define gotweb_render_header(struct template *tp) }}
+{{ define gotweb_render_page(struct template *tp,
+    int (*body)(struct template *)) }}
 {!
 	struct request		*c = tp->tp_arg;
 	struct server		*srv = c->srv;
@@ -109,13 +110,7 @@ static inline int rss_author(struct template *, char *
         </div>
       </div>
       <div id="content">
-{{ end }}
-
-{{ define gotweb_render_footer(struct template *tp) }}
-{!
-	struct request		*c = tp->tp_arg;
-	struct server		*srv = c->srv;
-!}
+        {{ render body(tp) }}
         <div id="site_owner_wrapper">
           <div id="site_owner">
             {{ if srv->show_site_owner }}
@@ -432,11 +427,11 @@ static inline int rss_author(struct template *, char *
 </div>
 {{ end }}
 
-{{ define gotweb_render_blob(struct template *tp,
-    struct got_blob_object *blob) }}
+{{ define gotweb_render_blob(struct template *tp) }}
 {!
 	struct request		*c = tp->tp_arg;
 	struct transport	*t = c->t;
+	struct got_blob_object	*blob = t->blob;
 	struct repo_commit	*rc = TAILQ_FIRST(&t->repo_commits);
 !}
 <div id="blob_title_wrapper">
@@ -710,10 +705,11 @@ static inline int rss_author(struct template *, char *
 </div>
 {{ end }}
 
-{{ define gotweb_render_diff(struct template *tp, FILE *fp) }}
+{{ define gotweb_render_diff(struct template *tp) }}
 {!
 	struct request		*c = tp->tp_arg;
 	struct transport	*t = c->t;
+	FILE			*fp = t->fp;
 	struct repo_commit	*rc = TAILQ_FIRST(&t->repo_commits);
 	char			*line = NULL;
 	size_t			 linesize = 0;
@@ -850,12 +846,12 @@ static inline int rss_author(struct template *, char *
 </div>
 {{ end }}
 
-{{ define gotweb_render_summary(struct template *tp,
-    struct got_reflist_head *refs) }}
+{{ define gotweb_render_summary(struct template *tp) }}
 {!
 	struct request		*c = tp->tp_arg;
 	struct server		*srv = c->srv;
 	struct transport	*t = c->t;
+	struct got_reflist_head	*refs = &t->refs;
 !}
 <div id="summary_wrapper">
   {{ if srv->show_repo_description }}
blob - cc7ccf7ef44f2f9a5f354959abcc57ee681162bf
file + gotwebd/parse.y
--- gotwebd/parse.y
+++ gotwebd/parse.y
@@ -47,9 +47,11 @@
 #include <syslog.h>
 #include <unistd.h>
 
+#include "got_sockaddr.h"
+#include "got_reference.h"
+
 #include "proc.h"
 #include "gotwebd.h"
-#include "got_sockaddr.h"
 
 TAILQ_HEAD(files, file)		 files = TAILQ_HEAD_INITIALIZER(files);
 static struct file {
blob - cfba5dfb0e7368b0b37738d2eb71b53d437db169
file + gotwebd/sockets.c
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -51,6 +51,7 @@
 
 #include "got_error.h"
 #include "got_opentemp.h"
+#include "got_reference.h"
 #include "got_repository.h"
 
 #include "proc.h"