From: Omar Polo Subject: simplify got_gotweb_openfile/flushfile To: gameoftrees@openbsd.org Date: Mon, 15 May 2023 17:44:41 +0200 it's a bit confusing to follow some parts due to all the various fN and fdN variable involved. Furthermore, we don't need to return both the FILE* and the file descriptor number in got_gotweb_openfile(), just the former is enough. Removing this "tuple" of FILE* and fd allows to make more easy to spot what's going on. 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. not ran on my instance, just on localhost, but should be straightforward. ok? diff /home/op/w/gotacl commit - ba0bed23e515b14cd3fe877c6847785c8c29971e path + /home/op/w/gotacl blob - f0ec1939cb99398076236b923bf5a669bb2956a2 file + gotwebd/got_operations.c --- gotwebd/got_operations.c +++ gotwebd/got_operations.c @@ -48,41 +48,40 @@ static const struct got_error *got_gotweb_openfile(FIL struct repo_commit *, struct got_commit_object *, struct got_reflist_head *, struct got_object_id *); static const struct got_error *got_gotweb_dupfd(int *, int *); -static const struct got_error *got_gotweb_openfile(FILE **, int *, int *); +static const struct got_error *got_gotweb_openfile(FILE **, int *); static const struct got_error *got_gotweb_blame_cb(void *, int, int, struct got_commit_object *,struct got_object_id *); const struct got_error * -got_gotweb_flushfile(FILE *f, int fd) +got_gotweb_flushfile(FILE *f) { if (fseek(f, 0, SEEK_SET) == -1) return got_error_from_errno("fseek"); - if (ftruncate(fd, 0) == -1) + if (ftruncate(fileno(f), 0) == -1) return got_error_from_errno("ftruncate"); - if (fsync(fd) == -1) + if (fsync(fileno(f)) == -1) return got_error_from_errno("fsync"); - if (f && fclose(f) == EOF) + if (fclose(f) == EOF) return got_error_from_errno("fclose"); - if (fd != -1 && close(fd) != -1) - return got_error_from_errno("close"); - return NULL; } static const struct got_error * -got_gotweb_openfile(FILE **f, int *priv_fd, int *fd) +got_gotweb_openfile(FILE **f, int *priv_fd) { - *fd = dup(*priv_fd); - if (*fd == -1) + int fd; + + fd = dup(*priv_fd); + if (fd == -1) return got_error_from_errno("dup"); - *f = fdopen(*fd, "w+"); + *f = fdopen(fd, "w+"); if (*f == NULL) { - close(*fd); + close(fd); return got_error(GOT_ERR_PRIVSEP_NO_FD); } @@ -1012,8 +1011,7 @@ got_output_file_blame(struct request *c, got_render_bl struct got_blob_object *blob = NULL; char *path = NULL, *in_repo_path = NULL; struct blame_cb_args bca; - int i, obj_type, fd1 = -1, fd2 = -1, fd3 = -1, fd4 = -1, fd5 = -1; - int fd6 = -1; + int i, obj_type, fd2 = -1, fd3 = -1, fd4 = -1; off_t filesize; FILE *f1 = NULL, *f2 = NULL; @@ -1054,7 +1052,7 @@ got_output_file_blame(struct request *c, got_render_bl goto done; } - error = got_gotweb_openfile(&bca.f, &c->priv_fd[BLAME_FD_1], &fd1); + error = got_gotweb_openfile(&bca.f, &c->priv_fd[BLAME_FD_1]); if (error) goto done; @@ -1098,11 +1096,11 @@ got_output_file_blame(struct request *c, got_render_bl if (error) goto done; - error = got_gotweb_openfile(&f1, &c->priv_fd[BLAME_FD_5], &fd5); + error = got_gotweb_openfile(&f1, &c->priv_fd[BLAME_FD_5]); if (error) goto done; - error = got_gotweb_openfile(&f2, &c->priv_fd[BLAME_FD_6], &fd6); + error = got_gotweb_openfile(&f2, &c->priv_fd[BLAME_FD_6]); if (error) goto done; @@ -1127,20 +1125,17 @@ done: if (fd4 != -1 && close(fd4) == -1 && error == NULL) error = got_error_from_errno("close"); if (bca.f) { - const struct got_error *bca_err = - got_gotweb_flushfile(bca.f, fd1); + const struct got_error *bca_err = got_gotweb_flushfile(bca.f); if (error == NULL) error = bca_err; } if (f1) { - const struct got_error *f1_err = - got_gotweb_flushfile(f1, fd5); + const struct got_error *f1_err = got_gotweb_flushfile(f1); if (error == NULL) error = f1_err; } if (f2) { - const struct got_error *f2_err = - got_gotweb_flushfile(f2, fd6); + const struct got_error *f2_err = got_gotweb_flushfile(f2); if (error == NULL) error = f2_err; } @@ -1157,7 +1152,7 @@ got_open_diff_for_output(FILE **fp, int *fd, struct re } const struct got_error * -got_open_diff_for_output(FILE **fp, int *fd, struct request *c) +got_open_diff_for_output(FILE **fp, struct request *c) { const struct got_error *error = NULL; struct transport *t = c->t; @@ -1166,22 +1161,21 @@ got_open_diff_for_output(FILE **fp, int *fd, struct re struct got_object_id *id1 = NULL, *id2 = NULL; struct got_reflist_head refs; FILE *f1 = NULL, *f2 = NULL, *f3 = NULL; - int obj_type, fd1, fd2, fd3, fd4 = -1, fd5 = -1; + int obj_type, fd4 = -1, fd5 = -1; *fp = NULL; - *fd = -1; TAILQ_INIT(&refs); - error = got_gotweb_openfile(&f1, &c->priv_fd[DIFF_FD_1], &fd1); + error = got_gotweb_openfile(&f1, &c->priv_fd[DIFF_FD_1]); if (error) return error; - error = got_gotweb_openfile(&f2, &c->priv_fd[DIFF_FD_2], &fd2); + error = got_gotweb_openfile(&f2, &c->priv_fd[DIFF_FD_2]); if (error) return error; - error = got_gotweb_openfile(&f3, &c->priv_fd[DIFF_FD_3], &fd3); + error = got_gotweb_openfile(&f3, &c->priv_fd[DIFF_FD_3]); if (error) return error; @@ -1251,7 +1245,6 @@ got_open_diff_for_output(FILE **fp, int *fd, struct re } *fp = f3; - *fd = fd3; done: if (fd4 != -1 && close(fd4) == -1 && error == NULL) @@ -1259,21 +1252,18 @@ done: if (fd5 != -1 && close(fd5) == -1 && error == NULL) error = got_error_from_errno("close"); if (f1) { - const struct got_error *f1_err = - got_gotweb_flushfile(f1, fd1); + const struct got_error *f1_err = got_gotweb_flushfile(f1); if (error == NULL) error = f1_err; } if (f2) { - const struct got_error *f2_err = - got_gotweb_flushfile(f2, fd2); + const struct got_error *f2_err = got_gotweb_flushfile(f2); if (error == NULL) error = f2_err; } if (error && f3) { - got_gotweb_flushfile(f3, fd3); + got_gotweb_flushfile(f3); *fp = NULL; - *fd = -1; } got_ref_list_free(&refs); free(id1); blob - 4920a6dec9f53186127eaaca71614011c8122a7d file + gotwebd/gotweb.c --- gotwebd/gotweb.c +++ gotwebd/gotweb.c @@ -288,7 +288,7 @@ gotweb_process_request(struct request *c) log_warnx("%s: %s", __func__, error->msg); goto err; } - error = got_open_diff_for_output(&c->t->fp, &c->t->fd, c); + error = got_open_diff_for_output(&c->t->fp, c); if (error) { log_warnx("%s: %s", __func__, error->msg); goto err; @@ -743,11 +743,10 @@ gotweb_free_transport(struct transport *t) if (t->blob) got_object_blob_close(t->blob); if (t->fp) { - err = got_gotweb_flushfile(t->fp, t->fd); + err = got_gotweb_flushfile(t->fp); if (err) log_warnx("%s: got_gotweb_flushfile failure: %s", __func__, err->msg); - t->fd = -1; } if (t->fd != -1) close(t->fd); blob - acf150d5111ada020e1d820e569e463a0cdaa4e5 file + gotwebd/gotwebd.h --- gotwebd/gotwebd.h +++ gotwebd/gotwebd.h @@ -506,15 +506,14 @@ const struct got_error *got_gotweb_flushfile(FILE *, i int fcgi_gen_binary_response(struct request *, const uint8_t *, int); /* got_operations.c */ -const struct got_error *got_gotweb_flushfile(FILE *, int); +const struct got_error *got_gotweb_flushfile(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 *); const struct got_error *got_get_repo_commits(struct request *, int); const struct got_error *got_get_repo_tags(struct request *, int); const struct got_error *got_get_repo_heads(struct request *); -const struct got_error *got_open_diff_for_output(FILE **, int *, - struct request *); +const struct got_error *got_open_diff_for_output(FILE **, struct request *); int got_output_repo_tree(struct request *, int (*)(struct template *, struct got_tree_entry *)); const struct got_error *got_open_blob_for_output(struct got_blob_object **,