ehlo,
Attached patches fix ticket #1759 sss_cache -N/-n should invalidate the hash table in sssd_nss
LS
On 07/30/2013 08:19 AM, Lukas Slebodnik wrote:
ehlo,
Attached patches fix ticket #1759 sss_cache -N/-n should invalidate the hash table in sssd_nss
LS
Nack.
The hash table is cleared successfully, however the netgroup is not removed from the sysdb when it is not found in ldap. Therefore the following set of commands still returns the netgroup, although it should not:
$ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com) ...delete ng-1 from ldap $ sudo sss_cache -N $ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com)
struct sbus_method monitor_nss_methods[] = { { MON_CLI_METHOD_PING, monitor_common_pong }, { MON_CLI_METHOD_RES_INIT, monitor_common_res_init }, { MON_CLI_METHOD_ROTATE, responder_logrotate }, { MON_CLI_METHOD_CLEAR_MEMCACHE, nss_clear_memcache},
- { MON_CLI_METHOD_CLEAR_ENUM_CACHE, nss_clear_netgroup_hash_table}, { NULL, NULL }
};
Please, rename the ENUM_CACHE to what the command actually does (clears the netgroup table).
Also put somewhere a debug message that netgroup in-memory cache is beeing cleared, like it is done with the memcache: [nss_clear_memcache] (0x0400): Clearing memory caches.
+errno_t nss_orphan_netgroups(struct nss_ctx *nctx) {
Break the line before the brace.
- if (!nctx || !nctx->netgroups) {
return EINVAL;- }
You can return EOK if nctx->netgroups == NULL, since the table doesn't exist, it is successfully cleared.
Since hash_delete() does not free the pointer itself, is the netgroup entry still deleted via setnetgrent_result_timeout()? If so, please comment it in the code.
On (31/07/13 12:59), Pavel Březina wrote:
On 07/30/2013 08:19 AM, Lukas Slebodnik wrote:
ehlo,
Attached patches fix ticket #1759 sss_cache -N/-n should invalidate the hash table in sssd_nss
LS
Nack.
The hash table is cleared successfully, however the netgroup is not removed from the sysdb when it is not found in ldap. Therefore the following set of commands still returns the netgroup, although it should not:
$ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com) ...delete ng-1 from ldap $ sudo sss_cache -N $ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com)
It works with IPA, but I spent a lot of time with debugging this problem and it is unrelated to my patches. Change(bug?) was introduced in commit ca344fde. First patch should fix this
struct sbus_method monitor_nss_methods[] = { { MON_CLI_METHOD_PING, monitor_common_pong }, { MON_CLI_METHOD_RES_INIT, monitor_common_res_init }, { MON_CLI_METHOD_ROTATE, responder_logrotate }, { MON_CLI_METHOD_CLEAR_MEMCACHE, nss_clear_memcache},
- { MON_CLI_METHOD_CLEAR_ENUM_CACHE, nss_clear_netgroup_hash_table}, { NULL, NULL }
};
Please, rename the ENUM_CACHE to what the command actually does (clears the netgroup table).
The same constant MON_CLI_METHOD_CLEAR_ENUM_CACHE is used to remove autofs hash table. I did not change it. If you think that it should be changed, I will send another patch.
Also put somewhere a debug message that netgroup in-memory cache is beeing cleared, like it is done with the memcache: [nss_clear_memcache] (0x0400): Clearing memory caches.
Message was added to function nss_orphan_netgroups
+errno_t nss_orphan_netgroups(struct nss_ctx *nctx) {
Break the line before the brace.
Fixed
- if (!nctx || !nctx->netgroups) {
return EINVAL;- }
You can return EOK if nctx->netgroups == NULL, since the table doesn't exist, it is successfully cleared.
I will not change return value, because sssd_nss relies on non null hash table (nctx->netgroups), but I can remove "!nctx->netgroups" from condition.
Since hash_delete() does not free the pointer itself, is the netgroup entry still deleted via setnetgrent_result_timeout()? If so, please comment it in the code.
It was mentioned in commit header, but I added small comment to function nss_orphan_netgroups.
Thank you for review. Updated patches are attached.
LS
On Fri, Aug 02, 2013 at 06:44:51PM +0200, Lukas Slebodnik wrote:
On (31/07/13 12:59), Pavel Březina wrote:
On 07/30/2013 08:19 AM, Lukas Slebodnik wrote:
ehlo,
Attached patches fix ticket #1759 sss_cache -N/-n should invalidate the hash table in sssd_nss
LS
Nack.
The hash table is cleared successfully, however the netgroup is not removed from the sysdb when it is not found in ldap. Therefore the following set of commands still returns the netgroup, although it should not:
$ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com) ...delete ng-1 from ldap $ sudo sss_cache -N $ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com)
It works with IPA, but I spent a lot of time with debugging this problem and it is unrelated to my patches. Change(bug?) was introduced in commit ca344fde. First patch should fix this
Wow, nice catch. But I would expect a compiler warning or Coverity report here..this is a clear refactoring bug..
Ack to the new patch. This needs to be pushed to Fedora right now.
On (06/08/13 13:52), Jakub Hrozek wrote:
On Fri, Aug 02, 2013 at 06:44:51PM +0200, Lukas Slebodnik wrote:
On (31/07/13 12:59), Pavel Březina wrote:
On 07/30/2013 08:19 AM, Lukas Slebodnik wrote:
ehlo,
Attached patches fix ticket #1759 sss_cache -N/-n should invalidate the hash table in sssd_nss
LS
Nack.
The hash table is cleared successfully, however the netgroup is not removed from the sysdb when it is not found in ldap. Therefore the following set of commands still returns the netgroup, although it should not:
$ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com) ...delete ng-1 from ldap $ sudo sss_cache -N $ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com)
It works with IPA, but I spent a lot of time with debugging this problem and it is unrelated to my patches. Change(bug?) was introduced in commit ca344fde. First patch should fix this
Wow, nice catch. But I would expect a compiler warning or Coverity report here..this is a clear refactoring bug..
Ack to the new patch. This needs to be pushed to Fedora right now.
I don't know why, but you can use NULL as a boolean argument. There will be no warning even with -Wextra. I check simple example with gcc, clang and clang static analyser. It was a big surprise for me.
LS
On 08/02/2013 06:44 PM, Lukas Slebodnik wrote:
On (31/07/13 12:59), Pavel Březina wrote:
On 07/30/2013 08:19 AM, Lukas Slebodnik wrote:
ehlo,
Attached patches fix ticket #1759 sss_cache -N/-n should invalidate the hash table in sssd_nss
LS
Nack.
The hash table is cleared successfully, however the netgroup is not removed from the sysdb when it is not found in ldap. Therefore the following set of commands still returns the netgroup, although it should not:
$ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com) ...delete ng-1 from ldap $ sudo sss_cache -N $ getent netgroup ng-1 ng-1 ( ,,bobby,example.com) ( ,,johny.example.com)
It works with IPA, but I spent a lot of time with debugging this problem and it is unrelated to my patches. Change(bug?) was introduced in commit ca344fde. First patch should fix this
Good job finding that.
struct sbus_method monitor_nss_methods[] = { { MON_CLI_METHOD_PING, monitor_common_pong }, { MON_CLI_METHOD_RES_INIT, monitor_common_res_init }, { MON_CLI_METHOD_ROTATE, responder_logrotate }, { MON_CLI_METHOD_CLEAR_MEMCACHE, nss_clear_memcache},
- { MON_CLI_METHOD_CLEAR_ENUM_CACHE, nss_clear_netgroup_hash_table}, { NULL, NULL }
};
Please, rename the ENUM_CACHE to what the command actually does (clears the netgroup table).
The same constant MON_CLI_METHOD_CLEAR_ENUM_CACHE is used to remove autofs hash table. I did not change it. If you think that it should be changed, I will send another patch.
Yeah, but in autofs you have only autofs stuff. In nss we work also with users, groups and services. However, looking at the code again, I'm fine with keeping the name.
Also put somewhere a debug message that netgroup in-memory cache is beeing cleared, like it is done with the memcache: [nss_clear_memcache] (0x0400): Clearing memory caches.
Message was added to function nss_orphan_netgroups
+errno_t nss_orphan_netgroups(struct nss_ctx *nctx) {
Break the line before the brace.
Fixed
- if (!nctx || !nctx->netgroups) {
return EINVAL;- }
You can return EOK if nctx->netgroups == NULL, since the table doesn't exist, it is successfully cleared.
I will not change return value, because sssd_nss relies on non null hash table (nctx->netgroups), but I can remove "!nctx->netgroups" from condition.
Since hash_delete() does not free the pointer itself, is the netgroup entry still deleted via setnetgrent_result_timeout()? If so, please comment it in the code.
It was mentioned in commit header, but I added small comment to function nss_orphan_netgroups.
You don't read commit message as often as the code :-)
Ack to all patches,
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina