From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Move got_opentempfd out of got_repo_open
To: gameoftrees@openbsd.org
Date: Thu, 2 Jun 2022 12:51:46 +0200

On Wed, Jun 01, 2022 at 10:22:49AM -0600, Tracey Emery wrote:
> Hello,
> 
> Here is a first attempt to move got_opentempfd out of got_repo_open. Is
> this direction ok? I am not asking for a commit ok yet, just starting a
> conversation about the direction.
> 
> This creates an array of fds twice the size of GOT_PACK_CACHE_SIZE to
> pass to got_repo_open by the caller.
> 
> Thoughts? This move will remove the need for gotwebd to have
> /var/www/got/tmp finally.

Not yet really, because at least lib/diffreg.c and lib/blame.c do
still have internal calls to got_opentemp().

> diff 2497f032fa6bc06264d8990fdd57a9ffbaf1429b /home/tracey/src/got
> blob - cf96009500ac33bb3b660dd7e99902801b0cb975
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -715,6 +715,8 @@ cmd_import(int argc, char *argv[])
>  	struct got_pathlist_head ignores;
>  	struct got_pathlist_entry *pe;
>  	int preserve_logmsg = 0;
> +	int cs = got_repo_get_pack_cache_size();
> +	int pack_fds[cs];

I think we should get rid of got_repo_get_pack_cache_size().

Instead, we could declare GOT_PACK_CACHE_SIZE in a public header,
and perhaps declare GOT_PACK_CACHE_SIZE * 2 as another macro
with a different name, which could then be used to set the
size of the pack_fds array at compile-time.

Alternatively, allocate the array dynamically in got_repo_pack_fds_init()
and return the number of elements in addition to returning the array, so
the caller could do something like this:

  int *fds = NULL;
  size_t nfds;

  err = got_repo_pack_fds_init(&fds, &nfds);
  [...]
done:
  got_repo_pack_fds_close(fds); /* closes files and then does a free(fds) */


And got_repo_pack_fds_open() might be a better name than _init().

> @@ -703,12 +742,12 @@ got_repo_open(struct got_repository **repop, const cha
>  		repo->pack_cache_size = rl.rlim_cur / 8;
>  	for (i = 0; i < nitems(repo->packs); i++) {
>  		if (i < repo->pack_cache_size) {
> -			repo->packs[i].basefd = got_opentempfd();
> +			repo->packs[i].basefd = dup(pack_fds[j++]);

Is use of dup() really necessary?
This doubles the amount of file descriptors required, doesn't it?
Is it not enough to require the caller to create one set of file
descriptors per repository instance they want to use?