From: Omar Polo Subject: Re: repo disconect cleanup To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Thu, 09 Feb 2023 18:31:28 +0100 On 2023/02/09 17:47:25 +0100, Stefan Sperling wrote: > Some follow-up tweaks related to the gotd segfault fix from > commit 90270f794c7f4e1ce2b58c8b5bfdc190df4799b6 i like it! since repo_read/repo_write are per-process now it's cleaner to have the initialization in main() and cleanup in shutdown. ok op@ > ----------------------------------------------- > > 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);