On 2022/11/15 09:56:23 +0100, Omar Polo <op@omarpolo.com> wrote:
> On 2022/11/09 19:54:41 +0100, Omar Polo <op@omarpolo.com> 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.

clarified this point with Tracey, the intent is to never end up
passing NULL to fcgi_printf but since we already fall back to "" if
the description or the clone url is NULL I think it's fine.  Tested
with both file absent and/or empty and gotwebd doesn't print "(null)".

On a second thought we could drop a few more lines and don't special
casing empty files.  We would end up allocating one byte, read zero
into it and return.  Don't know if it's preferable, I'm being seduced
by having more - in the diff.

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 <dirent.h>
 #include <errno.h>
 #include <event.h>
+#include <fcntl.h>
 #include <imsg.h>
 #include <sha1.h>
 #include <stdio.h>
@@ -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,119 +2481,94 @@ 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);
-		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 (len == 0)
-		goto done;
 
-	if (fseek(f, 0, SEEK_SET) == -1) {
-		error = got_ferror(f, GOT_ERR_IO);
+	if (lseek(fd, 0, SEEK_SET) == -1) {
+		error = got_error_from_errno("lseek");
 		goto done;
 	}
 
+	if (len > SIZE_MAX - 1)
+		len = SIZE_MAX - 1;
+
 	*url = calloc(len + 1, sizeof(**url));
 	if (*url == NULL) {
 		error = got_error_from_errno("calloc");
 		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);