From: Omar Polo <op@omarpolo.com>
Subject: Re: gotd session process
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Tue, 10 Jan 2023 12:22:02 +0100

On 2023/01/09 21:39:51 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Mon, Jan 09, 2023 at 01:50:37PM +0100, Stefan Sperling wrote:
> >  add a gotd session process which manipulates files on behalf of the parent
> 
> New diff, rebased on latest main branch which received some relevant fixes.

ok op@, some nits (mostly style) inline.

> --- gotd/gotd.c
> +++ gotd/gotd.c
> [...]
> @@ -1448,383 +1019,45 @@ list_refs_request(struct gotd_client *client, struct g
>  }
>  
>  static const struct got_error *
> -list_refs_request(struct gotd_client *client, struct gotd_imsgev *iev)
> +connect_repo_child(struct gotd_client *client, struct gotd_child_proc *repo_proc)

style nit: i'd fold the line (> 80)

>  {
>  	static const struct got_error *err;
> -	struct gotd_imsg_list_refs_internal ilref;
> -	int fd;
> +	struct gotd_imsgev *session_iev = &client->session->iev;
> +	struct gotd_imsg_connect_repo_child ireq;
> +	int pipe[2];
>  
> [...]
> @@ -2598,7 +2019,7 @@ main(int argc, char **argv)
>  	if (proc_id != PROC_GOTD)
>  		fatal("invalid process id %d", proc_id);
>  
> -	apply_unveil();
> +	apply_unveil_selfexec();
>  
>  	signal_set(&evsigint, SIGINT, gotd_sighdlr, NULL);
>  	signal_set(&evsigterm, SIGTERM, gotd_sighdlr, NULL);
> @@ -2615,6 +2036,8 @@ main(int argc, char **argv)
>  
>  	event_dispatch();
>  
> +	gotd_shutdown();
> +

gotd_shutdown() calls exit so the code below now is dead.  Not a huge
issue (just some close and a free) but worth noting.

>  	if (pack_fds)
>  		got_repo_pack_fds_close(pack_fds);
>  	free(repo_path);
> [...]
> --- /dev/null
> +++ gotd/session.c
> [...]
> +static const struct got_error *
> +begin_ref_updates(struct gotd_session_client *client, struct imsg *imsg)
> +{
> +	struct gotd_imsg_ref_updates_start istart;
> +	size_t datalen;
> +
> +	if (client->nref_updates != -1)
> +		return got_error_msg(GOT_ERR_PRIVSEP_MSG, "begin_ref_updates 1");

fold; > 80

> +	datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
> +	if (datalen != sizeof(istart))
> +		return got_error(GOT_ERR_PRIVSEP_LEN);
> +	memcpy(&istart, imsg->data, sizeof(istart));
> +
> +	if (istart.nref_updates <= 0)
> +		return got_error_msg(GOT_ERR_PRIVSEP_MSG, "begin_ref_updates 2");

fold; > 80

> [...]
> +static void
> +session_dispatch_repo_child(int fd, short event, void *arg)
> +{
> [...]
> +		if (do_disconnect) {
> +			if (err)
> +				disconnect_on_error(client, err);
> +			else
> +				disconnect(client);
> +		} else {
> +			if (do_packfile_install)
> +				err = install_pack(client, gotd_session.repo->path,

fold; > 80

> +				    &imsg);
> +			else if (do_ref_updates)
> +				err = begin_ref_updates(client, &imsg);
> +			else if (do_ref_update)
> +				err = update_ref(client, gotd_session.repo->path,

fold; > 80

> [...]
> +static void
> +session_dispatch_listener(int fd, short events, void *arg)
> +{
> +	struct gotd_imsgev *iev = arg;
> +	struct imsgbuf *ibuf = &iev->ibuf;
> +	struct gotd_session_client *client = &gotd_session_client;
> +	const struct got_error *err = NULL;
> +	struct imsg imsg;
> +	ssize_t n;
> +
> +	if (events & EV_WRITE) {
> [...]
> +	}
> +
> +	if ((events & EV_READ) == 0)
> +		return;
> +
> +	memset(&imsg, 0, sizeof(imsg));

why zeroing imsg?

> +	while (err == NULL) {
> +		err = gotd_imsg_recv(&imsg, ibuf, 0);
> +		if (err) {
> +			if (err->code == GOT_ERR_PRIVSEP_READ)
> +				err = NULL;
> +			break;
> +		}
> +
> +		evtimer_del(&client->tmo);
> +
> +		switch (imsg.hdr.type) {
> [...]
> +		}
> +
> +		imsg_free(&imsg);
> +	}
> +
> +	if (err) {
> +		if (err->code != GOT_ERR_EOF ||
> +		    client->state != GOTD_STATE_EXPECT_PACKFILE)
> +			disconnect_on_error(client, err);
> +	} else {
> +		gotd_imsg_event_add(iev);
> +		evtimer_add(&client->tmo, &gotd_session.request_timeout);
> +	}
> +}