Re: RLIMIT_NOFILE fallback

["Torsten Bögershausen" <tboegi@web.de>]

On 2013-12-20 10.12, Jeff King wrote:
> On Thu, Dec 19, 2013 at 09:39:55AM -0800, Junio C Hamano wrote:
> 
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>> Thanks for an interesting reading,
>>> please allow a side question:
>>> Could it be, that "-1 == unlimited" is Linux specific?
>>> And therefore not 100% portable ?
>>>
>>> And doesn't "unlimited" number of files call for trouble,
>>> having the risk to starve the machine ?
>>>
>>> BTW: cygwin returns 256.
>>
>> If you look at the caller, you will see that we do cap the value
>> returned from this helper function down to a more reasonable and not
>> so selfish maximum, exactly for the purpose of avoiding the risk of
>> starving other processes.
> 
> I am not sure you are reading the capping in the right direction. We do
> not cap at 25, but rather keep 25 open for "other stuff". So at
> unlimited, we are consuming a mere UINT_MAX-25 descriptors. :)
> 
> I think that 25 is not for the benefit of the rest of the system, but
> rather for _us_ to avoid running out of descriptors for normal
> operations. I do not think we need to be careful about starving other
> processes at all. That is the job of the ulimit in the first place, and
> we respect it. If the sysadmin turns off the limit, then we are just
> following their instructions.
> 
> In practice, I'd be shocked if git behaved reasonably above about 500
> packs anyway, so that puts a practical cap on our fd use. :)
> 
> None of that impacts the patch under discussion, though. The only thing
> I was trying to bring up earlier is that on a system with:
> 
>   1. No (or broken) getrlimit
> 
>   2. No OPEN_MAX defined
> 
>   3. sysconf that works, and returns -1 for unlimited
> 
>   4. a sysadmin who has set the descriptor limit to "unlimited"
> 
> We will end up at "1". Which is not great, but I am skeptical that a
> system matching the above 4 constraints actually exists. So I think the
> patch is fine in practice.
> 
> -Peff

My wrong: I was carefully reading the wrong version of the patch :-(
Sorry for the noise.
/torsten