From: Omar Polo Subject: gotwebd: finishing the template refactoring To: gameoftrees@openbsd.org Date: Tue, 21 Feb 2023 20:19:32 +0100 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 #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 #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, "\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 #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 *
-{{ end }} - -{{ define gotweb_render_footer(struct template *tp) }} -{! - struct request *c = tp->tp_arg; - struct server *srv = c->srv; -!} + {{ render body(tp) }}
{{ if srv->show_site_owner }} @@ -432,11 +427,11 @@ static inline int rss_author(struct template *, char *
{{ 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); !}
@@ -710,10 +705,11 @@ static inline int rss_author(struct template *, char *
{{ 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 *
{{ 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; !}
{{ if srv->show_repo_description }} blob - cc7ccf7ef44f2f9a5f354959abcc57ee681162bf file + gotwebd/parse.y --- gotwebd/parse.y +++ gotwebd/parse.y @@ -47,9 +47,11 @@ #include #include +#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"