[SSSD] [PATCH] IFP: cached objects

Pavel Březina pbrezina at redhat.com
Tue Jun 2 09:18:30 UTC 2015


On 05/26/2015 05:22 PM, Jakub Hrozek wrote:
> 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?= <pbrezina at 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..)

Index was omitted.

> I would prefer a different name for the new attribute, cache is too generic,
> can we prefix the name?

Renamed to ifp_cached.

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

I don't see how those warning happends... base_dn, class and paths is 
initialized.

>>  From 3cd493aa2783f57eb8e8acb55d4129ddcfac47de Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at 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?= <pbrezina at 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?= <pbrezina at 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-IFP-Implement-org.freedesktop.sssd.infopipe.Cache-.O.patch
Type: text/x-patch
Size: 32402 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150602/dc4d79bd/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SBUS-Use-default-GetAll-invoker-if-none-is-set.patch
Type: text/x-patch
Size: 1235 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150602/dc4d79bd/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-SBUS-Add-support-for-node-in-introspection.patch
Type: text/x-patch
Size: 10837 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150602/dc4d79bd/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-IFP-Export-nodes.patch
Type: text/x-patch
Size: 11182 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150602/dc4d79bd/attachment-0003.bin>


More information about the sssd-devel mailing list