URL: https://github.com/SSSD/sssd/pull/5541 Author: sumit-bose Title: #5541: nss client: make innetgr() thread safe Action: opened
PR body: """ The innetgr() call is expected to be thread safe but SSSD's the current implementation isn't. In glibc innetgr() is implementend by calling the setnetgrent(), getnetgrent(), endgrent() sequence with a private context (struct __netgrent) with provides a member where NSS modules can store data between the calls.
With this patch setnetgrent() will read all required data from the NSS responder and store it in the data member of the __netgrent struct. Upcoming getnetgrent() calls will only operate on the stored data and not connect to the NSS responder anymore. endgrent() will free the data. Since the netgroup data is read in a single request to the NSS responder protected by a mutex and stored in private context of innetgr() this call is now thread-safe.
Resolves: https://github.com/SSSD/sssd/issues/5540 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5541/head:pr5541 git checkout pr5541
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
sumit-bose commented: """ Hi,
I have send two different implementations, the second is #5542.
Each has its pros and cons. This one requires more memory since all data is read in a single run and kept in the client process between the calls. This is more or less the same as libnss_files.so does, the related data from /etc/netgroup is read by setnetgrent() and processed later.
The second one uses a new connection for each request sequences. Since the socket has to be opened and the connection established this will require a bit more time. My main concern here is that the file descriptor might get lost and the connection stays open if the caller does not implement the setnetgrent(), getentgrent(), endgrent() sequence correctly or calls them in threads because the sequence itself is not thread-safe, only the implementation with innetgr().
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-801735204
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
alexey-tikhonov commented: """ In general, I prefer this solution as more clear and simple. See https://github.com/SSSD/sssd/pull/5542#issuecomment-809590594 """
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-809617868
URL: https://github.com/SSSD/sssd/pull/5541 Author: sumit-bose Title: #5541: nss client: make innetgr() thread safe Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5541/head:pr5541 git checkout pr5541
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
sumit-bose commented: """ Hi,
thank you for the review. I set `enum_limit` to `0` and added a comment which explains that it is not used for this specific call.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-821319735
URL: https://github.com/SSSD/sssd/pull/5541 Author: sumit-bose Title: #5541: nss client: make innetgr() thread safe Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5541/head:pr5541 git checkout pr5541
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
alexey-tikhonov commented: """ Sorry I missed test code last time (I have a couple of minor comments in its regard.) Main patch looks good to me now (but Pavel self-assigned so probably wants to take a look as well). """
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-822518951
URL: https://github.com/SSSD/sssd/pull/5541 Author: sumit-bose Title: #5541: nss client: make innetgr() thread safe Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5541/head:pr5541 git checkout pr5541
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
sumit-bose commented: """ Hi,
the latest version fixes the comments for the test program as well and adds a GPLv3 header to the file.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823136621
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
alexey-tikhonov commented: """ Hi, a couple of other remarks. """
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823243602
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/5541 Author: sumit-bose Title: #5541: nss client: make innetgr() thread safe Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5541/head:pr5541 git checkout pr5541
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
alexey-tikhonov commented: """ Thank you. ACK from my side.
@pbrezina, what is your opinion (in general and on "this PR vs #5542")? """
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823377946
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
pbrezina commented: """ I'm fine with this. Ack. """
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823925310
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
pbrezina commented: """ Pushed PR: https://github.com/SSSD/sssd/pull/5541
* `master` * 29abf94e3aec2c54c76adf61318099097c41ea77 - intg test: test is innetgr() is thread-safe * 88eec1c22f188adc49f41b81fe5af03c995c690d - nss client: make innetgr() thread safe
"""
See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823940736
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/5541 Author: sumit-bose Title: #5541: nss client: make innetgr() thread safe Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5541/head:pr5541 git checkout pr5541
URL: https://github.com/SSSD/sssd/pull/5541 Title: #5541: nss client: make innetgr() thread safe
Label: +Pushed
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
pbrezina
-
sumit-bose