simplify template APIs

TL;DR: this moves the buffering from the fastcgi code to the template, reducing the indirections in fcgi.c and with a net negative. -*-*- When I was first tinkering with template(1) I wrote its API around two primitives, putc and puts, because I was dealing with code that looped over strings one char at the time to do the escaping and so it was natural to build on top of that. The template assumed buffering is done at a lower level, which we already had. Having the template code doing the buffering instead allows for a simpler usage / setup and to get rid of a few layer of functions in fcgi.c. Caching at the fcgi level is okish but not great, see the special case in fcgi_gen_binary_response to avoid wasting all of our time in memcpy(). The only downside of moving the buffering is that now we have to remember to call template_flush() in a few places to flush what could remain in the template' cache. The changes in gotweb.c are mechanical, just replace the fcgi_*() with tmpl_*(). gotwebd.h and sockets.c are trivial, as well as the regress changes. The codegen changes in template/parse.y should also be trivial. The only 'magic' is in introducing a few more functions in tmpl.c and dropping some layer of outputting in fcgi.c. A minor change in the codegen (template/parse.y) is that now we compute the string length at 'compile' time when possible, doing fewer strlen() at runtime which is nice :-) (i have this running on my instance) diffstat /home/op/w/got M gotwebd/fcgi.c | 9+ 90- M gotwebd/gotweb.c | 33+ 31- M gotwebd/gotwebd.h | 1+ 8- M gotwebd/sockets.c | 1+ 1- M regress/template/runbase.c | 12+ 20- M regress/template/runlist.c | 11+ 19- M template/parse.y | 6+ 5- M template/tmpl.c | 74+ 12- M template/tmpl.h | 13+ 8- 9 files changed, 160 insertions(+), 194 deletions(-) diff /home/op/w/got commit - 0ec0e499ac07b3fa388b786eb0cb489bd810df26 path + /home/op/w/got blob - 5c06bfd1e66558fb49eb1dbacf1a866876717461 file + gotwebd/fcgi.c --- gotwebd/fcgi.c +++ gotwebd/fcgi.c @@ -140,12 +140,6 @@ fcgi_parse_record(uint8_t *buf, size_t n, struct reque break; case FCGI_STDIN: case FCGI_ABORT_REQUEST: - if (c->sock->client_status != CLIENT_DISCONNECT && - c->outbuf_len != 0) { - fcgi_send_response(c, FCGI_STDOUT, c->outbuf, - c->outbuf_len); - } - fcgi_create_end_record(c); fcgi_cleanup_request(c); return 0; @@ -196,6 +190,7 @@ fcgi_parse_params(uint8_t *buf, uint16_t n, struct req if (n == 0) { gotweb_process_request(c); + template_flush(c->tp); return; } @@ -276,90 +271,6 @@ int fcgi_cleanup_request((struct request*) arg); } -int -fcgi_puts(struct template *tp, const char *str) -{ - if (str == NULL) - return 0; - return fcgi_gen_binary_response(tp->tp_arg, str, strlen(str)); -} - -int -fcgi_putc(struct template *tp, int ch) -{ - uint8_t c = ch; - return fcgi_gen_binary_response(tp->tp_arg, &c, 1); -} - -int -fcgi_vprintf(struct request *c, const char *fmt, va_list ap) -{ - char *str; - int r; - - r = vasprintf(&str, fmt, ap); - if (r == -1) { - log_warn("%s: asprintf", __func__); - return -1; - } - - r = fcgi_gen_binary_response(c, str, r); - free(str); - return r; -} - -int -fcgi_printf(struct request *c, const char *fmt, ...) -{ - va_list ap; - int r; - - va_start(ap, fmt); - r = fcgi_vprintf(c, fmt, ap); - va_end(ap); - - return r; -} - -int -fcgi_gen_binary_response(struct request *c, const uint8_t *data, int len) -{ - int r; - - if (c->sock->client_status == CLIENT_DISCONNECT) - return -1; - - if (data == NULL || len == 0) - return 0; - - /* - * special case: send big replies -like blobs- directly - * without copying. - */ - if (len > sizeof(c->outbuf)) { - if (c->outbuf_len > 0) { - fcgi_send_response(c, FCGI_STDOUT, - c->outbuf, c->outbuf_len); - c->outbuf_len = 0; - } - return fcgi_send_response(c, FCGI_STDOUT, data, len); - } - - if (len < sizeof(c->outbuf) - c->outbuf_len) { - memcpy(c->outbuf + c->outbuf_len, data, len); - c->outbuf_len += len; - return 0; - } - - r = fcgi_send_response(c, FCGI_STDOUT, c->outbuf, c->outbuf_len); - if (r == -1) - return -1; - - memcpy(c->outbuf, data, len); - c->outbuf_len = len; - return 0; -} - static int send_response(struct request *c, int type, const uint8_t *data, size_t len) @@ -464,6 +375,14 @@ void return send_response(c, type, data, len); } +int +fcgi_write(void *arg, const void *buf, size_t len) +{ + struct request *c = arg; + + return fcgi_send_response(c, FCGI_STDOUT, buf, len); +} + void fcgi_create_end_record(struct request *c) { blob - d9c0aa2de83b12df70d1fbce28bfddc549348a9c file + gotwebd/gotweb.c --- gotwebd/gotweb.c +++ gotwebd/gotweb.c @@ -106,25 +106,25 @@ gotweb_reply(struct request *c, int status, const char { const char *csp; - if (status != 200 && fcgi_printf(c, "Status: %d\r\n", status) == -1) + if (status != 200 && tp_writef(c->tp, "Status: %d\r\n", status) == -1) return -1; if (location) { - if (fcgi_puts(c->tp, "Location: ") == -1 || + if (tp_writes(c->tp, "Location: ") == -1 || gotweb_render_url(c, location) == -1 || - fcgi_puts(c->tp, "\r\n") == -1) + tp_writes(c->tp, "\r\n") == -1) return -1; } csp = "Content-Security-Policy: default-src 'self'; " "script-src 'none'; object-src 'none';\r\n"; - if (fcgi_puts(c->tp, csp) == -1) + if (tp_writes(c->tp, csp) == -1) return -1; - if (ctype && fcgi_printf(c, "Content-Type: %s\r\n", ctype) == -1) + if (ctype && tp_writef(c->tp, "Content-Type: %s\r\n", ctype) == -1) return -1; - return fcgi_puts(c->tp, "\r\n"); + return tp_writes(c->tp, "\r\n"); } static int @@ -133,7 +133,7 @@ gotweb_reply_file(struct request *c, const char *ctype { int r; - r = fcgi_printf(c, "Content-Disposition: attachment; " + r = tp_writef(c->tp, "Content-Disposition: attachment; " "filename=%s%s\r\n", file, suffix ? suffix : ""); if (r == -1) return -1; @@ -255,6 +255,8 @@ gotweb_process_request(struct request *c) r = gotweb_reply(c, 200, "text/plain", NULL); if (r == -1) return; + if (template_flush(c->tp) == -1) + return; for (;;) { error = got_object_blob_read_block(&len, c->t->blob); @@ -263,7 +265,7 @@ gotweb_process_request(struct request *c) if (len == 0) break; buf = got_object_blob_get_read_buf(c->t->blob); - if (fcgi_gen_binary_response(c, buf, len) == -1) + if (fcgi_write(c, (char *)buf, len) == -1) break; } return; @@ -1004,25 +1006,25 @@ gotweb_render_url(struct request *c, struct gotweb_url action = gotweb_action_name(url->action); if (action != NULL) { - if (fcgi_printf(c, "?action=%s", action) == -1) + if (tp_writef(c->tp, "?action=%s", action) == -1) return -1; sep = "&"; } if (url->commit) { - if (fcgi_printf(c, "%scommit=%s", sep, url->commit) == -1) + if (tp_writef(c->tp, "%scommit=%s", sep, url->commit) == -1) return -1; sep = "&"; } if (url->previd) { - if (fcgi_printf(c, "%sprevid=%s", sep, url->previd) == -1) + if (tp_writef(c->tp, "%sprevid=%s", sep, url->previd) == -1) return -1; sep = "&"; } if (url->prevset) { - if (fcgi_printf(c, "%sprevset=%s", sep, url->prevset) == -1) + if (tp_writef(c->tp, "%sprevset=%s", sep, url->prevset) == -1) return -1; sep = "&"; } @@ -1031,7 +1033,7 @@ gotweb_render_url(struct request *c, struct gotweb_url tmp = gotweb_urlencode(url->file); if (tmp == NULL) return -1; - r = fcgi_printf(c, "%sfile=%s", sep, tmp); + r = tp_writef(c->tp, "%sfile=%s", sep, tmp); free(tmp); if (r == -1) return -1; @@ -1042,7 +1044,7 @@ gotweb_render_url(struct request *c, struct gotweb_url tmp = gotweb_urlencode(url->folder); if (tmp == NULL) return -1; - r = fcgi_printf(c, "%sfolder=%s", sep, tmp); + r = tp_writef(c->tp, "%sfolder=%s", sep, tmp); free(tmp); if (r == -1) return -1; @@ -1053,7 +1055,7 @@ gotweb_render_url(struct request *c, struct gotweb_url tmp = gotweb_urlencode(url->headref); if (tmp == NULL) return -1; - r = fcgi_printf(c, "%sheadref=%s", sep, url->headref); + r = tp_writef(c->tp, "%sheadref=%s", sep, url->headref); free(tmp); if (r == -1) return -1; @@ -1061,7 +1063,7 @@ gotweb_render_url(struct request *c, struct gotweb_url } if (url->index_page != -1) { - if (fcgi_printf(c, "%sindex_page=%d", sep, + if (tp_writef(c->tp, "%sindex_page=%d", sep, url->index_page) == -1) return -1; sep = "&"; @@ -1071,7 +1073,7 @@ gotweb_render_url(struct request *c, struct gotweb_url tmp = gotweb_urlencode(url->path); if (tmp == NULL) return -1; - r = fcgi_printf(c, "%spath=%s", sep, tmp); + r = tp_writef(c->tp, "%spath=%s", sep, tmp); free(tmp); if (r == -1) return -1; @@ -1079,7 +1081,7 @@ gotweb_render_url(struct request *c, struct gotweb_url } if (url->page != -1) { - if (fcgi_printf(c, "%spage=%d", sep, url->page) == -1) + if (tp_writef(c->tp, "%spage=%d", sep, url->page) == -1) return -1; sep = "&"; } @@ -1093,8 +1095,8 @@ gotweb_render_absolute_url(struct request *c, struct g struct template *tp = c->tp; const char *proto = c->https ? "https" : "http"; - if (fcgi_puts(tp, proto) == -1 || - fcgi_puts(tp, "://") == -1 || + if (tp_writes(tp, proto) == -1 || + tp_writes(tp, "://") == -1 || tp_htmlescape(tp, c->server_name) == -1 || tp_htmlescape(tp, c->document_uri) == -1) return -1; @@ -1368,36 +1370,36 @@ gotweb_render_age(struct template *tp, time_t committe case TM_DIFF: diff_time = time(NULL) - committer_time; if (diff_time > 60 * 60 * 24 * 365 * 2) { - if (fcgi_printf(c, "%lld %s", + if (tp_writef(c->tp, "%lld %s", (diff_time / 60 / 60 / 24 / 365), years) == -1) return -1; } else if (diff_time > 60 * 60 * 24 * (365 / 12) * 2) { - if (fcgi_printf(c, "%lld %s", + if (tp_writef(c->tp, "%lld %s", (diff_time / 60 / 60 / 24 / (365 / 12)), months) == -1) return -1; } else if (diff_time > 60 * 60 * 24 * 7 * 2) { - if (fcgi_printf(c, "%lld %s", + if (tp_writef(c->tp, "%lld %s", (diff_time / 60 / 60 / 24 / 7), weeks) == -1) return -1; } else if (diff_time > 60 * 60 * 24 * 2) { - if (fcgi_printf(c, "%lld %s", + if (tp_writef(c->tp, "%lld %s", (diff_time / 60 / 60 / 24), days) == -1) return -1; } else if (diff_time > 60 * 60 * 2) { - if (fcgi_printf(c, "%lld %s", + if (tp_writef(c->tp, "%lld %s", (diff_time / 60 / 60), hours) == -1) return -1; } else if (diff_time > 60 * 2) { - if (fcgi_printf(c, "%lld %s", (diff_time / 60), + if (tp_writef(c->tp, "%lld %s", (diff_time / 60), minutes) == -1) return -1; } else if (diff_time > 2) { - if (fcgi_printf(c, "%lld %s", diff_time, + if (tp_writef(c->tp, "%lld %s", diff_time, seconds) == -1) return -1; } else { - if (fcgi_puts(tp, now) == -1) + if (tp_writes(tp, now) == -1) return -1; } break; @@ -1409,8 +1411,8 @@ gotweb_render_age(struct template *tp, time_t committe if (s == NULL) return -1; - if (fcgi_puts(tp, datebuf) == -1 || - fcgi_puts(tp, " UTC") == -1) + if (tp_writes(tp, datebuf) == -1 || + tp_writes(tp, " UTC") == -1) return -1; break; case TM_RFC822: @@ -1422,7 +1424,7 @@ gotweb_render_age(struct template *tp, time_t committe if (r == 0) return -1; - if (fcgi_puts(tp, datebuf) == -1) + if (tp_writes(tp, datebuf) == -1) return -1; break; } blob - d276e0ebe586663208dfe4640e9c7714bed9f58d file + gotwebd/gotwebd.h --- gotwebd/gotwebd.h +++ gotwebd/gotwebd.h @@ -240,7 +240,6 @@ struct request { size_t buf_len; uint8_t outbuf[GOTWEBD_CACHESIZE]; - size_t outbuf_len; char querystring[MAX_QUERYSTRING]; char document_uri[MAX_DOCUMENT_URI]; @@ -495,13 +494,7 @@ int fcgi_puts(struct template *, const char *); void fcgi_cleanup_request(struct request *); void fcgi_create_end_record(struct request *); void dump_fcgi_record(const char *, struct fcgi_record_header *); -int fcgi_puts(struct template *, const char *); -int fcgi_putc(struct template *, int); -int fcgi_vprintf(struct request *, const char *, va_list); -int fcgi_printf(struct request *, const char *, ...) - __attribute__((__format__(printf, 2, 3))) - __attribute__((__nonnull__(2))); -int fcgi_gen_binary_response(struct request *, const uint8_t *, int); +int fcgi_write(void *, const void *, size_t); /* got_operations.c */ const struct got_error *got_gotweb_closefile(FILE *); blob - 3f932978bf05a203c0c4bcd43bf7390fb4d153c8 file + gotwebd/sockets.c --- gotwebd/sockets.c +++ gotwebd/sockets.c @@ -620,7 +620,7 @@ sockets_socket_accept(int fd, short event, void *arg) return; } - c->tp = template(c, fcgi_puts, fcgi_putc); + c->tp = template(c, &fcgi_write, c->outbuf, sizeof(c->outbuf)); if (c->tp == NULL) { log_warn("%s", __func__); close(s); blob - 602717bcefc2fd65c0cb426c4d1fe263cdbaee7f file + regress/template/runbase.c --- regress/template/runbase.c +++ regress/template/runbase.c @@ -21,44 +21,36 @@ int my_putc(struct template *, int); #include "tmpl.h" int base(struct template *, const char *title); -int my_putc(struct template *, int); -int my_puts(struct template *, const char *); +int my_write(void *, void *, size_t); int -my_putc(struct template *tp, int c) +my_write(void *arg, void *s, size_t len) { - FILE *fp = tp->tp_arg; + FILE *fp = arg; - if (putc(c, fp) < 0) + if (fwrite(s, 1, len, fp) < 0) return (-1); return (0); } int -my_puts(struct template *tp, const char *s) -{ - FILE *fp = tp->tp_arg; - - if (fputs(s, fp) < 0) - return (-1); - - return (0); -} - -int main(int argc, char **argv) { - struct template *tp; + struct template *tp; + char buf[3]; + /* use a ridiculously small buffer in regress */ - if ((tp = template(stdout, my_puts, my_putc)) == NULL) + if ((tp = template(stdout, my_write, buf, sizeof(buf))) == NULL) err(1, "template"); - if (base(tp, " *hello* ") == -1) + if (base(tp, " *hello* ") == -1 || + template_flush(tp) == -1) return (1); puts(""); - if (base(tp, "<hello>") == -1) + if (base(tp, "<hello>") == -1 || + template_flush(tp) == -1) return (1); puts(""); blob - f6a415f000a087d0b1d41be0343b51916bebe9dd file + regress/template/runlist.c --- regress/template/runlist.c +++ regress/template/runlist.c @@ -22,40 +22,30 @@ int my_putc(struct template *, int); #include "lists.h" int base(struct template *, struct tailhead *); -int my_putc(struct template *, int); -int my_puts(struct template *, const char *); +int my_write(void *, void *, size_t); int -my_putc(struct template *tp, int c) +my_write(void *arg, void *s, size_t len) { - FILE *fp = tp->tp_arg; + FILE *fp = arg; - if (putc(c, fp) < 0) + if (fwrite(s, 1, len, fp) < 0) return (-1); return (0); } int -my_puts(struct template *tp, const char *s) -{ - FILE *fp = tp->tp_arg; - - if (fputs(s, fp) < 0) - return (-1); - - return (0); -} - -int main(int argc, char **argv) { struct template *tp; struct tailhead head; struct entry *np; int i; + char buf[3]; + /* use a ridiculously small buffer in regress */ - if ((tp = template(stdout, my_puts, my_putc)) == NULL) + if ((tp = template(stdout, my_write, buf, sizeof(buf))) == NULL) err(1, "template"); TAILQ_INIT(&head); @@ -67,7 +57,8 @@ main(int argc, char **argv) TAILQ_INSERT_TAIL(&head, np, entries); } - if (base(tp, &head) == -1) + if (base(tp, &head) == -1 || + template_flush(tp) == -1) return (1); puts(""); @@ -77,7 +68,8 @@ main(int argc, char **argv) free(np); } - if (base(tp, &head) == -1) + if (base(tp, &head) == -1 || + template_flush(tp) == -1) return (1); puts(""); blob - c97478128fa19c9cddcf687a802db929cfa6bd36 file + template/parse.y --- template/parse.y +++ template/parse.y @@ -139,9 +139,10 @@ raw : nstring { raw : nstring { dbg(); - fprintf(fp, "if ((tp_ret = tp->tp_puts(tp, "); + fprintf(fp, "if ((tp_ret = tp_write(tp, "); printq($1); - fputs(")) == -1) goto err;\n", fp); + fprintf(fp, ", %zu)) == -1) goto err;\n", + strlen($1)); free($1); } @@ -189,7 +190,7 @@ special : '{' RENDER string '}' { | '{' string '|' UNSAFE '}' { dbg(); fprintf(fp, - "if ((tp_ret = tp->tp_puts(tp, %s)) == -1)\n", + "if ((tp_ret = tp_writes(tp, %s)) == -1)\n", $2); fputs("goto err;\n", fp); free($2); @@ -205,7 +206,7 @@ special : '{' RENDER string '}' { | '{' string '}' { dbg(); fprintf(fp, - "if ((tp_ret = tp->tp_escape(tp, %s)) == -1)\n", + "if ((tp_ret = tp_htmlescape(tp, %s)) == -1)\n", $2); fputs("goto err;\n", fp); free($2); @@ -218,7 +219,7 @@ printf : '{' PRINTF { } printfargs '}' { fputs(") == -1)\n", fp); fputs("goto err;\n", fp); - fputs("if ((tp_ret = tp->tp_escape(tp, tp->tp_tmp)) " + fputs("if ((tp_ret = tp_htmlescape(tp, tp->tp_tmp)) " "== -1)\n", fp); fputs("goto err;\n", fp); fputs("free(tp->tp_tmp);\n", fp); blob - 18ee428516dc3b90ef925c5ac135e90278c0d5f9 file + template/tmpl.c --- template/tmpl.c +++ template/tmpl.c @@ -15,12 +15,62 @@ */ #include <ctype.h> +#include <stdarg.h> #include <stdio.h> #include <stdlib.h> +#include <string.h> #include "tmpl.h" int +tp_write(struct template *tp, const char *str, size_t len) +{ + size_t avail; + + while (len > 0) { + avail = tp->tp_cap - tp->tp_len; + if (avail == 0) { + if (template_flush(tp) == -1) + return (-1); + avail = tp->tp_cap; + } + + if (len < avail) + avail = len; + + memcpy(tp->tp_buf + tp->tp_len, str, avail); + tp->tp_len += avail; + str += avail; + len -= avail; + } + + return (0); +} + +int +tp_writes(struct template *tp, const char *str) +{ + return (tp_write(tp, str, strlen(str))); +} + +int +tp_writef(struct template *tp, const char *fmt, ...) +{ + va_list ap; + char *str; + int r; + + va_start(ap, fmt); + r = vasprintf(&str, fmt, ap); + va_end(ap); + if (r == -1) + return (-1); + r = tp_write(tp, str, r); + free(str); + return (r); +} + +int tp_urlescape(struct template *tp, const char *str) { int r; @@ -36,10 +86,10 @@ tp_urlescape(struct template *tp, const char *str) r = snprintf(tmp, sizeof(tmp), "%%%2X", *str); if (r < 0 || (size_t)r >= sizeof(tmp)) return (0); - if (tp->tp_puts(tp, tmp) == -1) + if (tp_write(tp, tmp, r) == -1) return (-1); } else { - if (tp->tp_putc(tp, *str) == -1) + if (tp_write(tp, str, 1) == -1) return (-1); } } @@ -58,22 +108,22 @@ tp_htmlescape(struct template *tp, const char *str) for (; *str; ++str) { switch (*str) { case '<': - r = tp->tp_puts(tp, "<"); + r = tp_write(tp, "<", 4); break; case '>': - r = tp->tp_puts(tp, ">"); + r = tp_write(tp, ">", 4); break; case '&': - r = tp->tp_puts(tp, "&"); + r = tp_write(tp, "&", 5); break; case '"': - r = tp->tp_puts(tp, """); + r = tp_write(tp, """, 6); break; case '\'': - r = tp->tp_puts(tp, "'"); + r = tp_write(tp, "'", 6); break; default: - r = tp->tp_putc(tp, *str); + r = tp_write(tp, str, 1); break; } @@ -85,7 +135,7 @@ template(void *arg, tmpl_puts putsfn, tmpl_putc putcfn } struct template * -template(void *arg, tmpl_puts putsfn, tmpl_putc putcfn) +template(void *arg, tmpl_write writefn, char *buf, size_t siz) { struct template *tp; @@ -93,13 +143,25 @@ template(void *arg, tmpl_puts putsfn, tmpl_putc putcfn return (NULL); tp->tp_arg = arg; - tp->tp_escape = tp_htmlescape; - tp->tp_puts = putsfn; - tp->tp_putc = putcfn; + tp->tp_write = writefn; + tp->tp_buf = buf; + tp->tp_cap = siz; return (tp); } +int +template_flush(struct template *tp) +{ + if (tp->tp_len == 0) + return (0); + + if (tp->tp_write(tp->tp_arg, tp->tp_buf, tp->tp_len) == -1) + return (-1); + tp->tp_len = 0; + return (0); +} + void template_free(struct template *tp) { blob - 4c8de903c9b7957de3c6bc1ed2058fa9a5db553b file + template/tmpl.h --- template/tmpl.h +++ template/tmpl.h @@ -19,21 +19,26 @@ typedef int (*tmpl_puts)(struct template *, const char struct template; -typedef int (*tmpl_puts)(struct template *, const char *); -typedef int (*tmpl_putc)(struct template *, int); +typedef int (*tmpl_write)(void *, const void *, size_t); struct template { void *tp_arg; char *tp_tmp; - tmpl_puts tp_escape; - tmpl_puts tp_puts; - tmpl_putc tp_putc; + tmpl_write tp_write; + char *tp_buf; + size_t tp_len; + size_t tp_cap; }; -int tp_urlescape(struct template *, const char *); -int tp_htmlescape(struct template *, const char *); +int tp_write(struct template *, const char *, size_t); +int tp_writes(struct template *, const char *); +int tp_writef(struct template *, const char *, ...) + __attribute__((__format__ (printf, 2, 3))); +int tp_urlescape(struct template *, const char *); +int tp_htmlescape(struct template *, const char *); -struct template *template(void *, tmpl_puts, tmpl_putc); +struct template *template(void *, tmpl_write, char *, size_t); +int template_flush(struct template *); void template_free(struct template *); #endif

2023-09-12 18:31 Omar Polo:
simplify template APIs
- 2023-09-12 22:07 Omar Polo:
  simplify template APIs
- - 2023-09-13 07:04 Stefan Sperling:
    simplify template APIs