On Thu, May 21, 2015 at 11:44:08AM +0200, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/2338
>
> Depends on IFP: users and groups
I haven't tested the code yet at all, but let's start the review :-)
> From b08f37a1bf9593c74cdec8d829f0d125b9c66c30 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Mon, 18 May 2015 12:05:38 +0200
> Subject: [PATCH 1/4] IFP: Implement
> org.freedesktop.sssd.infopipe.Cache[.Object]
This patch need to be split into one that just adds the functionality
and one that adds the sysdb upgrade.
But mostly, I'm not sure we need to index this attribute, do you think
the lookups for cached objects would be so frequent? Keep in mind that
indexing adds some penalty on the cache size but also building the
indexes is not totally for free, so we should carefully consider the
indexes..
Additionally, we were talking with Sumit about sysdb upgrades for
indexing attributes with the context being:
https://fedorahosted.org/sssd/ticket/2597
What Sumit proposed to investigate if we could get away of not bumping
the version (or adding a .z version to the sysdb string) if all we
change is indexes..do you have time to test that? (newer indexes with
older client etc..)
I would prefer a different name for the new attribute, cache is too generic,
can we prefix the name?
Either way, I think we should add indexes for both in a single patch,
expecially if we end up doing the upgrade.
There is a compilation warning discovered by Coverity:
Error: COMPILER_WARNING:
sssd-1.12.90/src/responder/ifp/ifp_cache.c:118:13: warning: 'base_dn' may be
used uninitialized in this function [-Wmaybe-uninitialized]
# 116| }
# 117|
# 118|-> ldb_ret = ldb_search(sysdb_ctx_get_ldb(domain->sysdb), tmp_ctx,
&result,
# 119| base_dn, LDB_SCOPE_SUBTREE, attrs,
# 120| "(&(objectClass=%s)(%s=TRUE))", class,
SYSDB_CACHED);
Error: COMPILER_WARNING:
sssd-1.12.90/src/responder/ifp/ifp_cache.c:118:13: warning: 'class' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# ldb_ret = ldb_search(sysdb_ctx_get_ldb(domain->sysdb), tmp_ctx, &result,
# ^
# 116| }
# 117|
# 118|-> ldb_ret = ldb_search(sysdb_ctx_get_ldb(domain->sysdb), tmp_ctx,
&result,
# 119| base_dn, LDB_SCOPE_SUBTREE, attrs,
# 120| "(&(objectClass=%s)(%s=TRUE))", class,
SYSDB_CACHED);
Error: COMPILER_WARNING:
sssd-1.12.90/src/responder/ifp/ifp_cache.c: scope_hint: In function
'ifp_cache_get_cached_objects'
sssd-1.12.90/src/responder/ifp/ifp_cache.c:134:18: warning: 'path' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# paths[i] = ifp_cache_build_path(paths, type, domain, result->msgs[i]);
# ^
# 132|
# 133| for (i = 0; i < result->count; i++) {
# 134|-> paths[i] = ifp_cache_build_path(paths, type, domain,
result->msgs[i]);
# 135| if (paths[i] == NULL) {
# 136| ret = ENOMEM;
> From 3cd493aa2783f57eb8e8acb55d4129ddcfac47de Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Wed, 20 May 2015 13:34:46 +0200
> Subject: [PATCH 2/4] SBUS: Use default GetAll invoker if none is set
ACK (code-wise)
> From 9c13aa15cbe377158356b7ec87e98c0fec0e5791 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Wed, 20 May 2015 14:14:53 +0200
> Subject: [PATCH 3/4] SBUS: Add support for <node /> in introspection
One issue:
> +const char **
> +sbus_nodes_hash_lookup(TALLOC_CTX *mem_ctx,
> + hash_table_t *table,
> + const char *object_path)
> +{
> + struct sbus_nodes_data *data;
> + hash_key_t key;
> + hash_value_t value;
> + int hret;
> +
> + key.type = HASH_KEY_STRING;
> + key.str = discard_const(object_path);
> +
> + hret = hash_lookup(table, &key, &value);
> + if (hret == HASH_ERROR_KEY_NOT_FOUND) {
> + return NULL;
> + } else if (hret != HASH_SUCCESS) {
> + DEBUG(SSSDBG_OP_FAILURE,
> + "Unable to search hash table: hret=%d\n", hret);
> + return NULL;
> + }
> +
> + data = (struct sbus_nodes_data *)(value.ptr);
Can you use talloc_get_type() here?
> +
> + return data->nodes_fn(mem_ctx, object_path, data->handler_data);
> +}
> +
> From 745f2a915c6920333c0c5594ab0ab1b37d81dabf Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Wed, 20 May 2015 14:45:25 +0200
> Subject: [PATCH 4/4] IFP: Export nodes
>
> IFP now exports cached users and groups in introspection.
> ---
> Makefile.am | 1 +
> src/responder/ifp/ifp_cache.c | 78 ++++++++++++++++------
> src/responder/ifp/ifp_cache.h | 10 ++-
> src/responder/ifp/ifp_iface_nodes.c | 129
> ++++++++++++++++++++++++++++++++++++
> src/responder/ifp/ifp_private.h | 2 +
> src/responder/ifp/ifpsrv.c | 2 +
> 6 files changed, 200 insertions(+), 22 deletions(-)
> create mode 100644 src/responder/ifp/ifp_iface_nodes.c
>
> diff --git a/Makefile.am b/Makefile.am
> index
> de0cf9738ad399eb8ebce8968082a7dc1913d06a..31f420ad5b47c11436cde94bd0fff5e87e974b29
> 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1086,6 +1086,7 @@ sssd_ifp_SOURCES = \
> src/responder/ifp/ifp_iface_generated.c \
> src/responder/ifp/ifp_iface_generated.h \
> src/responder/ifp/ifp_iface.c \
> + src/responder/ifp/ifp_iface_nodes.c \
> src/responder/ifp/ifpsrv_util.c \
> src/responder/ifp/ifp_domains.c \
> src/responder/ifp/ifp_components.c \
> diff --git a/src/responder/ifp/ifp_cache.c b/src/responder/ifp/ifp_cache.c
> index
> 498e75595edb114ccde7c0fec9dc689a2d8fd82d..97e8e26c4aabe41975b8ef1286773ec10c0c9134
> 100644
Do the ifp_cache.c changes belong to this patch or the first one?
The rest looks good to me.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
