From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Move got_opentempfd out of got_repo_open
To: gameoftrees@openbsd.org
Date: Tue, 14 Jun 2022 09:58:39 +0200

On Mon, Jun 13, 2022 at 02:41:28PM -0600, Tracey Emery wrote:
> On Sat, Jun 11, 2022 at 09:54:18PM +0200, Stefan Sperling wrote:
> > 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.
> >
> 
> See fixes for this in the diff below.
> 
> > > +	}
> > > +	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?
> > 
> 
> Here, got_repo_open accepts a single array of fds to work with. This
> function copies the opened fds from repo->packs into a single array for
> run_blame, since we need to use our existing fds here, which are split
> up between accumfd and basefd. This cannot be inlined, since we can't
> work directly on the repo struct in tog or got.
> 
> Perhaps you can think of a better way to do this for tog? I'm open to
> suggestions. :)

So you did this because tog creates a separate repo instance for its
blame thread, since our library code isn't thread-safe with an instance
of struct got_repository being shared across threads.

The main thread isn't actually using this repo once the blame operation
has started, but it has already opened the files. You want to avoid
closing pack_fds in stop_blame() and re-opening them again in run_blame()?
Is there a reason other than efficiency for avoiding this?
In the log-thread case, which is similar, you have instead chosen to close
and re-open.

I think we'll need a different name for got_repo_pack_fds_copy() if we
end up keeping this function. Something other than "copy"; "copy" implies
that duplicates of these file descriptors are being created, which is not
what this function does. Perhaps got_repo_pack_fds_get()?
/* Obtain the array of open pack file descriptors used by a repository. */
const struct got_error *got_repo_pack_fds_get(int **, struct got_repository *);

> @@ -2311,8 +2331,12 @@ cmd_fetch(int argc, char *argv[])
>  		goto done;
>  	}
>  
> +	error = got_repo_pack_fds_open(&pack_fds);
> +	if (error != NULL)
> +		goto done;
> +
>  	if (repo_path == NULL) {
> -		error = got_worktree_open(&worktree, cwd);
> +		error = got_worktree_open(&worktree, cwd, pack_fds);
>  		if (error && error->code != GOT_ERR_NOT_WORKTREE)
>  			goto done;
>  		else

It is unfortunate to have got_worktree_open depend on pack_fds.
The reason is that the work tree base commit ID is being resolved
at open() time. I will try to find a way to avoid this to allow you
to drop all of these chunks from your diff if possible.

> blob - fff58b083aa8eadb970ebd09ce593455d223fcc9
> blob + c148b0c64ac4412a9bfc2da3e193337b4cc91238
> --- lib/pack_create.c
> +++ lib/pack_create.c
> @@ -1520,21 +1520,13 @@ load_packed_tree_ids(void *arg, struct got_tree_object
>  	if (tree == NULL) {
>  		free(a->id);
>  		a->id = got_object_id_dup(id);
> -		if (a->id == NULL) {
> -			err = got_error_from_errno("got_object_id_dup");
> -			free(a->dpath);
> -			a->dpath = NULL;
> -			return err;
> -		}
> +		if (a->id == NULL)
> +			return got_error_from_errno("got_object_id_dup");
>  
>  		free(a->dpath);
>  		a->dpath = strdup(dpath);
> -		if (a->dpath == NULL) {
> -			err = got_error_from_errno("strdup");
> -			free(a->id);
> -			a->id = NULL;
> -			return err;
> -		}
> +		if (a->dpath == NULL)
> +			return got_error_from_errno("strdup");
>  
>  		a->mtime = mtime;
>  		return NULL;

The above looks like a rebase gone wrong. This change re-introduces leaks
that were present in an earlier version of my object-enumerate patch.

> @@ -4162,7 +4202,7 @@ draw_blame(struct tog_view *view)
>  			width = 9;
>  			wline = wcsdup(L"");
>  			if (wline == NULL) {
> -				err = got_error_from_errno("wcsdup");
> +				err = got_error_from_errno("wcsup");
>  				free(line);
>  				return err;

This change does not look like it was intentional either?