On 2022/11/22 19:23:17 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Nov 22, 2022 at 12:07:49PM +0100, Omar Polo wrote:
> > -	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");
> 
> The error message previously contained the whole path.
> It might be more useful to see an absolute path in error logs.
> If you want to retain this you could pass both the path to the directory
> and the open fd, and then use got_error_from_errno_fmt() here.

ah, right.  for some errors it's useful to log the full path!

> Same applies to gotweb_get_clone_url().
> 
> >  		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;
> 
> This could instead use fstat() and read sb.st_size. That is what
> we usually do and avoids having to seek back to the beginning below.

agreed.  I kept the seeks because it was already there, but fstat is cleaner.

Thanks!

P.S.: the check len < SIZE_MAX - 1 is there just to avoid the implicit
cast.  It's silly to try to calloc a number like that, but here i'd
like to focus on just switchi to openat and reducing the seeks, a
sensible upper limit can be discussed and applied later.

-----------------------------------------------
commit 71372d239470c3b6f689f6aae04cf14de1f359c2 (w)
from: Omar Polo <op@omarpolo.com>
date: Tue Nov 22 18:38:31 2022 UTC
 
 gotwebd: rework gotweb_get_repo_{description,cloneurl}
 
  - use openat(2) since we've already opened the containing dir
  - simplify reading the files
 
diff 39afa5819b25cb2a5a8411de6f16d1e5667bb2c7 71372d239470c3b6f689f6aae04cf14de1f359c2
commit - 39afa5819b25cb2a5a8411de6f16d1e5667bb2c7
commit + 71372d239470c3b6f689f6aae04cf14de1f359c2
blob - 87aa73baf749f6c131882947826effd5278c6c05
blob + 6991dbcb9481482215de315ffb2caf894bad5350
--- 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 *, const char *, int);
 static const struct got_error *gotweb_get_clone_url(char **, struct server *,
-    char *);
+    const 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 *);
@@ -2438,7 +2439,7 @@ done:
 	}
 	t->repo = repo;
 	error = gotweb_get_repo_description(&repo_dir->description, srv,
-	    repo_dir->path);
+	    repo_dir->path, dirfd(dt));
 	if (error)
 		goto err;
 	error = got_get_repo_owner(&repo_dir->owner, c);
@@ -2447,7 +2448,8 @@ done:
 	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, repo_dir->path,
+	    dirfd(dt));
 err:
 	free(dir_test);
 	if (dt != NULL && closedir(dt) == EOF && error == NULL)
@@ -2480,105 +2482,82 @@ 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,
+    const char *dirpath, int dir)
 {
 	const struct got_error *error = NULL;
-	FILE *f = NULL;
-	char *d_file = NULL;
-	unsigned int len;
-	size_t n;
+	struct stat sb;
+	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) {
-		if (errno != ENOENT && errno != EACCES)
-			error = got_error_from_errno2("fopen", d_file);
+	fd = openat(dir, "description", O_RDONLY);
+	if (fd == -1) {
+		if (errno != ENOENT && errno != EACCES) {
+			error = got_error_from_errno_fmt("openat %s/%s",
+			    dirpath, "description");
+		}
 		goto done;
 	}
 
-	if (fseek(f, 0, SEEK_END) == -1) {
-		error = got_ferror(f, GOT_ERR_IO);
+	if (fstat(fd, &sb) == -1) {
+		error = got_error_from_errno_fmt("fstat %s/%s",
+		    dirpath, "description");
 		goto done;
 	}
-	len = ftell(f);
-	if (len == -1) {
-		error = got_ferror(f, GOT_ERR_IO);
-		goto done;
-	}
 
-	if (len == 0) {
-		*description = strdup("");
-		if (*description == NULL)
-			error = got_error_from_errno("strdup");
-		goto done;
-	}
+	len = sb.st_size;
+	if (len > SIZE_MAX - 1)
+		len = SIZE_MAX - 1;
 
-	if (fseek(f, 0, SEEK_SET) == -1) {
-		error = got_ferror(f, GOT_ERR_IO);
-		goto done;
-	}
 	*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, const char *dirpath,
+    int dir)
 {
 	const struct got_error *error = NULL;
-	FILE *f;
-	char *d_file = NULL;
-	unsigned int len;
-	size_t n;
+	struct stat sb;
+	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) {
-		if (errno != ENOENT && errno != EACCES)
-			error = got_error_from_errno2("fopen", d_file);
+	fd = openat(dir, "cloneurl", O_RDONLY);
+	if (fd == -1) {
+		if (errno != ENOENT && errno != EACCES) {
+			error = got_error_from_errno_fmt("openat %s/%s",
+			    dirpath, "cloneurl");
+		}
 		goto done;
 	}
 
-	if (fseek(f, 0, SEEK_END) == -1) {
-		error = got_ferror(f, GOT_ERR_IO);
+	if (fstat(fd, &sb) == -1) {
+		error = got_error_from_errno_fmt("fstat %s/%s",
+		    dirpath, "cloneurl");
 		goto done;
 	}
-	len = ftell(f);
-	if (len == -1) {
-		error = got_ferror(f, GOT_ERR_IO);
-		goto done;
-	}
-	if (len == 0)
-		goto done;
 
-	if (fseek(f, 0, SEEK_SET) == -1) {
-		error = got_ferror(f, GOT_ERR_IO);
-		goto done;
-	}
+	len = sb.st_size;
+	if (len > SIZE_MAX - 1)
+		len = SIZE_MAX - 1;
 
 	*url = calloc(len + 1, sizeof(**url));
 	if (*url == NULL) {
@@ -2586,13 +2565,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)
-		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;
 }