Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Hi Junio,

On 2015-10-05 22:57, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
>> There was a lot of repeated code to close the file descriptor of
>> a given pack. Let's just refactor this code into a single function.
> 
> That is a very good idea, but...
> 
>> +static int close_pack_fd(struct packed_git *p)
>> +{
>> +	if (p->pack_fd < 0)
>> +		return 0;
> 
> Is this "return 0" compatible with ...
> 
>> +	close(p->pack_fd);
>> +	pack_open_fds--;
>> +	p->pack_fd = -1;
>> +
>> +	return 1;
>> +}
>> +
>>  /*
>>   * The LRU pack is the one with the oldest MRU window, preferring packs
>>   * with no used windows, or the oldest mtime if it has no windows allocated.
>> @@ -853,12 +865,8 @@ static int close_one_pack(void)
>>  		find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);
>>  	}
>>
>> -	if (lru_p) {
>> -		close(lru_p->pack_fd);
>> -		pack_open_fds--;
>> -		lru_p->pack_fd = -1;
>> -		return 1;
>> -	}
>> +	if (lru_p)
>> +		return close_pack_fd(lru_p);
> 
> ... what is returned from here?

Yes. At this point, `lru_p` can only be non-NULL if lru_p->pack_fd is not larger than 0 (hence the call to `close(lru_p->pack_fd)` does not fail all the time, and hence the `pack_open_fds--` does not result in inconsistent state).

> It seems to me that it would be a bug if (p->pack_fd < 0) in this
> codepath, so in practice nobody will receive a newly invented return
> value 0 from this function, but it feels wrong.

Yes, it would be a bug. And more subtle things would go wrong if that was the case, too.

> 
>>  	return 0;
>>  }
>> @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)
>>  		if (strcmp(pack_name, p->pack_name) == 0) {
>>  			clear_delta_base_cache();
>>  			close_pack_windows(p);
>> -			if (p->pack_fd != -1) {
>> -				close(p->pack_fd);
>> -				pack_open_fds--;
>> -			}
>> +			close_pack_fd(p);
> 
> And here, the closer _must_ be (and it currently is) very aware that
> the pack chosen may not be open anymore.  By giving a function that
> conditionally closes if the pack is still open too bland a name,
> that distinction is lost at these two call sites.

Please note that the `close_pack_fd(p)` call does not invalidate the data. It is the caller (`free_pack_by_name()`) that does. Which is safe.

> Also closing "fd" is not the only thing the new helper does, so in
> that sense its name is suboptimal, too.

Yes, it also decrements the counter. But that is a necessary consequence of closing the file descriptor, otherwise the state would be inconsistent.

> Perhaps a new helper function that is close_pack(), which is
> unconditional, with another close_pack_if_open() around it?

Next patch. And that `close_pack()` actually does do more than closing the file descriptor.

>>  			if (!win->offset && win->len == p->pack_size
>> -				&& !p->do_not_close) {
>> -				close(p->pack_fd);
>> -				pack_open_fds--;
>> -				p->pack_fd = -1;
>> -			}
>> +				&& !p->do_not_close)
>> +				close_pack_fd(p);
> 
> I wonder how this do_not_close bit should interact with "we are
> shutting down (or we are spawning another to do the real work, while
> we won't do anything useful anymore), so close everything down".

The `close_all_packs()` function is meant for the use case where you really close all the packs, no question asked.

I cannot think of a use case where it would make sense to try to be gentle, yet still ask for *all* of the packs to be closed. Maybe you can think of one to convince me that it might make sense to respect the `do_not_close` flag in such a function? Because so far, I am totally unconvinced.

Ciao,
Johannes