On 2023/02/09 17:47:25 +0100, Stefan Sperling <stsp@stsp.name> 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);