autofs: distinguish between empty cache and offline state #5343
Conversation
This is, of course, out of SSSD scope, but what happens - currently and planned - if: In some sense this is a change of API between autofs and plugin. Is this change backward compatible with autofs that doesn't implement proper handling of this error code yet? |
|
Even though autofs uses nsswitch.conf for database order, it does not use its logic (the autofs api is not part of glibc). But the code suggest that it's going to fail, so I asked Ian once again in the bugzilla for reassurance. |
pbrezina
force-pushed
the
autofs
branch
2 times, most recently
from
412c622
to
acf1a0a
Compare
|
I added _sss_auto_protocol_version to solve the circular dependency between autofs and SSSD, this is yet to be checked with Ian. |
|
Ian approved these changes. Please continue with the review. Autofs test client got a new |
src/sss_client/autofs/sss_autofs.c
Outdated
| @@ -33,6 +33,32 @@ | |||
| /* How many entries shall _sss_getautomntent_r retrieve at once */ | |||
| #define GETAUTOMNTENT_MAX_ENTRIES 512 | |||
|
|
|||
| unsigned int _protocol = 0; | |||
There was a problem hiding this comment.
Hi,
I think _protocol should be:
staticat the very least.- strictly speaking access to this var should be thread safe.
I would prefer C11atomic_intbutsss_clientin general useslibpthread.
There was a problem hiding this comment.
atomics are better in that they do not incur the heavy weight of acquiring a mutx (which is done via atomics anyway). So I see no problem in using that.
There was a problem hiding this comment.
And how about thread_local, do you see any problem there?
There was a problem hiding this comment.
Imo, this won't hurt but I'm not sure if it makes much sense.
_Thread_local here would be to cover case "application wants to use sssd autofs lib with different protocol version from different threads".
Frankly I can't imagine sane reason to do so. But even if there are, and we want to support this, then we should document this somehow/somewhere. Currently looking at the API itself one can't judge if there is thread specific context.
There was a problem hiding this comment.
There is no functional reason - autofs will always try to used its latest protocol version, atomic should be enough to do that.
|
Thank you for the updates. I have some concerns wrt global var. Please see inline. |
|
btw, "pbrezina added 5 commits on Mar 6" - date looks weird... |
|
:-) It's fine, I had the first patch set ready in March but it was blocked for a long time by Ian's work. BTW RHEL7 and CentOS apparently does not have stdatomic.h (gcc 4.8 bug resolved in 4.9) so if we choose to use this we need to stop running prci against rhel7/centos7. I'm fine with that. |
Perhaps we could add ./configure check for this header to error out gracefully. |
… and cache is empty Resolves: SSSD#3413
So we do not publish internal error code. Resolves: SSSD#3413
If the backend is offline when autofs starts and reads auto.master map we don't want to wait 60 seconds before the offline flag is reset. We need to allow autofs to retry the call much sooner. Resolves: SSSD#3413
|
Added. |
It checks: but doesn't error out. I would expect ./configure to stop here saying project won't build on this system? |
Recent autofs patches adds dependency on automic_uint/_Atomic type from C11 standard. This is supported in both gcc and clang for a long time now.
|
Thank you, ACK. |
|
Pushed PR: #5343
|
If the cache is empty and SSSD is offline we now return a proper
error code to autofs so it can iplement a proper retry logic to
fetch the maps when SSSD comes back offline.
This also requires changes in autofs which are not yet available
so the best way to test it with SSSD is to use the autofs test
tool (autofs_test_client) to see that it returns correct error
codes.
Resolves:
#3413