Bug 498096

Summary: 2 max file patches for xinetd
Product: [Fedora] Fedora Reporter: James M. Leddy <james.leddy>
Component: xinetdAssignee: Jan Zeleny <jzeleny>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: 10CC: gborsuk, jzeleny, sgrubb, vanhoof
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 527311 (view as bug list) Environment:
Last Closed: 2009-10-20 13:58:52 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 527311, 639447    
Attachments:
Description Flags
patch allowing fd limits in xinetd.conf
none
better accounting mechanism for select()
none
Vince's patch w/ doc
none
Patch replacing select() with poll()
none
Completed poll patch (cleaned) none

Description James M. Leddy 2009-04-28 20:42:52 UTC
Created attachment 341649 [details]
patch allowing fd limits in xinetd.conf

Description of problem:

Two patches to address the issue of xinetd limiting fd's to FD_SETSIZE.

The first (xinetd-2.3.14-add-files-limit-vw4-2.patch) allows one to set a limit on the max number of files for a child service, in the same way that we already do with rlimit_as and friends in xinetd.conf.

The second (xinetd-2.3.14-fdsetsize.patch) fixes a known issue with xinetd imposing a limit of FD_SETSIZE on itself to get around needing to account for file descriptors in select().  This patch puts a good accounting mechanism in, so that the setrlimit() is no longer required.

Both are against the upstream 2.3.14 release, though they should apply cleanly to the fedora version.

I'll be notifying the upstream maintainer, since last time he accepted a patch it was keyed off a fedora bugzilla.

Comment 1 James M. Leddy 2009-04-28 21:06:26 UTC
Created attachment 341653 [details]
better accounting mechanism for select()

Comment 2 James M. Leddy 2009-04-28 21:10:10 UTC
These patches don't apply in consecutively, because they both remove 

- #ifdef RLIMIT_NOFILE
-    rl.rlim_max = ps.ros.orig_max_descriptors ;
-    rl.rlim_cur = ps.ros.max_descriptors ;
-    (void) setrlimit( RLIMIT_NOFILE, &rl ) ;
- #endif

from child.c.rej

Aside from that it compiles cleanly.

Comment 3 Jan Zeleny 2009-05-11 13:48:03 UTC
I reviewed the patches. There is one thing I don't fully understand - when I take patch adding new config options into consideration, is the other one (better accounting mechanism for select function) really necessary?

Comment 4 James M. Leddy 2009-05-14 20:05:38 UTC
The other patch taken on it's own fixes a longstanding hack.  The process should not need to set file descriptor limits just because it doesn't check FD_SETSIZE.  It should be left to the discretion of the users and sysadmins what resources a process is given.

That said, I can change the patch to correctly apply the old limits after fork().  That's the way they were hacking around that, but it turns out there is a bug in that code as well.

upstream still has yet to comment on the patches.

Comment 11 James M. Leddy 2009-05-22 16:24:44 UTC
Created attachment 345118 [details]
Vince's patch w/ doc

I forgot to put the patch up that included the documentation.

diff -pru xinetd-2.3.14/xinetd/xinetd.conf.man xinetd-2.3.14-add_fd_limit/xinetd/xinetd.conf.man
--- xinetd-2.3.14/xinetd/xinetd.conf.man        2005-10-05 13:15:33.000000000 -0400
+++ xinetd-2.3.14-add_fd_limit/xinetd/xinetd.conf.man   2009-04-28 17:32:34.000000000 -0400
@@ -564,6 +564,11 @@ is implemented, it is more useful to set
 rlimit_rss and rlimit_stack. This resource limit is only implemented on
 Linux systems.
 .TP
+.B rlimit_files
+Sets the maximum number of open files that the service may use.
+One parameter is required, which is either a positive integer representing
+the number of open file descriptors, or "UNLIMITED".
+.TP
 .B rlimit_cpu
 Sets the maximum number of CPU seconds that the service may use.
 One parameter is required, which is either a positive integer representing

Comment 16 Jan Zeleny 2009-06-18 14:26:07 UTC
Created attachment 348478 [details]
Patch replacing select() with poll()

I've created patch which replaces fdsetsize patch. This one uses poll() instead of select(). Patch was not tested yet, it's really very first version. Feel free to test it and post here the results. Any comments and suggestions are welcomed as well.

Comment 30 Jan Zeleny 2009-09-14 12:54:35 UTC
Created attachment 360936 [details]
Completed poll patch (cleaned)

Here is the updated final version of the patch. It has been reviewed and significantly cleaned up.

Comment 36 Jan Zeleny 2009-10-20 13:58:52 UTC
The second patch is in cvs for F-13 as well as an update candidate for F-12, closing this bug.