[SSSD] [PATCHES] be_refresh: support users and groups

Jakub Hrozek jhrozek at redhat.com
Tue Mar 3 12:03:08 UTC 2015


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 at 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 at 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 at 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 at 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 at 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 at 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

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?



More information about the sssd-devel mailing list