From: Omar Polo <op@omarpolo.com>
Subject: Re: simplify got_gotweb_openfile/flushfile
To: "Todd C. Miller" <millert@openbsd.org>
Cc: gameoftrees@openbsd.org
Date: Tue, 16 May 2023 10:35:08 +0200

On 2023/05/15 13:08:38 -0600, Todd C. Miller <millert@openbsd.org> wrote:
> On Mon, 15 May 2023 17:44:41 +0200, Omar Polo wrote:
> > I'll rename the various fdN in a follow up commit, otherwise diff
> > below would have been unreadable.  I'll also follow up to with a
> > change for got_gotweb_flushfile() to always close the file.  and to
> > log eventual close() errors in gotweb_free_transport() too.

I've committed the diff, the fdN rename and the missing logging in
gotweb_free_transport().

> When you do this, maybe consider renaming got_gotweb_flushfile()
> to got_gotweb_closefile().

Agreed.  I think "flush" came from the fsync() usage?  Don't know.

I'm not too sure about the usefulness of fsync() here by the way.  The
files obtained via got_gotweb_opentemp() are scratch and only used for
temporary storage of blobs and diffs.  Rewinding and truncating is
correct, otherwise we might output data from previous requests, but
flushing the changes to disk seems forcing unnecessary I/O to me.

Here's a diff with the rename and that makes it always closing the
file, even if a failure occurs inbetween.  fseek and ftruncate
shouldn't really fail however, so I'm likely being too paranoid.

The missing context is that patrick@ reported that he ran into

: gotweb_process_request: ftruncate: Bad file descriptor

got_gotweb_flushfile() (with its double close()) was my major suspect.
It's strange however to get EBADF ftruncate() since we should really
get it from fseek().


Thanks!


diff /home/op/w/gotacl
commit - 276bccc4651afa03134d0faa1cf1c703b5f819bd
path + /home/op/w/gotacl
blob - 8badab72875c91358f1c37cf5df22ced9e96953a
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -53,21 +53,23 @@ got_gotweb_flushfile(FILE *f)
     struct got_commit_object *,struct got_object_id *);
 
 const struct got_error *
-got_gotweb_flushfile(FILE *f)
+got_gotweb_closefile(FILE *f)
 {
+	const struct got_error *err = NULL;
+
 	if (fseek(f, 0, SEEK_SET) == -1)
-		return got_error_from_errno("fseek");
+		err = got_error_from_errno("fseek");
 
-	if (ftruncate(fileno(f), 0) == -1)
-		return got_error_from_errno("ftruncate");
+	if (err == NULL && ftruncate(fileno(f), 0) == -1)
+		err = got_error_from_errno("ftruncate");
 
-	if (fsync(fileno(f)) == -1)
-		return got_error_from_errno("fsync");
+	if (err == NULL && fsync(fileno(f)) == -1)
+		err = got_error_from_errno("fsync");
 
-	if (fclose(f) == EOF)
-		return got_error_from_errno("fclose");
+	if (fclose(f) == EOF && err == NULL)
+		err = got_error_from_errno("fclose");
 
-	return NULL;
+	return err;
 }
 
 static const struct got_error *
@@ -1125,17 +1127,17 @@ done:
 	if (fd2 != -1 && close(fd2) == -1 && error == NULL)
 		error = got_error_from_errno("close");
 	if (bca.f) {
-		const struct got_error *bca_err = got_gotweb_flushfile(bca.f);
+		const struct got_error *bca_err = got_gotweb_closefile(bca.f);
 		if (error == NULL)
 			error = bca_err;
 	}
 	if (f1) {
-		const struct got_error *f1_err = got_gotweb_flushfile(f1);
+		const struct got_error *f1_err = got_gotweb_closefile(f1);
 		if (error == NULL)
 			error = f1_err;
 	}
 	if (f2) {
-		const struct got_error *f2_err = got_gotweb_flushfile(f2);
+		const struct got_error *f2_err = got_gotweb_closefile(f2);
 		if (error == NULL)
 			error = f2_err;
 	}
@@ -1252,17 +1254,17 @@ done:
 	if (fd2 != -1 && close(fd2) == -1 && error == NULL)
 		error = got_error_from_errno("close");
 	if (f1) {
-		const struct got_error *f1_err = got_gotweb_flushfile(f1);
+		const struct got_error *f1_err = got_gotweb_closefile(f1);
 		if (error == NULL)
 			error = f1_err;
 	}
 	if (f2) {
-		const struct got_error *f2_err = got_gotweb_flushfile(f2);
+		const struct got_error *f2_err = got_gotweb_closefile(f2);
 		if (error == NULL)
 			error = f2_err;
 	}
 	if (error && f3) {
-		got_gotweb_flushfile(f3);
+		got_gotweb_closefile(f3);
 		*fp = NULL;
 	}
 	got_ref_list_free(&refs);
blob - b6d0cc835948d84bab46566884f83f5ffb33f43b
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -743,9 +743,9 @@ gotweb_free_transport(struct transport *t)
 	if (t->blob)
 		got_object_blob_close(t->blob);
 	if (t->fp) {
-		err = got_gotweb_flushfile(t->fp);
+		err = got_gotweb_closefile(t->fp);
 		if (err)
-			log_warnx("%s: got_gotweb_flushfile failure: %s",
+			log_warnx("%s: got_gotweb_closefile failure: %s",
 			    __func__, err->msg);
 	}
 	if (t->fd != -1 && close(t->fd) == -1)
blob - 56d5af6cb41cbb9f36d922d1044540bccdefe980
file + gotwebd/gotwebd.h
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -506,7 +506,7 @@ const struct got_error *got_gotweb_flushfile(FILE *);
 int fcgi_gen_binary_response(struct request *, const uint8_t *, int);
 
 /* got_operations.c */
-const struct got_error *got_gotweb_flushfile(FILE *);
+const struct got_error *got_gotweb_closefile(FILE *);
 const struct got_error *got_get_repo_owner(char **, struct request *);
 const struct got_error *got_get_repo_age(time_t *, struct request *,
     const char *);