From: Tracey Emery Subject: Re: gotwebd: finishing the template refactoring To: Omar Polo Cc: gameoftrees@openbsd.org Date: Fri, 10 Mar 2023 11:20:39 -0700 On Fri, Mar 10, 2023 at 06:54:45PM +0100, Omar Polo wrote: > On 2023/02/21 20:19:32 +0100, Omar Polo wrote: > > 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. > > friendly ping :) ok > > 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 - 5add7f42e1397d136860680e1f0411db17b4f22c > 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 - ba1803b45941d2c4b83b31f232b8e7edfb92df63 > 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 - 52aa037e4a8d734d17311aabbd7586d7e23d8fcd > file + gotwebd/gotweb.c > --- gotwebd/gotweb.c > +++ gotwebd/gotweb.c > @@ -85,7 +85,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, > @@ -144,17 +144,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) { > @@ -217,7 +212,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; > > @@ -230,12 +226,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; > } > @@ -259,7 +255,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) { > @@ -289,9 +286,6 @@ render: > goto done; > html = 1; > > - if (gotweb_render_header(c->tp) == -1) > - goto err; > - > if (error2) { > error = error2; > goto err; > @@ -304,15 +298,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: > @@ -321,7 +315,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: > @@ -330,23 +324,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__, > @@ -361,7 +360,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: > @@ -375,7 +374,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: > @@ -384,7 +383,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: > @@ -393,7 +392,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: > @@ -421,21 +420,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 * > @@ -474,6 +459,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; > @@ -483,6 +469,8 @@ gotweb_init_transport(struct transport **t) > (*t)->next_disp = 0; > (*t)->prev_disp = 0; > > + (*t)->fd = -1; > + > return error; > } > > @@ -770,9 +758,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); > @@ -786,6 +777,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); > } > > @@ -848,30 +855,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; > > @@ -884,7 +885,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; > @@ -898,50 +899,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 - bea0461c4b1286b162ce1d279fb93dd847ff4ab5 > file + gotwebd/pages.tmpl > --- gotwebd/pages.tmpl > +++ gotwebd/pages.tmpl > @@ -54,7 +54,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; > @@ -110,13 +111,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 }} > @@ -433,11 +428,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); > !} >
> @@ -711,10 +706,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; > @@ -851,12 +847,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 - ecb88027e072a125a309c08b985733a77ab2a26d > 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" > -- Tracey Emery