Title: #246: filter_users and filter_groups stop working properly in v 1.15
fidencio commented:
"""
On Thu, Apr 27, 2017 at 1:29 PM, Fabiano Fidêncio <fidencio(a)redhat.com>
wrote:
On Thu, Apr 27, 2017 at 1:26 PM, Pavel Březina <notifications(a)github.com>
wrote:
> *@pbrezina* requested changes on this pull request.
> ------------------------------
>
> In src/responder/common/cache_req/cache_req_search.c
> <
https://github.com/SSSD/sssd/pull/246#discussion_r113672158>:
>
> > + if (cr->plugin->ncache_filter_fn == NULL) {
> + CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
> + "This request type does not support filtering negative
cache\n");
> + *_result = result;
> + return EOK;
> + }
> +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + return ENOMEM;
> + }
> +
> + CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
> + "Filtering out results from negative cache\n");
> +
> + filtered_result = talloc_zero(mem_ctx, struct ldb_result);
>
> Unused at this moment.
> ------------------------------
>
> In src/responder/common/cache_req/cache_req_search.c
> <
https://github.com/SSSD/sssd/pull/246#discussion_r113672504>:
>
> > +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + return ENOMEM;
> + }
> +
> + CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
> + "Filtering out results from negative cache\n");
> +
> + filtered_result = talloc_zero(mem_ctx, struct ldb_result);
> + if (filtered_result == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + msgs = talloc_zero(tmp_ctx, struct ldb_message *);
>
> Because this function will filter out only users and groups from the
> configuration file, there won't be many objects filtered out. But there
> will be many allocations with enumeration. It would be better to allocate
> enough space to hold all the messages at once to safe some allocations. I.e.
>
> msgs = talloc_zero_array(tmp_ctx, struct ldb_message *, result->count)
>
> And remove the reallocs.
> ------------------------------
>
> In src/responder/common/cache_req/cache_req_search.c
> <
https://github.com/SSSD/sssd/pull/246#discussion_r113672787>:
>
> > + TALLOC_CTX *tmp_ctx;
> + const char *name;
> + char *cased_name;
> + errno_t ret;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + return ENOMEM;
> + }
> +
> + name = sss_get_name_from_msg(domain, msg);
> + if (name == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "sss_get_name_from_msg() returned NULL, which should never
"
> + "happen in this scenario!\n");
> + ret = ENOMEM;
>
> ERR_INTERNAL since it is corrupted cache.
> ------------------------------
>
> In src/responder/common/cache_req/cache_req_search.c
> <
https://github.com/SSSD/sssd/pull/246#discussion_r113672851>:
>
> > +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + return ENOMEM;
> + }
> +
> + name = sss_get_name_from_msg(domain, msg);
> + if (name == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "sss_get_name_from_msg() returned NULL, which should never
"
> + "happen in this scenario!\n");
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + cased_name = sss_get_cased_name(tmp_ctx, name, domain->case_preserve);
>
> You used sss_output_name in previous patch set. Why did you change it?
>
I realized that getting sss_output_name() doesn't make much sense here.
We're just interested in the fully-qualified name (which we already have
here).
So, the whole part of sss_output_name() + sss_create_internal_fqname() is
unnecessary here.
Do you agree?
I have updated the PR with the other issues you have noticed.
One thing that is not exactly clear to me (and I may need your guidance) is
... just getting the name from the DB is not enough to the the
fully-qualified name that we added to the negative cache? Do I really have
to get the cased name and replace the space?
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <
https://github.com/SSSD/sssd/pull/246#pullrequestreview-35079762>, or mute
> the thread
>
<
https://github.com/notifications/unsubscribe-auth/AAG4eujB3Aq3lA4kxmVS4g5...
> .
>