Fix major problems introduced by commit 1162165

[yadij]

... when Squid is built with select() or poll() I/O loops, and
on systems with limited resources.

[rousskov]

PR description: Fix major problems introduced by commit 1162165

What major problems does this PR fix?

[kinkie]

we can use the type system.
The change has been build tested

[yadij]

PR description: Fix major problems introduced by commit 1162165

What major problems does this PR fix?

This PR is implementing:

squid/src/tools.cc

Line 823 in dd5ea20

// XXX: When checkLimits, take ./configure --with-filedescriptors (if any) into account.

and fixing the issues documented alongside that:

1. squid/src/tools.cc

   Line 826 in dd5ea20

   // XXX: If this increases Squid_MaxFD, then the new value will violate any defined SQUID_MAXFD_LIMIT.

Squid select(2) is often enabled by ./configure as a backup I/O mechanism. As a result the Squid_MaxFD is by default almost always initialized to FD_SETSIZE - which is a relatively small value suitable for select(2) or poll(2) API structures.

2. squid/src/tools.cc

   Line 825 in dd5ea20

   // XXX: The new value may make Squid_MaxFD different from SQUID_MAXFD still used by ModEpoll, ModPoll, ipcCreate(), etc.!

This is actually guaranteed to occur for at least a period during Squid startup (from

squid/src/main.cc

Lines 1433 to 1434 in dd5ea20

	if (SQUID_MAXFD_LIMIT < Squid_MaxFD)
	Squid_MaxFD = SQUID_MAXFD_LIMIT;

until

squid/src/main.cc

Lines 1337 to 1339 in dd5ea20

	StartUsingConfig()
	{
	setMaxFD();

). Due to FD_SETSIZE being something smaller than the arbitrary 100*1024 constant.

[rousskov]

PR description: Fix major problems introduced by commit 1162165

What major problems does this PR fix?

This PR is implementing:

squid/src/tools.cc

Line 823 in dd5ea20

// XXX: When checkLimits, take ./configure --with-filedescriptors (if any) into account.

and fixing the issues documented alongside that:

1. squid/src/tools.cc

   Line 826 in dd5ea20

   // XXX: If this increases Squid_MaxFD, then the new value will violate any defined SQUID_MAXFD_LIMIT.

2. squid/src/tools.cc

   Line 825 in dd5ea20

   // XXX: The new value may make Squid_MaxFD different from SQUID_MAXFD still used by ModEpoll, ModPoll, ipcCreate(), etc.!

AFAICT, all of the above references point to problems that existed before commit 1162165. In other words, those problems were not "introduced by commit 1162165" as claimed in the PR title. The corresponding XXXs just documented reality that existed before that commit and continued to exist after that commit.

The discussion-relevant changes introduced by commit 1162165 can be summarized by this diff (which replaces rl.rlim_cur variable with its billion descriptors value in relevant Kubernets environments):

- Squid_MaxFD = 1073741816;
+ Squid_MaxFD =     102400;

Both values may be wrong in some cases (hence, XXXs), but the new/smaller value is certainly not worse than the old one in those problematic cases.

Even if you disagree with the above analysis, the current PR title/description does not disclose what "major problems" this PR is fixing. None of those XXXs can be easily identified as "major problems" IMO, and it is not clear that the current PR title is actually referring to those XXXs in the first place!

This PR direction was explicitly rejected during commit 1162165 (i.e. PR #2483) work as detailed in unofficial commit ce8c540 message. That fact does not imply that this direction is necessarily wrong -- different PRs have different scopes and evil comparison functions -- but I believe the original reasons for rejecting this direction are valid, and nothing in this PR convinces me that a different decision should be made now, in this PR.

I am pretty sure that, after the backlog issue is dealt with, the correct path forward starts with agreeing on what max_filedescriptors directive should represent (including the default behavior) and then adjusting code to match that agreement. I speculate that, on that path, we may "remove SQUID_MAXFD and its guessing ./configure code that leads to many inconsistencies and complications" (as was suggested in ce8c540).

[yadij]

PR description: Fix major problems introduced by commit 1162165

What major problems does this PR fix?

This PR is implementing:

squid/src/tools.cc

Line 823 in dd5ea20

// XXX: When checkLimits, take ./configure --with-filedescriptors (if any) into account.

and fixing the issues documented alongside that:

1. squid/src/tools.cc

   Line 826 in dd5ea20

   // XXX: If this increases Squid_MaxFD, then the new value will violate any defined SQUID_MAXFD_LIMIT.

2. squid/src/tools.cc

   Line 825 in dd5ea20

   // XXX: The new value may make Squid_MaxFD different from SQUID_MAXFD still used by ModEpoll, ModPoll, ipcCreate(), etc.!

AFAICT, all of the above references point to problems that existed before commit 1162165.

Before that commit, the limit was either SQUID_MAXFD_LIMIT or undefined/infinity. Which then gets reduced to the admin setting (if any). Worse case it stayed the same (infinity).

After the commit, the limit gets increased to 100*1024 when SQUID_MAXFD_LIMIT is small and no admin limit. That is new.

The epoll etc using SQUID_MAXFD instead of SQUID_MAXFD_LIMIT BUT based off the same ./configure details. So no problem occured in reality.

After the commit, Squid_MaxFD potentially increases beyond SQUID_MAXFD is a new major problem with memory allocations smaller than Squid_MaxFD value being accessed up to Squid_MaxFD offset.

IFF the new behaviour was only reducing Squid_MaxFD values you would be right. Sadly it allows increases.

I am pretty sure that, after the backlog issue is dealt with,

That is an idealists view. Even the official GNU "Hello World" C source code has a history of changes, and currently has an open issue to be resolved.

This PR is fixing some issues while we wait for that infinitely far away Milestone to occur.

the correct path forward starts with agreeing on what max_filedescriptors directive should ...

Nice idea, but that is architectural design discussion. This PR scope is just fixing the existing design/behaviour to not be actively buggy before the code settles into a v7 release.