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(a)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(a)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(a)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(a)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(a)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(a)redhat.com>
>>>>> Date: Thu, 19 Feb 2015 12:10:23 +0100
>>>>> Subject: [PATCH 6/6] be_refresh: support groups
>>>>>
>>>>> Resolves:
>>>>>
https://fedorahosted.org/sssd/ticket/2346
>>
>> 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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel