https://fedorahosted.org/sssd/ticket/2346
Hi, the first patch add support for subdomains, which was missing in current implementation. It probably wasn't a big deal with netgroups but I think it would be with users and groups.
The majority of these patches refactors the current be_refresh to make support of other object types than netgroups a lot easier and straightforward.
On Thu, Feb 19, 2015 at 12:18:13PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2346
Hi, the first patch add support for subdomains, which was missing in current implementation. It probably wasn't a big deal with netgroups but I think it would be with users and groups.
The majority of these patches refactors the current be_refresh to make support of other object types than netgroups a lot easier and straightforward.
Code review so far, no testing was done. I'm just sending my notes so you can reply while I test the patches.
From e744e3dbeab301c149accbff521cbc5232c6a48d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:16:04 +0100 Subject: [PATCH 1/6] be_refresh: refresh all domains in backend
src/providers/dp_refresh.c | 82 ++++++++++++++++++++++++---------------
Can you also write a test for this module while we're changing it?
src/providers/dp_refresh.h | 1 + src/providers/ldap/ldap_common.h | 1 + src/providers/ldap/sdap_refresh.c | 15 +++++-- 4 files changed, 64 insertions(+), 35 deletions(-)
diff --git a/src/providers/dp_refresh.c b/src/providers/dp_refresh.c index 817b6213ca47bba3fa34ce28fdcd1621d349b651..bd02d0cd99f9a061109f0c17797c6e018d602dc5 100644 --- a/src/providers/dp_refresh.c +++ b/src/providers/dp_refresh.c @@ -117,6 +117,7 @@ typedef errno_t
struct be_refresh_cb {
- const char *name; bool enabled; be_refresh_get_values_t get_values; be_refresh_send_t send_fn;
@@ -137,6 +138,7 @@ struct be_refresh_ctx *be_refresh_ctx_init(TALLOC_CTX *mem_ctx) return NULL; }
- ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].name = "netgroups"; ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].get_values \ = be_refresh_get_netgroups;
In retrospective I wish we didn't use a pointer function here. A pointer function might make sense in cases where the function might be from a different provider for instance, but the sysdb expiration will be always read in the same manner. A switch-case here would be much simpler and allow for easier code browsing.
I also wonder if be_refresh_send needs the be_ctx parameter at all, since be_ctx is also part of the be_ptask structure?
I know the two remarks above are not strictly related to your patches, but I had to re-read the be_refresh code to make sure I understand it :-)
The rest of the patch looks good to me.
From 85b2f150134e89672f764e7b267f7f47a285938f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:49:17 +0100 Subject: [PATCH 2/6] sdap_handle_acct_req_send: remove be_req
be_req was used only as a talloc context for subreq. This memory context was replace by state of the parent request which is more suitable for tevent coding style.
ACK. I traced the parent contexts all the way up and confirmed that in fact nothing changes, be_req is always used as the top-level state.
From a92f1ffac23e90cfff848a05ef9611fa021b78a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 16 Feb 2015 13:42:02 +0100 Subject: [PATCH 3/6] be_refresh: refactor netgroups refresh
[...]
- state->account_req = talloc_zero(state, struct be_acct_req);
- if (state->account_req == NULL) {
ret = ENOMEM;goto immediately;- }
- state->account_req->entry_type = entry_type;
- state->account_req->attr_type = BE_ATTR_ALL;
I know that this attribute is currently not used really, but can you use BE_ATTR_CORE here instead? IIRC all the other invokers use _CORE as well, so using it here as well might make any future refactoring easier.
From 4cff90263b247d8701de33ad1cf76db95698659a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:04:09 +0100 Subject: [PATCH 4/6] be_refresh: add sdap_refresh_init
[...]
@@ -179,14 +179,11 @@ static int ldap_id_init_internal(struct be_ctx *bectx, }
/* setup periodical refresh of expired records */
- ret = be_refresh_add_cb(bectx->refresh_ctx, BE_REFRESH_TYPE_NETGROUPS,
sdap_refresh_netgroups_send,sdap_refresh_netgroups_recv,ctx);- if (ret != EOK && ret != EEXIST) {
DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh of netgroups ""will not work [%d]: %s\n", ret, strerror(ret));- }
- ret = sdap_refresh_init(bectx->refresh_ctx, ctx);
if (ret != EOK && ret != EEXIST) {DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh ""will not work [%d]: %s\n", ret, strerror(ret));}
Wrong indent, otherwise patch LGTM.
From 42efd926f41c4691758e5871fd3a3330802e7f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:00:52 +0100 Subject: [PATCH 5/6] be_refresh: support users
Resolves: https://fedorahosted.org/sssd/ticket/2346
src/db/sysdb.c | 7 +++++++ src/db/sysdb.h | 2 ++ src/providers/dp_refresh.c | 23 +++++++++++++++++++++++ src/providers/dp_refresh.h | 1 + src/providers/ldap/sdap_refresh.c | 29 +++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 61a2240016b5cb77e6fbbc3286fd1a194c5a0b48..2bb4a41aa4a9e6201ac27ac8d9a1803c1fb5c43e 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -172,6 +172,13 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom, return dn; }
+struct ldb_dn *sysdb_user_base_dn(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom)+{
- return ldb_dn_new_fmt(mem_ctx, dom->sysdb->ldb,
SYSDB_TMPL_USER_BASE, dom->name);+}
I don't like adding a convenience function and then only using it on one place. Either use ldb_dn_new_fmt() directly or convert the uses of ldb_dn_new_fmt to this new convenience function.
From 20bcc5371af8e429b46b0289ef9fd517c1e932c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:10:23 +0100 Subject: [PATCH 6/6] be_refresh: support groups
Same comment about the convenience function, otherwise LGTM.
CI passed: http://sssd-ci.duckdns.org/logs/job/8/57/summary.html
Did you test the refresh with really large (hundreds at least, better thousands of entries) directories?
On 03/03/2015 01:03 PM, Jakub Hrozek wrote:
On Thu, Feb 19, 2015 at 12:18:13PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2346
Hi, the first patch add support for subdomains, which was missing in current implementation. It probably wasn't a big deal with netgroups but I think it would be with users and groups.
The majority of these patches refactors the current be_refresh to make support of other object types than netgroups a lot easier and straightforward.
Code review so far, no testing was done. I'm just sending my notes so you can reply while I test the patches.
From e744e3dbeab301c149accbff521cbc5232c6a48d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:16:04 +0100 Subject: [PATCH 1/6] be_refresh: refresh all domains in backend
src/providers/dp_refresh.c | 82 ++++++++++++++++++++++++---------------
Can you also write a test for this module while we're changing it?
Well, there is a ticket for it. But the main problem is how do you want to write a meaningful test?
The be_refresh module consist of:
be_refresh_send (public) - fetch expired objects (internal) - refresh them (internal)
The only thing you can test is that sdap was actually called which isn't really worth the time IMHO. Or do you have other idea?
src/providers/dp_refresh.h | 1 + src/providers/ldap/ldap_common.h | 1 + src/providers/ldap/sdap_refresh.c | 15 +++++-- 4 files changed, 64 insertions(+), 35 deletions(-)
diff --git a/src/providers/dp_refresh.c b/src/providers/dp_refresh.c index 817b6213ca47bba3fa34ce28fdcd1621d349b651..bd02d0cd99f9a061109f0c17797c6e018d602dc5 100644 --- a/src/providers/dp_refresh.c +++ b/src/providers/dp_refresh.c @@ -117,6 +117,7 @@ typedef errno_t
struct be_refresh_cb {
- const char *name; bool enabled; be_refresh_get_values_t get_values; be_refresh_send_t send_fn;
@@ -137,6 +138,7 @@ struct be_refresh_ctx *be_refresh_ctx_init(TALLOC_CTX *mem_ctx) return NULL; }
- ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].name = "netgroups"; ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].get_values \ = be_refresh_get_netgroups;
In retrospective I wish we didn't use a pointer function here. A pointer function might make sense in cases where the function might be from a different provider for instance, but the sysdb expiration will be always read in the same manner. A switch-case here would be much simpler and allow for easier code browsing.
Yes... do you want me to write a patch?
I also wonder if be_refresh_send needs the be_ctx parameter at all, since be_ctx is also part of the be_ptask structure?
It is signature for be_ptask send functions.
I know the two remarks above are not strictly related to your patches, but I had to re-read the be_refresh code to make sure I understand it :-)
The rest of the patch looks good to me.
I will wait until you finish testing, since the issues are below are trivial.
From 85b2f150134e89672f764e7b267f7f47a285938f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:49:17 +0100 Subject: [PATCH 2/6] sdap_handle_acct_req_send: remove be_req
be_req was used only as a talloc context for subreq. This memory context was replace by state of the parent request which is more suitable for tevent coding style.
ACK. I traced the parent contexts all the way up and confirmed that in fact nothing changes, be_req is always used as the top-level state.
From a92f1ffac23e90cfff848a05ef9611fa021b78a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 16 Feb 2015 13:42:02 +0100 Subject: [PATCH 3/6] be_refresh: refactor netgroups refresh
[...]
- state->account_req = talloc_zero(state, struct be_acct_req);
- if (state->account_req == NULL) {
ret = ENOMEM;goto immediately;- }
- state->account_req->entry_type = entry_type;
- state->account_req->attr_type = BE_ATTR_ALL;
I know that this attribute is currently not used really, but can you use BE_ATTR_CORE here instead? IIRC all the other invokers use _CORE as well, so using it here as well might make any future refactoring easier.
From 4cff90263b247d8701de33ad1cf76db95698659a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:04:09 +0100 Subject: [PATCH 4/6] be_refresh: add sdap_refresh_init
[...]
@@ -179,14 +179,11 @@ static int ldap_id_init_internal(struct be_ctx *bectx, }
/* setup periodical refresh of expired records */
- ret = be_refresh_add_cb(bectx->refresh_ctx, BE_REFRESH_TYPE_NETGROUPS,
sdap_refresh_netgroups_send,sdap_refresh_netgroups_recv,ctx);- if (ret != EOK && ret != EEXIST) {
DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh of netgroups ""will not work [%d]: %s\n", ret, strerror(ret));- }
- ret = sdap_refresh_init(bectx->refresh_ctx, ctx);
if (ret != EOK && ret != EEXIST) {DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh ""will not work [%d]: %s\n", ret, strerror(ret));}Wrong indent, otherwise patch LGTM.
From 42efd926f41c4691758e5871fd3a3330802e7f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:00:52 +0100 Subject: [PATCH 5/6] be_refresh: support users
Resolves: https://fedorahosted.org/sssd/ticket/2346
src/db/sysdb.c | 7 +++++++ src/db/sysdb.h | 2 ++ src/providers/dp_refresh.c | 23 +++++++++++++++++++++++ src/providers/dp_refresh.h | 1 + src/providers/ldap/sdap_refresh.c | 29 +++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 61a2240016b5cb77e6fbbc3286fd1a194c5a0b48..2bb4a41aa4a9e6201ac27ac8d9a1803c1fb5c43e 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -172,6 +172,13 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom, return dn; }
+struct ldb_dn *sysdb_user_base_dn(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom)+{
- return ldb_dn_new_fmt(mem_ctx, dom->sysdb->ldb,
SYSDB_TMPL_USER_BASE, dom->name);+}
I don't like adding a convenience function and then only using it on one place. Either use ldb_dn_new_fmt() directly or convert the uses of ldb_dn_new_fmt to this new convenience function.
From 20bcc5371af8e429b46b0289ef9fd517c1e932c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:10:23 +0100 Subject: [PATCH 6/6] be_refresh: support groups
Same comment about the convenience function, otherwise LGTM.
CI passed: http://sssd-ci.duckdns.org/logs/job/8/57/summary.html
Did you test the refresh with really large (hundreds at least, better thousands of entries) directories? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Mar 04, 2015 at 09:57:02AM +0100, Pavel Březina wrote:
On 03/03/2015 01:03 PM, Jakub Hrozek wrote:
On Thu, Feb 19, 2015 at 12:18:13PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2346
Hi, the first patch add support for subdomains, which was missing in current implementation. It probably wasn't a big deal with netgroups but I think it would be with users and groups.
The majority of these patches refactors the current be_refresh to make support of other object types than netgroups a lot easier and straightforward.
Code review so far, no testing was done. I'm just sending my notes so you can reply while I test the patches.
From e744e3dbeab301c149accbff521cbc5232c6a48d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:16:04 +0100 Subject: [PATCH 1/6] be_refresh: refresh all domains in backend
src/providers/dp_refresh.c | 82 ++++++++++++++++++++++++---------------
Can you also write a test for this module while we're changing it?
Well, there is a ticket for it. But the main problem is how do you want to write a meaningful test?
The be_refresh module consist of:
be_refresh_send (public)
- fetch expired objects (internal)
- refresh them (internal)
The only thing you can test is that sdap was actually called which isn't really worth the time IMHO. Or do you have other idea?
I was thinking about: - adding expired entries to cache when the test starts - having a dummy ptask that marks the entries as current
Then the test would run a single refresh and verify that the ptask actually ran and the entries were found by the matching functions.
Again, this would be for master and would verify that the be_refresh_get_* functions works and the refresh works at all.
src/providers/dp_refresh.h | 1 + src/providers/ldap/ldap_common.h | 1 + src/providers/ldap/sdap_refresh.c | 15 +++++-- 4 files changed, 64 insertions(+), 35 deletions(-)
diff --git a/src/providers/dp_refresh.c b/src/providers/dp_refresh.c index 817b6213ca47bba3fa34ce28fdcd1621d349b651..bd02d0cd99f9a061109f0c17797c6e018d602dc5 100644 --- a/src/providers/dp_refresh.c +++ b/src/providers/dp_refresh.c @@ -117,6 +117,7 @@ typedef errno_t
struct be_refresh_cb {
- const char *name; bool enabled; be_refresh_get_values_t get_values; be_refresh_send_t send_fn;
@@ -137,6 +138,7 @@ struct be_refresh_ctx *be_refresh_ctx_init(TALLOC_CTX *mem_ctx) return NULL; }
- ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].name = "netgroups"; ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].get_values \ = be_refresh_get_netgroups;
In retrospective I wish we didn't use a pointer function here. A pointer function might make sense in cases where the function might be from a different provider for instance, but the sysdb expiration will be always read in the same manner. A switch-case here would be much simpler and allow for easier code browsing.
Yes... do you want me to write a patch?
Yes, for master, it will make the code more readable. Let's not delay this patchset any longer.
I also wonder if be_refresh_send needs the be_ctx parameter at all, since be_ctx is also part of the be_ptask structure?
It is signature for be_ptask send functions.
I know the two remarks above are not strictly related to your patches, but I had to re-read the be_refresh code to make sure I understand it :-)
The rest of the patch looks good to me.
I will wait until you finish testing, since the issues are below are trivial.
From 85b2f150134e89672f764e7b267f7f47a285938f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:49:17 +0100 Subject: [PATCH 2/6] sdap_handle_acct_req_send: remove be_req
be_req was used only as a talloc context for subreq. This memory context was replace by state of the parent request which is more suitable for tevent coding style.
ACK. I traced the parent contexts all the way up and confirmed that in fact nothing changes, be_req is always used as the top-level state.
From a92f1ffac23e90cfff848a05ef9611fa021b78a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 16 Feb 2015 13:42:02 +0100 Subject: [PATCH 3/6] be_refresh: refactor netgroups refresh
[...]
- state->account_req = talloc_zero(state, struct be_acct_req);
- if (state->account_req == NULL) {
ret = ENOMEM;goto immediately;- }
- state->account_req->entry_type = entry_type;
- state->account_req->attr_type = BE_ATTR_ALL;
I know that this attribute is currently not used really, but can you use BE_ATTR_CORE here instead? IIRC all the other invokers use _CORE as well, so using it here as well might make any future refactoring easier.
From 4cff90263b247d8701de33ad1cf76db95698659a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:04:09 +0100 Subject: [PATCH 4/6] be_refresh: add sdap_refresh_init
[...]
@@ -179,14 +179,11 @@ static int ldap_id_init_internal(struct be_ctx *bectx, }
/* setup periodical refresh of expired records */
- ret = be_refresh_add_cb(bectx->refresh_ctx, BE_REFRESH_TYPE_NETGROUPS,
sdap_refresh_netgroups_send,sdap_refresh_netgroups_recv,ctx);- if (ret != EOK && ret != EEXIST) {
DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh of netgroups ""will not work [%d]: %s\n", ret, strerror(ret));- }
- ret = sdap_refresh_init(bectx->refresh_ctx, ctx);
if (ret != EOK && ret != EEXIST) {DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh ""will not work [%d]: %s\n", ret, strerror(ret));}Wrong indent, otherwise patch LGTM.
From 42efd926f41c4691758e5871fd3a3330802e7f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:00:52 +0100 Subject: [PATCH 5/6] be_refresh: support users
Resolves: https://fedorahosted.org/sssd/ticket/2346
src/db/sysdb.c | 7 +++++++ src/db/sysdb.h | 2 ++ src/providers/dp_refresh.c | 23 +++++++++++++++++++++++ src/providers/dp_refresh.h | 1 + src/providers/ldap/sdap_refresh.c | 29 +++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 61a2240016b5cb77e6fbbc3286fd1a194c5a0b48..2bb4a41aa4a9e6201ac27ac8d9a1803c1fb5c43e 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -172,6 +172,13 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom, return dn; }
+struct ldb_dn *sysdb_user_base_dn(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom)+{
- return ldb_dn_new_fmt(mem_ctx, dom->sysdb->ldb,
SYSDB_TMPL_USER_BASE, dom->name);+}
I don't like adding a convenience function and then only using it on one place. Either use ldb_dn_new_fmt() directly or convert the uses of ldb_dn_new_fmt to this new convenience function.
How did you decide about the convenience function? I'm fine with keeping it, but then it must be used on other places in the code so that there's one pattern to construct the base DN, not two. That's probably out of scope of this changeset and can be done in master.
From 20bcc5371af8e429b46b0289ef9fd517c1e932c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:10:23 +0100 Subject: [PATCH 6/6] be_refresh: support groups
nack after testing :-)
You need to handle group in the sdap code: @@ -78,6 +78,9 @@ static struct tevent_req *sdap_refresh_send(TALLOC_CTX *mem_ctx, case BE_REQ_USER: state->type = "user"; break; + case BE_REQ_GROUP: + state->type = "group"; + break; case BE_REQ_NETGROUP: state->type = "netgroup"; break;
To sum, up we need to 1) Fix the group refresh 2) Fix the BE_ATTR and indent 3) agree about the sysdb convenience function. I'm fine with keeping it and write a patch for master that uses the function elsewhere as well. The subsequent sysdb patch wouldn't block this patchset. 4) Write a unit test, for master, after we push this set.
Does it make sense
On 03/05/2015 10:03 AM, Jakub Hrozek wrote:
On Wed, Mar 04, 2015 at 09:57:02AM +0100, Pavel Březina wrote:
On 03/03/2015 01:03 PM, Jakub Hrozek wrote:
On Thu, Feb 19, 2015 at 12:18:13PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2346
Hi, the first patch add support for subdomains, which was missing in current implementation. It probably wasn't a big deal with netgroups but I think it would be with users and groups.
The majority of these patches refactors the current be_refresh to make support of other object types than netgroups a lot easier and straightforward.
Code review so far, no testing was done. I'm just sending my notes so you can reply while I test the patches.
From e744e3dbeab301c149accbff521cbc5232c6a48d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:16:04 +0100 Subject: [PATCH 1/6] be_refresh: refresh all domains in backend
src/providers/dp_refresh.c | 82 ++++++++++++++++++++++++---------------
Can you also write a test for this module while we're changing it?
Well, there is a ticket for it. But the main problem is how do you want to write a meaningful test?
The be_refresh module consist of:
be_refresh_send (public)
- fetch expired objects (internal)
- refresh them (internal)
The only thing you can test is that sdap was actually called which isn't really worth the time IMHO. Or do you have other idea?
I was thinking about: - adding expired entries to cache when the test starts - having a dummy ptask that marks the entries as current
Then the test would run a single refresh and verify that the ptask actually ran and the entries were found by the matching functions.
Again, this would be for master and would verify that the be_refresh_get_* functions works and the refresh works at all.
src/providers/dp_refresh.h | 1 + src/providers/ldap/ldap_common.h | 1 + src/providers/ldap/sdap_refresh.c | 15 +++++-- 4 files changed, 64 insertions(+), 35 deletions(-)
diff --git a/src/providers/dp_refresh.c b/src/providers/dp_refresh.c index 817b6213ca47bba3fa34ce28fdcd1621d349b651..bd02d0cd99f9a061109f0c17797c6e018d602dc5 100644 --- a/src/providers/dp_refresh.c +++ b/src/providers/dp_refresh.c @@ -117,6 +117,7 @@ typedef errno_t
struct be_refresh_cb {
- const char *name; bool enabled; be_refresh_get_values_t get_values; be_refresh_send_t send_fn;
@@ -137,6 +138,7 @@ struct be_refresh_ctx *be_refresh_ctx_init(TALLOC_CTX *mem_ctx) return NULL; }
- ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].name = "netgroups"; ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].get_values \ = be_refresh_get_netgroups;
In retrospective I wish we didn't use a pointer function here. A pointer function might make sense in cases where the function might be from a different provider for instance, but the sysdb expiration will be always read in the same manner. A switch-case here would be much simpler and allow for easier code browsing.
Yes... do you want me to write a patch?
Yes, for master, it will make the code more readable. Let's not delay this patchset any longer.
I also wonder if be_refresh_send needs the be_ctx parameter at all, since be_ctx is also part of the be_ptask structure?
It is signature for be_ptask send functions.
I know the two remarks above are not strictly related to your patches, but I had to re-read the be_refresh code to make sure I understand it :-)
The rest of the patch looks good to me.
I will wait until you finish testing, since the issues are below are trivial.
From 85b2f150134e89672f764e7b267f7f47a285938f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:49:17 +0100 Subject: [PATCH 2/6] sdap_handle_acct_req_send: remove be_req
be_req was used only as a talloc context for subreq. This memory context was replace by state of the parent request which is more suitable for tevent coding style.
ACK. I traced the parent contexts all the way up and confirmed that in fact nothing changes, be_req is always used as the top-level state.
From a92f1ffac23e90cfff848a05ef9611fa021b78a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 16 Feb 2015 13:42:02 +0100 Subject: [PATCH 3/6] be_refresh: refactor netgroups refresh
[...]
- state->account_req = talloc_zero(state, struct be_acct_req);
- if (state->account_req == NULL) {
ret = ENOMEM;goto immediately;- }
- state->account_req->entry_type = entry_type;
- state->account_req->attr_type = BE_ATTR_ALL;
I know that this attribute is currently not used really, but can you use BE_ATTR_CORE here instead? IIRC all the other invokers use _CORE as well, so using it here as well might make any future refactoring easier.
From 4cff90263b247d8701de33ad1cf76db95698659a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:04:09 +0100 Subject: [PATCH 4/6] be_refresh: add sdap_refresh_init
[...]
@@ -179,14 +179,11 @@ static int ldap_id_init_internal(struct be_ctx *bectx, }
/* setup periodical refresh of expired records */
- ret = be_refresh_add_cb(bectx->refresh_ctx, BE_REFRESH_TYPE_NETGROUPS,
sdap_refresh_netgroups_send,sdap_refresh_netgroups_recv,ctx);- if (ret != EOK && ret != EEXIST) {
DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh of netgroups ""will not work [%d]: %s\n", ret, strerror(ret));- }
- ret = sdap_refresh_init(bectx->refresh_ctx, ctx);
if (ret != EOK && ret != EEXIST) {DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh ""will not work [%d]: %s\n", ret, strerror(ret));}Wrong indent, otherwise patch LGTM.
From 42efd926f41c4691758e5871fd3a3330802e7f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:00:52 +0100 Subject: [PATCH 5/6] be_refresh: support users
Resolves: https://fedorahosted.org/sssd/ticket/2346
src/db/sysdb.c | 7 +++++++ src/db/sysdb.h | 2 ++ src/providers/dp_refresh.c | 23 +++++++++++++++++++++++ src/providers/dp_refresh.h | 1 + src/providers/ldap/sdap_refresh.c | 29 +++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 61a2240016b5cb77e6fbbc3286fd1a194c5a0b48..2bb4a41aa4a9e6201ac27ac8d9a1803c1fb5c43e 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -172,6 +172,13 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom, return dn; }
+struct ldb_dn *sysdb_user_base_dn(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom)+{
- return ldb_dn_new_fmt(mem_ctx, dom->sysdb->ldb,
SYSDB_TMPL_USER_BASE, dom->name);+}
I don't like adding a convenience function and then only using it on one place. Either use ldb_dn_new_fmt() directly or convert the uses of ldb_dn_new_fmt to this new convenience function.
How did you decide about the convenience function? I'm fine with keeping it, but then it must be used on other places in the code so that there's one pattern to construct the base DN, not two. That's probably out of scope of this changeset and can be done in master.
From 20bcc5371af8e429b46b0289ef9fd517c1e932c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:10:23 +0100 Subject: [PATCH 6/6] be_refresh: support groups
nack after testing :-)
You need to handle group in the sdap code: @@ -78,6 +78,9 @@ static struct tevent_req *sdap_refresh_send(TALLOC_CTX *mem_ctx, case BE_REQ_USER: state->type = "user"; break;
- case BE_REQ_GROUP:
state->type = "group";break; case BE_REQ_NETGROUP: state->type = "netgroup"; break;To sum, up we need to 1) Fix the group refresh 2) Fix the BE_ATTR and indent 3) agree about the sysdb convenience function. I'm fine with keeping it and write a patch for master that uses the function elsewhere as well. The subsequent sysdb patch wouldn't block this patchset. 4) Write a unit test, for master, after we push this set.
Does it make sense _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Mar 05, 2015 at 03:41:28PM +0100, Pavel Březina wrote:
On 03/05/2015 10:03 AM, Jakub Hrozek wrote:
On Wed, Mar 04, 2015 at 09:57:02AM +0100, Pavel Březina wrote:
On 03/03/2015 01:03 PM, Jakub Hrozek wrote:
On Thu, Feb 19, 2015 at 12:18:13PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2346
Hi, the first patch add support for subdomains, which was missing in current implementation. It probably wasn't a big deal with netgroups but I think it would be with users and groups.
The majority of these patches refactors the current be_refresh to make support of other object types than netgroups a lot easier and straightforward.
Code review so far, no testing was done. I'm just sending my notes so you can reply while I test the patches.
From e744e3dbeab301c149accbff521cbc5232c6a48d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:16:04 +0100 Subject: [PATCH 1/6] be_refresh: refresh all domains in backend
src/providers/dp_refresh.c | 82 ++++++++++++++++++++++++---------------
Can you also write a test for this module while we're changing it?
Well, there is a ticket for it. But the main problem is how do you want to write a meaningful test?
The be_refresh module consist of:
be_refresh_send (public)
- fetch expired objects (internal)
- refresh them (internal)
The only thing you can test is that sdap was actually called which isn't really worth the time IMHO. Or do you have other idea?
I was thinking about: - adding expired entries to cache when the test starts - having a dummy ptask that marks the entries as current
Then the test would run a single refresh and verify that the ptask actually ran and the entries were found by the matching functions.
Again, this would be for master and would verify that the be_refresh_get_* functions works and the refresh works at all.
src/providers/dp_refresh.h | 1 + src/providers/ldap/ldap_common.h | 1 + src/providers/ldap/sdap_refresh.c | 15 +++++-- 4 files changed, 64 insertions(+), 35 deletions(-)
diff --git a/src/providers/dp_refresh.c b/src/providers/dp_refresh.c index 817b6213ca47bba3fa34ce28fdcd1621d349b651..bd02d0cd99f9a061109f0c17797c6e018d602dc5 100644 --- a/src/providers/dp_refresh.c +++ b/src/providers/dp_refresh.c @@ -117,6 +117,7 @@ typedef errno_t
struct be_refresh_cb {
- const char *name; bool enabled; be_refresh_get_values_t get_values; be_refresh_send_t send_fn;
@@ -137,6 +138,7 @@ struct be_refresh_ctx *be_refresh_ctx_init(TALLOC_CTX *mem_ctx) return NULL; }
- ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].name = "netgroups"; ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].get_values \ = be_refresh_get_netgroups;
In retrospective I wish we didn't use a pointer function here. A pointer function might make sense in cases where the function might be from a different provider for instance, but the sysdb expiration will be always read in the same manner. A switch-case here would be much simpler and allow for easier code browsing.
Yes... do you want me to write a patch?
Yes, for master, it will make the code more readable. Let's not delay this patchset any longer.
I also wonder if be_refresh_send needs the be_ctx parameter at all, since be_ctx is also part of the be_ptask structure?
It is signature for be_ptask send functions.
I know the two remarks above are not strictly related to your patches, but I had to re-read the be_refresh code to make sure I understand it :-)
The rest of the patch looks good to me.
I will wait until you finish testing, since the issues are below are trivial.
From 85b2f150134e89672f764e7b267f7f47a285938f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:49:17 +0100 Subject: [PATCH 2/6] sdap_handle_acct_req_send: remove be_req
be_req was used only as a talloc context for subreq. This memory context was replace by state of the parent request which is more suitable for tevent coding style.
ACK. I traced the parent contexts all the way up and confirmed that in fact nothing changes, be_req is always used as the top-level state.
From a92f1ffac23e90cfff848a05ef9611fa021b78a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 16 Feb 2015 13:42:02 +0100 Subject: [PATCH 3/6] be_refresh: refactor netgroups refresh
[...]
- state->account_req = talloc_zero(state, struct be_acct_req);
- if (state->account_req == NULL) {
ret = ENOMEM;goto immediately;- }
- state->account_req->entry_type = entry_type;
- state->account_req->attr_type = BE_ATTR_ALL;
I know that this attribute is currently not used really, but can you use BE_ATTR_CORE here instead? IIRC all the other invokers use _CORE as well, so using it here as well might make any future refactoring easier.
From 4cff90263b247d8701de33ad1cf76db95698659a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:04:09 +0100 Subject: [PATCH 4/6] be_refresh: add sdap_refresh_init
[...]
@@ -179,14 +179,11 @@ static int ldap_id_init_internal(struct be_ctx *bectx, }
/* setup periodical refresh of expired records */
- ret = be_refresh_add_cb(bectx->refresh_ctx, BE_REFRESH_TYPE_NETGROUPS,
sdap_refresh_netgroups_send,sdap_refresh_netgroups_recv,ctx);- if (ret != EOK && ret != EEXIST) {
DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh of netgroups ""will not work [%d]: %s\n", ret, strerror(ret));- }
- ret = sdap_refresh_init(bectx->refresh_ctx, ctx);
if (ret != EOK && ret != EEXIST) {DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh ""will not work [%d]: %s\n", ret, strerror(ret));}Wrong indent, otherwise patch LGTM.
From 42efd926f41c4691758e5871fd3a3330802e7f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:00:52 +0100 Subject: [PATCH 5/6] be_refresh: support users
Resolves: https://fedorahosted.org/sssd/ticket/2346
src/db/sysdb.c | 7 +++++++ src/db/sysdb.h | 2 ++ src/providers/dp_refresh.c | 23 +++++++++++++++++++++++ src/providers/dp_refresh.h | 1 + src/providers/ldap/sdap_refresh.c | 29 +++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 61a2240016b5cb77e6fbbc3286fd1a194c5a0b48..2bb4a41aa4a9e6201ac27ac8d9a1803c1fb5c43e 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -172,6 +172,13 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom, return dn; }
+struct ldb_dn *sysdb_user_base_dn(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom)+{
- return ldb_dn_new_fmt(mem_ctx, dom->sysdb->ldb,
SYSDB_TMPL_USER_BASE, dom->name);+}
I don't like adding a convenience function and then only using it on one place. Either use ldb_dn_new_fmt() directly or convert the uses of ldb_dn_new_fmt to this new convenience function.
How did you decide about the convenience function? I'm fine with keeping it, but then it must be used on other places in the code so that there's one pattern to construct the base DN, not two. That's probably out of scope of this changeset and can be done in master.
From 20bcc5371af8e429b46b0289ef9fd517c1e932c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:10:23 +0100 Subject: [PATCH 6/6] be_refresh: support groups
nack after testing :-)
You need to handle group in the sdap code: @@ -78,6 +78,9 @@ static struct tevent_req *sdap_refresh_send(TALLOC_CTX *mem_ctx, case BE_REQ_USER: state->type = "user"; break;
- case BE_REQ_GROUP:
state->type = "group";break;To sum, up we need to 1) Fix the group refresh 2) Fix the BE_ATTR and indent 3) agree about the sysdb convenience function. I'm fine with keeping it and write a patch for master that uses the function elsewhere as well. The subsequent sysdb patch wouldn't block this patchset. 4) Write a unit test, for master, after we push this set.
Does it make sense _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 98689445df6ee0a3c3589eb5c1346ce19ebcc8aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:16:04 +0100 Subject: [PATCH 1/8] be_refresh: refresh all domains in backend
ACK
From 5c4944ee9c582439811abce958832490c006d7a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:49:17 +0100 Subject: [PATCH 2/8] sdap_handle_acct_req_send: remove be_req
ACK
From e2fbec112fee67ca094c27b702c12f1c7f553728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 16 Feb 2015 13:42:02 +0100 Subject: [PATCH 3/8] be_refresh: refactor netgroups refresh
ACK
From 8696088315cda39ef4c63eb99235c973fe840c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:04:09 +0100 Subject: [PATCH 4/8] be_refresh: add sdap_refresh_init
ACK
From a3971fb7f504af84def4419af0ed21ab74f6b268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:00:52 +0100 Subject: [PATCH 5/8] be_refresh: support users
ACK
From 0fa8b678e03fb99f73ffc1684092360ffa54f781 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:10:23 +0100 Subject: [PATCH 6/8] be_refresh: support groups
ACK
From 7d165d350f9d757bd9e6bdf434af39c0db359c82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 5 Mar 2015 15:29:07 +0100 Subject: [PATCH 7/8] be_refresh: get rid of callback pointers
ACK (master only)
From a584c8b0d46c7ad7b8907c4739c8cd69ef47a6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 5 Mar 2015 15:40:41 +0100 Subject: [PATCH 8/8] sysdb: use sysdb_user/group_dn
What about src/db/sysdb.c:989?
I'll pushed the acked patches already in order to not delay any further. Only the use sysdb_user/group_dn is left pending.
CI link: http://sssd-ci.duckdns.org/logs/job/9/02/summary.html
* master: * 61c8d13e55ebafc28da1b0b5ad9ae578d687e288 * e77d6366ff9e49dbbb607f1709f1ae4190b99489 * 17531a398cc9084036cb08d69fe876a8f12707bb * ab0eda3622b828df2bfb7850c96d1395f614eb13 * a849d848d53f305a90613a74c1767a42b250deda * b0d3164ca2bd842e176268c26935c5ce54f7f76e * 26d6ed2f190817b77df7c5b0041515f60ec2fb46 * sssd-1-12: * 0a26dd82639cd3fc80433d19f4bb7363db7975e2 * 40f5e40aa0bb9aa6b80b547e9643bebf53f7620a * 4c714a37865979f03c56d82d5984558a63c392da * dd9dfa7ddc257b09a73252fffe7cb4d002f5990a * 4d0286e4f7701974f8f7c3ead76a2ab5a93f6ffe * 20b08bcfd6740316f528ca84d3a69be9a6535945
On 03/09/2015 01:11 PM, Jakub Hrozek wrote:
On Thu, Mar 05, 2015 at 03:41:28PM +0100, Pavel Březina wrote:
On 03/05/2015 10:03 AM, Jakub Hrozek wrote:
On Wed, Mar 04, 2015 at 09:57:02AM +0100, Pavel Březina wrote:
On 03/03/2015 01:03 PM, Jakub Hrozek wrote:
On Thu, Feb 19, 2015 at 12:18:13PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2346
Hi, the first patch add support for subdomains, which was missing in current implementation. It probably wasn't a big deal with netgroups but I think it would be with users and groups.
The majority of these patches refactors the current be_refresh to make support of other object types than netgroups a lot easier and straightforward.
Code review so far, no testing was done. I'm just sending my notes so you can reply while I test the patches.
From e744e3dbeab301c149accbff521cbc5232c6a48d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:16:04 +0100 Subject: [PATCH 1/6] be_refresh: refresh all domains in backend
src/providers/dp_refresh.c | 82 ++++++++++++++++++++++++---------------
Can you also write a test for this module while we're changing it?
Well, there is a ticket for it. But the main problem is how do you want to write a meaningful test?
The be_refresh module consist of:
be_refresh_send (public)
- fetch expired objects (internal)
- refresh them (internal)
The only thing you can test is that sdap was actually called which isn't really worth the time IMHO. Or do you have other idea?
I was thinking about: - adding expired entries to cache when the test starts - having a dummy ptask that marks the entries as current
Then the test would run a single refresh and verify that the ptask actually ran and the entries were found by the matching functions.
Again, this would be for master and would verify that the be_refresh_get_* functions works and the refresh works at all.
src/providers/dp_refresh.h | 1 + src/providers/ldap/ldap_common.h | 1 + src/providers/ldap/sdap_refresh.c | 15 +++++-- 4 files changed, 64 insertions(+), 35 deletions(-)
diff --git a/src/providers/dp_refresh.c b/src/providers/dp_refresh.c index 817b6213ca47bba3fa34ce28fdcd1621d349b651..bd02d0cd99f9a061109f0c17797c6e018d602dc5 100644 --- a/src/providers/dp_refresh.c +++ b/src/providers/dp_refresh.c @@ -117,6 +117,7 @@ typedef errno_t
struct be_refresh_cb {
- const char *name; bool enabled; be_refresh_get_values_t get_values; be_refresh_send_t send_fn;
@@ -137,6 +138,7 @@ struct be_refresh_ctx *be_refresh_ctx_init(TALLOC_CTX *mem_ctx) return NULL; }
- ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].name = "netgroups"; ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].get_values \ = be_refresh_get_netgroups;
In retrospective I wish we didn't use a pointer function here. A pointer function might make sense in cases where the function might be from a different provider for instance, but the sysdb expiration will be always read in the same manner. A switch-case here would be much simpler and allow for easier code browsing.
Yes... do you want me to write a patch?
Yes, for master, it will make the code more readable. Let's not delay this patchset any longer.
I also wonder if be_refresh_send needs the be_ctx parameter at all, since be_ctx is also part of the be_ptask structure?
It is signature for be_ptask send functions.
I know the two remarks above are not strictly related to your patches, but I had to re-read the be_refresh code to make sure I understand it :-)
The rest of the patch looks good to me.
I will wait until you finish testing, since the issues are below are trivial.
From 85b2f150134e89672f764e7b267f7f47a285938f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:49:17 +0100 Subject: [PATCH 2/6] sdap_handle_acct_req_send: remove be_req
be_req was used only as a talloc context for subreq. This memory context was replace by state of the parent request which is more suitable for tevent coding style.
ACK. I traced the parent contexts all the way up and confirmed that in fact nothing changes, be_req is always used as the top-level state.
From a92f1ffac23e90cfff848a05ef9611fa021b78a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 16 Feb 2015 13:42:02 +0100 Subject: [PATCH 3/6] be_refresh: refactor netgroups refresh
[...]
- state->account_req = talloc_zero(state, struct be_acct_req);
- if (state->account_req == NULL) {
ret = ENOMEM;goto immediately;- }
- state->account_req->entry_type = entry_type;
- state->account_req->attr_type = BE_ATTR_ALL;
I know that this attribute is currently not used really, but can you use BE_ATTR_CORE here instead? IIRC all the other invokers use _CORE as well, so using it here as well might make any future refactoring easier.
From 4cff90263b247d8701de33ad1cf76db95698659a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:04:09 +0100 Subject: [PATCH 4/6] be_refresh: add sdap_refresh_init
[...]
@@ -179,14 +179,11 @@ static int ldap_id_init_internal(struct be_ctx *bectx, }
/* setup periodical refresh of expired records */
- ret = be_refresh_add_cb(bectx->refresh_ctx, BE_REFRESH_TYPE_NETGROUPS,
sdap_refresh_netgroups_send,sdap_refresh_netgroups_recv,ctx);- if (ret != EOK && ret != EEXIST) {
DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh of netgroups ""will not work [%d]: %s\n", ret, strerror(ret));- }
- ret = sdap_refresh_init(bectx->refresh_ctx, ctx);
if (ret != EOK && ret != EEXIST) {DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh ""will not work [%d]: %s\n", ret, strerror(ret));}Wrong indent, otherwise patch LGTM.
From 42efd926f41c4691758e5871fd3a3330802e7f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:00:52 +0100 Subject: [PATCH 5/6] be_refresh: support users
Resolves: https://fedorahosted.org/sssd/ticket/2346
src/db/sysdb.c | 7 +++++++ src/db/sysdb.h | 2 ++ src/providers/dp_refresh.c | 23 +++++++++++++++++++++++ src/providers/dp_refresh.h | 1 + src/providers/ldap/sdap_refresh.c | 29 +++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 61a2240016b5cb77e6fbbc3286fd1a194c5a0b48..2bb4a41aa4a9e6201ac27ac8d9a1803c1fb5c43e 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -172,6 +172,13 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom, return dn; }
+struct ldb_dn *sysdb_user_base_dn(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom)+{
- return ldb_dn_new_fmt(mem_ctx, dom->sysdb->ldb,
SYSDB_TMPL_USER_BASE, dom->name);+}
I don't like adding a convenience function and then only using it on one place. Either use ldb_dn_new_fmt() directly or convert the uses of ldb_dn_new_fmt to this new convenience function.
How did you decide about the convenience function? I'm fine with keeping it, but then it must be used on other places in the code so that there's one pattern to construct the base DN, not two. That's probably out of scope of this changeset and can be done in master.
From 20bcc5371af8e429b46b0289ef9fd517c1e932c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:10:23 +0100 Subject: [PATCH 6/6] be_refresh: support groups
nack after testing :-)
You need to handle group in the sdap code: @@ -78,6 +78,9 @@ static struct tevent_req *sdap_refresh_send(TALLOC_CTX *mem_ctx, case BE_REQ_USER: state->type = "user"; break;
- case BE_REQ_GROUP:
state->type = "group";break; case BE_REQ_NETGROUP: state->type = "netgroup"; break;To sum, up we need to 1) Fix the group refresh 2) Fix the BE_ATTR and indent 3) agree about the sysdb convenience function. I'm fine with keeping it and write a patch for master that uses the function elsewhere as well. The subsequent sysdb patch wouldn't block this patchset. 4) Write a unit test, for master, after we push this set.
Does it make sense _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 98689445df6ee0a3c3589eb5c1346ce19ebcc8aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:16:04 +0100 Subject: [PATCH 1/8] be_refresh: refresh all domains in backend
ACK
From 5c4944ee9c582439811abce958832490c006d7a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 13 Feb 2015 13:49:17 +0100 Subject: [PATCH 2/8] sdap_handle_acct_req_send: remove be_req
ACK
From e2fbec112fee67ca094c27b702c12f1c7f553728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 16 Feb 2015 13:42:02 +0100 Subject: [PATCH 3/8] be_refresh: refactor netgroups refresh
ACK
From 8696088315cda39ef4c63eb99235c973fe840c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:04:09 +0100 Subject: [PATCH 4/8] be_refresh: add sdap_refresh_init
ACK
From a3971fb7f504af84def4419af0ed21ab74f6b268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:00:52 +0100 Subject: [PATCH 5/8] be_refresh: support users
ACK
From 0fa8b678e03fb99f73ffc1684092360ffa54f781 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 19 Feb 2015 12:10:23 +0100 Subject: [PATCH 6/8] be_refresh: support groups
ACK
From 7d165d350f9d757bd9e6bdf434af39c0db359c82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 5 Mar 2015 15:29:07 +0100 Subject: [PATCH 7/8] be_refresh: get rid of callback pointers
ACK (master only)
From a584c8b0d46c7ad7b8907c4739c8cd69ef47a6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 5 Mar 2015 15:40:41 +0100 Subject: [PATCH 8/8] sysdb: use sysdb_user/group_dn
What about src/db/sysdb.c:989?
You don't have domain object available there nor a way how to obtain it, since the only parameters are sysdb and domain_name.
I'll pushed the acked patches already in order to not delay any further. Only the use sysdb_user/group_dn is left pending.
CI link: http://sssd-ci.duckdns.org/logs/job/9/02/summary.html
- master:
- 61c8d13e55ebafc28da1b0b5ad9ae578d687e288
- e77d6366ff9e49dbbb607f1709f1ae4190b99489
- 17531a398cc9084036cb08d69fe876a8f12707bb
- ab0eda3622b828df2bfb7850c96d1395f614eb13
- a849d848d53f305a90613a74c1767a42b250deda
- b0d3164ca2bd842e176268c26935c5ce54f7f76e
- 26d6ed2f190817b77df7c5b0041515f60ec2fb46
- sssd-1-12:
- 0a26dd82639cd3fc80433d19f4bb7363db7975e2
- 40f5e40aa0bb9aa6b80b547e9643bebf53f7620a
- 4c714a37865979f03c56d82d5984558a63c392da
- dd9dfa7ddc257b09a73252fffe7cb4d002f5990a
- 4d0286e4f7701974f8f7c3ead76a2ab5a93f6ffe
- 20b08bcfd6740316f528ca84d3a69be9a6535945
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Mar 09, 2015 at 01:29:58PM +0100, Pavel Březina wrote:
From a584c8b0d46c7ad7b8907c4739c8cd69ef47a6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 5 Mar 2015 15:40:41 +0100 Subject: [PATCH 8/8] sysdb: use sysdb_user/group_dn
What about src/db/sysdb.c:989?
You don't have domain object available there nor a way how to obtain it, since the only parameters are sysdb and domain_name.
Oops, right.
ACK
On Mon, Mar 09, 2015 at 01:38:13PM +0100, Jakub Hrozek wrote:
On Mon, Mar 09, 2015 at 01:29:58PM +0100, Pavel Březina wrote:
From a584c8b0d46c7ad7b8907c4739c8cd69ef47a6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 5 Mar 2015 15:40:41 +0100 Subject: [PATCH 8/8] sysdb: use sysdb_user/group_dn
What about src/db/sysdb.c:989?
You don't have domain object available there nor a way how to obtain it, since the only parameters are sysdb and domain_name.
Oops, right.
ACK
* master: 12a000c8c7c07259e438fb1e992134bdd07d9a30
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina