From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Move got_opentempfd out of got_repo_open
To: gameoftrees@openbsd.org
Date: Sat, 11 Jun 2022 21:54:18 +0200

On Tue, Jun 07, 2022 at 10:22:10AM -0600, Tracey Emery wrote:
> +	for (i = 0; i < GOT_PACK_NUM_TEMPFILES; i++) {
> +		pack_fds_tmp[i] = got_opentempfd();
> +		if (pack_fds_tmp[i] == -1)
> +			return got_error_from_errno("got_opentempfd");

If this fails during the loop then we leak files which
were already opened in earlier loop iterations. This does
not seem intentional since got_repo_pack_fds_close() cannot
handle a partially initialized array, but assumes that every
file descriptor is valid. We should probably close on error,
and let got_repo_pack_fds_close() ignore fds which are initialized
to -1 such that it can be called on a partial array of open fds.

> +	}
> +	memcpy(*pack_fds, pack_fds_tmp, sizeof(pack_fds_tmp));
> +	return err;
> +}
> +
> +const struct got_error *
> +got_repo_pack_fds_copy(int **pack_fds, struct got_repository *repo)
> +{

I see this was added for tog. Can you explain why it is needed?

> @@ -2370,9 +2372,16 @@ open_log_view(struct tog_view *view, struct got_object
>  	view->search_start = search_start_log_view;
>  	view->search_next = search_next_log_view;
>  
> -	err = got_repo_open(&thread_repo, got_repo_get_path(repo), NULL);
> +	err = got_repo_pack_fds_open(&pack_fds);
>  	if (err)
>  		goto done;
> +	err = got_repo_open(&thread_repo, got_repo_get_path(repo), NULL,
> +	    pack_fds);
> +	if (err)
> +		goto done;
> +	err = got_repo_pack_fds_close(pack_fds);
> +	if (err)
> +		goto done;

This looks very strange. Should the temp fds ever be closed before
the repository instance which uses them is closed?

Something else I noticed while reviewing this patch is that some
commands in got.c seem to forget about calling got_repo_close()
on repositories they have opened... that needs to be fixed but
can be done in a separate patch.