From: Omar Polo Subject: Re: gotwebd: openat(2) and less params for scraping repo info To: Omar Polo Cc: gameoftrees@openbsd.org Date: Tue, 15 Nov 2022 09:56:23 +0100 On 2022/11/09 19:54:41 +0100, Omar Polo wrote: > diff below does two things: > > - drops the unused path argument to got_get_repo_{owner,age} > > - makes gotweb_get_repo_{description,clone_url} use openat instead of > re-constructing the path. less strings allocated on-the-fly. Here's a reiteration of the previous diff. Since i was already using openat(2) in gotweb_get_repo_{description,clone_url} i thought i could avoid the FILE handling at all: all we need to do is seek a bit to see how big the file is and read from it. It also changes len to be off_t since now it's using lseek instead of fseek. There is also another small change bundled. gotweb_get_repo_description and clone_url differs in how they handle the "len == 0" case when reading the file. One sets the return to "", the other one leaves NULL. I'm normalizing the behaviour to return NULL in both functions. i thought of splitting it into two, but since dropping the unused path argument part is very small i'm keeping everything in one diff, will separate when committing. ok? diff /home/op/w/got commit - 906ae6c0c06930fb1fc983786177f7de6c4d0403 path + /home/op/w/got blob - b5a140ba22d460fe720b01f0db1a2c26b2d0661f file + gotwebd/got_operations.c --- gotwebd/got_operations.c +++ gotwebd/got_operations.c @@ -108,7 +108,7 @@ got_get_repo_owner(char **owner, struct request *c, ch } const struct got_error * -got_get_repo_owner(char **owner, struct request *c, char *dir) +got_get_repo_owner(char **owner, struct request *c) { struct server *srv = c->srv; struct transport *t = c->t; @@ -134,7 +134,7 @@ got_get_repo_age(char **repo_age, struct request *c, c } const struct got_error * -got_get_repo_age(char **repo_age, struct request *c, char *dir, +got_get_repo_age(char **repo_age, struct request *c, const char *refname, int ref_tm) { const struct got_error *error = NULL; blob - 09d8f166fab40a704104d07aaa4c8e0564295c66 file + gotwebd/gotweb.c --- gotwebd/gotweb.c +++ gotwebd/gotweb.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -93,9 +94,9 @@ static const struct got_error *gotweb_get_repo_descrip static const struct got_error *gotweb_load_got_path(struct request *c, struct repo_dir *); static const struct got_error *gotweb_get_repo_description(char **, - struct server *, char *); + struct server *, int); static const struct got_error *gotweb_get_clone_url(char **, struct server *, - char *); + int); static const struct got_error *gotweb_render_navs(struct request *); static const struct got_error *gotweb_render_blame(struct request *); static const struct got_error *gotweb_render_briefs(struct request *); @@ -1545,8 +1546,7 @@ gotweb_render_branches(struct request *c) if (strncmp(refname, "refs/heads/", 11) != 0) continue; - error = got_get_repo_age(&age, c, qs->path, refname, - TM_DIFF); + error = got_get_repo_age(&age, c, refname, TM_DIFF); if (error) goto done; @@ -2431,7 +2431,6 @@ done: goto err; } - repo = find_cached_repo(srv, repo_dir->path); if (repo == NULL) { error = cache_repo(&repo, srv, repo_dir, sock); @@ -2440,17 +2439,16 @@ done: } t->repo = repo; error = gotweb_get_repo_description(&repo_dir->description, srv, - repo_dir->path); + dirfd(dt)); if (error) goto err; - error = got_get_repo_owner(&repo_dir->owner, c, repo_dir->path); + error = got_get_repo_owner(&repo_dir->owner, c); if (error) goto err; - error = got_get_repo_age(&repo_dir->age, c, repo_dir->path, - NULL, TM_DIFF); + error = got_get_repo_age(&repo_dir->age, c, NULL, TM_DIFF); if (error) goto err; - error = gotweb_get_clone_url(&repo_dir->url, srv, repo_dir->path); + error = gotweb_get_clone_url(&repo_dir->url, srv, dirfd(dt)); err: free(dir_test); if (dt != NULL && closedir(dt) == EOF && error == NULL) @@ -2483,105 +2481,88 @@ gotweb_get_repo_description(char **description, struct } static const struct got_error * -gotweb_get_repo_description(char **description, struct server *srv, char *dir) +gotweb_get_repo_description(char **description, struct server *srv, int dir) { const struct got_error *error = NULL; - FILE *f = NULL; - char *d_file = NULL; - unsigned int len; - size_t n; + int fd = -1; + off_t len; *description = NULL; if (srv->show_repo_description == 0) return NULL; - if (asprintf(&d_file, "%s/description", dir) == -1) - return got_error_from_errno("asprintf"); - - f = fopen(d_file, "r"); - if (f == NULL) { + fd = openat(dir, "description", O_RDONLY); + if (fd == -1) { if (errno != ENOENT && errno != EACCES) - error = got_error_from_errno2("fopen", d_file); + error = got_error_from_errno2("openat", "description"); goto done; } - if (fseek(f, 0, SEEK_END) == -1) { - error = got_ferror(f, GOT_ERR_IO); - goto done; - } - len = ftell(f); + len = lseek(fd, 0, SEEK_END); if (len == -1) { - error = got_ferror(f, GOT_ERR_IO); + error = got_error_from_errno("lseek"); goto done; } - if (len == 0) { - *description = strdup(""); - if (*description == NULL) - error = got_error_from_errno("strdup"); + if (lseek(fd, 0, SEEK_SET) == -1) { + error = got_error_from_errno("fseek"); goto done; } - if (fseek(f, 0, SEEK_SET) == -1) { - error = got_ferror(f, GOT_ERR_IO); + if (len == 0) goto done; - } + + if (len > SIZE_MAX - 1) + len = SIZE_MAX - 1; + *description = calloc(len + 1, sizeof(**description)); if (*description == NULL) { error = got_error_from_errno("calloc"); goto done; } - n = fread(*description, 1, len, f); - if (n == 0 && ferror(f)) - error = got_ferror(f, GOT_ERR_IO); + if (read(fd, *description, len) == -1) + error = got_error_from_errno("read"); done: - if (f != NULL && fclose(f) == EOF && error == NULL) - error = got_error_from_errno("fclose"); - free(d_file); + if (fd != -1 && close(fd) == -1 && error == NULL) + error = got_error_from_errno("close"); return error; } static const struct got_error * -gotweb_get_clone_url(char **url, struct server *srv, char *dir) +gotweb_get_clone_url(char **url, struct server *srv, int dir) { const struct got_error *error = NULL; - FILE *f; - char *d_file = NULL; - unsigned int len; - size_t n; + int fd = -1; + off_t len; *url = NULL; - if (srv->show_repo_cloneurl == 0) return NULL; - if (asprintf(&d_file, "%s/cloneurl", dir) == -1) - return got_error_from_errno("asprintf"); - - f = fopen(d_file, "r"); - if (f == NULL) { + fd = openat(dir, "cloneurl", O_RDONLY); + if (fd == -1) { if (errno != ENOENT && errno != EACCES) - error = got_error_from_errno2("fopen", d_file); + error = got_error_from_errno2("openat", "cloneurl"); goto done; } - if (fseek(f, 0, SEEK_END) == -1) { - error = got_ferror(f, GOT_ERR_IO); - goto done; - } - len = ftell(f); + len = lseek(fd, 0, SEEK_END); if (len == -1) { - error = got_ferror(f, GOT_ERR_IO); + error = got_error_from_errno("lseek"); goto done; } + + if (lseek(fd, 0, SEEK_SET) == -1) { + error = got_error_from_errno("lseek"); + goto done; + } + if (len == 0) goto done; - if (fseek(f, 0, SEEK_SET) == -1) { - error = got_ferror(f, GOT_ERR_IO); - goto done; - } + if (len > SIZE_MAX - 1) + len = SIZE_MAX - 1; *url = calloc(len + 1, sizeof(**url)); if (*url == NULL) { @@ -2589,13 +2570,11 @@ gotweb_get_clone_url(char **url, struct server *srv, c goto done; } - n = fread(*url, 1, len, f); - if (n == 0 && ferror(f)) - error = got_ferror(f, GOT_ERR_IO); + if (read(fd, *url, len) == -1) + error = got_error_from_errno("read"); done: - if (f != NULL && fclose(f) == EOF && error == NULL) + if (fd != -1 && close(fd) == -1 && error == NULL) error = got_error_from_errno("fclose"); - free(d_file); return error; } blob - a8a55276acbd0b209205938c75b88d6918b1c6b8 file + gotwebd/gotwebd.h --- gotwebd/gotwebd.h +++ gotwebd/gotwebd.h @@ -455,8 +455,8 @@ const struct got_error *got_get_repo_owner(char **, st int fcgi_gen_binary_response(struct request *, const uint8_t *, int); /* got_operations.c */ -const struct got_error *got_get_repo_owner(char **, struct request *, char *); -const struct got_error *got_get_repo_age(char **, struct request *, char *, +const struct got_error *got_get_repo_owner(char **, struct request *); +const struct got_error *got_get_repo_age(char **, struct request *, const char *, int); const struct got_error *got_get_repo_commits(struct request *, int); const struct got_error *got_get_repo_tags(struct request *, int);