-
Notifications
You must be signed in to change notification settings - Fork 273
RESPONDER: fix responder_set_fd_limit()
#8371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors responder_set_fd_limit() to avoid attempting to set the hard limit for file descriptors, as SSSD responders typically lack the necessary CAP_SYS_RESOURCE capability. The change correctly removes the initial setrlimit() call that was likely to fail or behave incorrectly, and instead directly queries the current hard limit to safely adjust the soft limit. The corresponding documentation in sssd.conf.5.xml has been updated to reflect this behavior, and a helpful debug message has been added for cases where the requested limit exceeds the hard limit. The changes are correct, improve efficiency by removing an unnecessary system call, and make the code's intent clearer. I have no further suggestions.
|
Note: Covscan is green. |
src/man/sssd.conf.5.xml
Outdated
| capability, the resulting value will be the lower | ||
| value of this or the limits.conf "hard" limit. | ||
| SSSD process. Note this value will be capped by | ||
| limits.conf "hard" limit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I would suggest to rephrase this a bit because limits.conf is the configuration file for pam_limits.so and even if we would add a configuration for the sssd user it would only apply if we would switch to the sssd user via a new PAM session. Additionally systemd ignores those settings.
What about "Note this value will be capped by the limits set by the init system at the startup of SSSD, see e.g. man systemd.exec for details."
bye,
Sumit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
e83a49b to
bacc47d
Compare
sumit-bose
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
thank you for the update, ACK.
bye,
Sumit
to not even try setting hard limit as SSSD never has CAP_SYS_RESOURCE Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
to not even try setting hard limit as SSSD never has CAP_SYS_RESOURCE