"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
repo disconect cleanup
To:
gameoftrees@openbsd.org
Date:
Thu, 9 Feb 2023 17:47:25 +0100

Download raw body.

Thread
Some follow-up tweaks related to the gotd segfault fix from
commit 90270f794c7f4e1ce2b58c8b5bfdc190df4799b6

-----------------------------------------------
 
 do not expect to see a DISCONNECT message in repo processes
 
 The parent no longer sends this message. Perform related cleanup
 in the shutdown path instead.
 
diff 90270f794c7f4e1ce2b58c8b5bfdc190df4799b6 94668037c826c7562bb0b036b9355a7bd88185b4
commit - 90270f794c7f4e1ce2b58c8b5bfdc190df4799b6
commit + 94668037c826c7562bb0b036b9355a7bd88185b4
blob - 658362effe9f7a546e607cdc600ad87c21b66f60
blob + 050a774d3a1387573a851619c802261f01af1e1f
--- gotd/repo_read.c
+++ gotd/repo_read.c
@@ -286,8 +286,6 @@ list_refs(struct imsg *imsg)
 	}
 	client->id = ireq.client_id;
 	client->fd = client_fd;
-	client->delta_cache_fd = -1;
-	client->pack_pipe = -1;
 
 	imsg_init(&ibuf, client_fd);
 
@@ -670,35 +668,6 @@ static const struct got_error *
 	return err;
 }
 
-static const struct got_error *
-recv_disconnect(struct imsg *imsg)
-{
-	const struct got_error *err = NULL;
-	struct gotd_imsg_disconnect idisconnect;
-	size_t datalen;
-	int delta_cache_fd, pack_pipe;
-	struct repo_read_client *client = &repo_read_client;
-
-	datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
-	if (datalen != sizeof(idisconnect))
-		return got_error(GOT_ERR_PRIVSEP_LEN);
-	memcpy(&idisconnect, imsg->data, sizeof(idisconnect));
-
-	log_debug("client disconnecting");
-
-	free_object_ids(&client->have_ids);
-	free_object_ids(&client->want_ids);
-	if (close(client->fd) == -1)
-		err = got_error_from_errno("close");
-	delta_cache_fd = client->delta_cache_fd;
-	if (delta_cache_fd != -1 && close(delta_cache_fd) == -1 && err == NULL)
-		return got_error_from_errno("close");
-	pack_pipe = client->pack_pipe;
-	if (pack_pipe != -1 && close(pack_pipe) == -1 && err == NULL)
-		return got_error_from_errno("close");
-	return err;
-}
-
 static void
 repo_read_dispatch_session(int fd, short event, void *arg)
 {
@@ -774,13 +743,6 @@ repo_read_dispatch_session(int fd, short event, void *
 				log_warnx("%s: sending packfile: %s",
 				    repo_read.title, err->msg);
 			break;
-		case GOTD_IMSG_DISCONNECT:
-			err = recv_disconnect(&imsg);
-			if (err)
-				log_warnx("%s: disconnect: %s",
-				    repo_read.title, err->msg);
-			shut = 1;
-			break;
 		default:
 			log_debug("%s: unexpected imsg %d", repo_read.title,
 			    imsg.hdr.type);
@@ -898,8 +860,13 @@ repo_read_main(const char *title, const char *repo_pat
     int *pack_fds, int *temp_fds)
 {
 	const struct got_error *err = NULL;
+	struct repo_read_client *client = &repo_read_client;
 	struct gotd_imsgev iev;
 
+	client->fd = -1;
+	client->delta_cache_fd = -1;
+	client->pack_pipe = -1;
+
 	repo_read.title = title;
 	repo_read.pid = getpid();
 	repo_read.pack_fds = pack_fds;
@@ -945,7 +912,19 @@ repo_read_shutdown(void)
 void
 repo_read_shutdown(void)
 {
+	struct repo_read_client *client = &repo_read_client;
+
 	log_debug("%s: shutting down", repo_read.title);
+
+	free_object_ids(&client->have_ids);
+	free_object_ids(&client->want_ids);
+	if (client->fd != -1)
+		close(client->fd);
+	if (client->delta_cache_fd != -1)
+		close(client->delta_cache_fd);
+	if (client->pack_pipe != -1)
+		close(client->pack_pipe);
+
 	if (repo_read.repo)
 		got_repo_close(repo_read.repo);
 	got_repo_pack_fds_close(repo_read.pack_fds);
blob - b425ba49c41d4c20c076b2c6c6a77b84366abf0d
blob + ea3be8ec0af54941dbfa5189f510f3a126c52d2b
--- gotd/repo_write.c
+++ gotd/repo_write.c
@@ -257,8 +257,6 @@ list_refs(struct imsg *imsg)
 	}
 	client->id = ireq.client_id;
 	client->fd = client_fd;
-	client->pack_pipe = -1;
-	client->packidx_fd = -1;
 	client->nref_updates = 0;
 	client->nref_del = 0;
 	client->nref_new = 0;
@@ -1196,8 +1194,6 @@ recv_disconnect(struct imsg *imsg)
 	const struct got_error *err = NULL;
 	struct gotd_imsg_disconnect idisconnect;
 	size_t datalen;
-	int pack_pipe = -1, idxfd = -1;
-	struct repo_write_client *client = &repo_write_client;
 
 	datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
 	if (datalen != sizeof(idisconnect))
@@ -1206,22 +1202,6 @@ recv_disconnect(struct imsg *imsg)
 
 	log_debug("client disconnecting");
 
-	while (!STAILQ_EMPTY(&client->ref_updates)) {
-		struct gotd_ref_update *ref_update;
-		ref_update = STAILQ_FIRST(&client->ref_updates);
-		STAILQ_REMOVE_HEAD(&client->ref_updates, entry);
-		got_ref_close(ref_update->ref);
-		free(ref_update);
-	}
-	err = got_pack_close(&client->pack);
-	if (client->fd != -1 && close(client->fd) == -1)
-		err = got_error_from_errno("close");
-	pack_pipe = client->pack_pipe;
-	if (pack_pipe != -1 && close(pack_pipe) == -1 && err == NULL)
-		err = got_error_from_errno("close");
-	idxfd = client->packidx_fd;
-	if (idxfd != -1 && close(idxfd) == -1 && err == NULL)
-		err = got_error_from_errno("close");
 	return err;
 }
 
@@ -1491,8 +1471,14 @@ repo_write_main(const char *title, const char *repo_pa
     int *pack_fds, int *temp_fds)
 {
 	const struct got_error *err = NULL;
+	struct repo_write_client *client = &repo_write_client;
 	struct gotd_imsgev iev;
 
+	client->fd = -1;
+	client->pack_pipe = -1;
+	client->packidx_fd = -1;
+	client->pack.fd = -1;
+
 	repo_write.title = title;
 	repo_write.pid = getpid();
 	repo_write.pack_fds = pack_fds;
@@ -1539,7 +1525,26 @@ repo_write_shutdown(void)
 void
 repo_write_shutdown(void)
 {
+	struct repo_write_client *client = &repo_write_client;
+	struct gotd_ref_update *ref_update;
+
 	log_debug("%s: shutting down", repo_write.title);
+
+	while (!STAILQ_EMPTY(&client->ref_updates)) {
+		ref_update = STAILQ_FIRST(&client->ref_updates);
+		STAILQ_REMOVE_HEAD(&client->ref_updates, entry);
+		got_ref_close(ref_update->ref);
+		free(ref_update);
+	}
+
+	got_pack_close(&client->pack);
+	if (client->fd != -1)
+		close(client->fd);
+	if (client->pack_pipe != -1)
+		close(client->pack_pipe);
+	if (client->packidx_fd != -1)
+		close(client->packidx_fd);
+
 	if (repo_write.repo)
 		got_repo_close(repo_write.repo);
 	got_repo_pack_fds_close(repo_write.pack_fds);